fix: isolation level and primary user reservation by sattvikc · Pull Request #288 · supertokens/supertokens-postgresql-plugin

sattvikc · 2025-11-27T10:39:24Z

Summary of change

(A few sentences about this PR)

Related issues

Link to issue1 here
Link to issue1 here

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your
changes work. Bonus points for screenshots and videos!)

Documentation changes

(If relevant, please create a PR in our docs repo, or create a checklist here
highlighting the necessary changes)

Checklist for important updates

Changelog has been updated
pluginInterfaceSupported.json file has been updated (if needed)
Changes to the version if needed
- In build.gradle
Had installed and ran the pre-commit hook
If there are new dependencies that have been added in build.gradle, please make sure to add them
in implementationDependencies.json.
Issue this PR against the latest non released version branch.
- To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the
  latest branch (git branch --all) whose X.Y is greater than the latest released tag.
- If no such branch exists, then create one from the latest released branch.
When adding new recipes, ensure that its performance is being measured in the OneMillionUsersTest

Remaining TODOs for this PR

Item1
Item2

porcellus · 2025-12-15T20:29:27Z

            GeneralQueries.linkAccounts_Transaction(this, sqlCon, appIdentifier, recipeUserId, primaryUserId);
+            AccountInfoQueries.reserveAccountInfoForLinking_Transaction(this, sqlCon, appIdentifier, recipeUserId, primaryUserId);


Is the idea here that we can just drop the GeneralQueries one sooner or later?

porcellus · 2025-12-15T20:33:57Z

+                    {
+                        io.supertokens.storage.postgresql.queries.Utils.takeAdvisoryLock(
+                                sqlCon, tenantConfig.tenantIdentifier.getConnectionUriDomain() + "~" + tenantConfig.tenantIdentifier.getAppId() + "~" + tenantConfig.tenantIdentifier.getTenantId());
+                    }


Do we need this? What do we want to achieve with this here?

tenant information is updated by deleting all existing information and then re-adding it (across multiple tables). we do not want any parallel createOrUpdate for a given tenant.

porcellus · 2025-12-15T20:35:52Z

+                    // Insert row for third party id
+                    AccountInfoQueries.addRecipeUserAccountInfo_Transaction(start, sqlCon, tenantIdentifier, id,
+                            THIRD_PARTY.toString(), ACCOUNT_INFO_TYPE.THIRD_PARTY, "", "",
+                            thirdParty.id + "::" + thirdParty.userId);


Do we validate the thirdParty.id cannot have "::"?

why does that even matter? we are never going to try to decode thirdparty id and user id from this value. It may matter when thirdPartyId ended with :: in one case and the user id started with ::. I don't see that as a regular usecase any way. We could use any thing in-between instead of ::, can change if you prefer anything specifically.

porcellus · 2025-12-15T20:57:18Z

+                + "account_info_value TEXT NOT NULL,"
+                + "third_party_id VARCHAR(28),"
+                + "third_party_user_id VARCHAR(256),"


We could also add a check constraint about "valid combinations" (i.e., tp+email or email or phone), but I don't think we actually need that. Please think it through and document the decision.

porcellus · 2025-12-15T21:02:04Z

+        List<Object> parameters = new ArrayList<>();
+
+        // Add app_id parameter
+        parameters.add(appIdentifier.getAppId());


Please move these right below/above where we add the ? this will replace.

no longer valid

porcellus · 2025-12-15T21:06:02Z

+        Connection sqlCon = (Connection) con.getConnection();
+
+        // Build the query dynamically based on which values are not null
+        StringBuilder QUERY = new StringBuilder("SELECT primary_user_id, account_info_type FROM " + getConfig(start).getPrimaryUserTenantsTable());


Couldn't we do this similarly to how the addPrimaryUserAccountInfo_Transaction, using a subquery based on the recipe user id?

this is now covered

porcellus · 2025-12-15T21:06:55Z

+        }
+    }
+
+    public static void checkIfLoginMethodsCanBeLinked_Transaction(Start start, TransactionConnection con, AppIdentifier appIdentifier, Set<String> tenantIds, Set<String> emails,


Please see if it makes sense to use a subquery here instead of the collected emails

porcellus · 2025-12-15T21:07:32Z

+        if (thirdParties != null && !thirdParties.isEmpty()) {
+            List<String> thirdPartyValues = new ArrayList<>();
+            for (LoginMethod.ThirdParty tp : thirdParties) {
+                thirdPartyValues.add(tp.id + "::" + tp.userId);


We should extract the concatenation into a single util function, just to ensure this stays consistent.

porcellus · 2025-12-15T21:08:24Z

+        StringBuilder QUERY = new StringBuilder("SELECT primary_user_id, account_info_type, account_info_value FROM ");
+        QUERY.append(getConfig(start).getPrimaryUserTenantsTable());
+        QUERY.append(" WHERE app_id = ? AND tenant_id IN (");
+        for (int i = 0; i < tenantIdsList.size(); i++) {
+            QUERY.append("?");
+            if (i != tenantIdsList.size() - 1) {
+                QUERY.append(",");
+            }
+        }


Please group this together with adding the params.

no longer applicable

porcellus · 2025-12-15T21:10:48Z

+        // 1) recipe user's account info -> all tenants of primary user
+        String QUERY_1 = "INSERT INTO " + primaryUserTenantsTable
+                + " (app_id, tenant_id, account_info_type, account_info_value, primary_user_id)"
+                + " SELECT ?, primary_tenants.tenant_id, recipe_ai.account_info_type, recipe_ai.account_info_value, ?"


We may want to add explicit types when using a parameter in the subselect "result" directly

Where is it done?

porcellus · 2026-01-06T15:33:30Z

+            // - Insert row for email (uses third_party_id + third_party_user_id columns)
+            recipeUserTenantsBatch.add(pst -> {
+                pst.setString(1, tenantIdentifier.getAppId());
+                pst.setString(2, user.userId);
+                pst.setString(3, tenantIdentifier.getTenantId());
+                pst.setString(4, THIRD_PARTY.toString());
+                pst.setString(5, ACCOUNT_INFO_TYPE.EMAIL.toString());
+                pst.setString(6, user.thirdpartyId);
+                pst.setString(7, user.thirdpartyUserId);
+                pst.setString(8, user.email);
+            });


Let's document why we need it, as it is not straightforward at first glance.

porcellus · 2026-01-06T15:38:26Z

+                "updateAccountInfo_Transaction should only be called with accountInfoType EMAIL or PHONE_NUMBER");
+    }
+
+    private static String[] getPrimaryUserTenantsConflictForAddTenant(Connection sqlCon, String primaryUserTenantsTable,


why is this getting the table name as a parameter?

removed unused function

porcellus · 2026-01-06T16:03:18Z

+        }
+    }
+
+    public static CanLinkAccountsResult checkIfLoginMethodsCanBeLinked_Transaction(Start start, TransactionConnection con,


Why do we need to do this in a transaction? We are not taking locks here as far as I can see.

Overall, this is a bit chaotic and hard to read/parse. Couldn't we do this using subqueries? Or would that impact performance?

porcellus · 2026-01-06T16:05:33Z

+        // 1) recipe user's account info -> all tenants of primary user
+        String QUERY_1 = "INSERT INTO " + primaryUserTenantsTable
+                + " (app_id, tenant_id, account_info_type, account_info_value, primary_user_id)"
+                + " SELECT ?, primary_tenants.tenant_id, recipe_ai.account_info_type, recipe_ai.account_info_value, ?"


Where is it done?

porcellus · 2026-01-06T20:47:46Z

+
+        // Pre-check conflicts before attempting the INSERT
+        try {
+            String accountInfoType = getRecipeUserTenantsConflictTypeForAddTenant(sqlCon, recipeUserTenantsTable, tenantIdentifier, userId);


Why do we need this?

porcellus · 2026-01-06T20:49:24Z

+
+        // Pre-check conflicts before attempting the INSERT
+        try {
+            String[] conflict = getPrimaryUserTenantsConflictForAddTenant(sqlCon, primaryUserTenantsTable, tenantIdentifier, supertokensUserId);


porcellus · 2026-01-06T21:32:18Z

+            String recipeUserTenantsTable = getConfig(start).getRecipeUserTenantsTable();
+
+            // 1. Remove account info that is not contributed by any other linked user.
+            String QUERY_1 = "DELETE FROM " + primaryUserTenantsTable + " p"


We should discuss this as it is a bit confusing, plus ideally I'd like to avoid dependencies on appIdToUserId

Dependency on appIdToUserId is removed

porcellus · 2026-01-14T12:32:17Z

+                    + " (app_id, tenant_id, account_info_type, account_info_value, primary_user_id)"
+                    + " SELECT r.app_id, r.tenant_id, r.account_info_type, r.account_info_value, ?"
+                    + " FROM " + recipeUserTenantsTable + " r"
+                    + " INNER JOIN " + recipeUserAccountInfosTable + " ai"


I think we can make this go away, lets evaluate this based on the perf test numbers.

porcellus · 2026-01-14T13:04:15Z

+                    + "         SELECT DISTINCT account_info_type, account_info_value"
+                    + "         FROM " + recipeUserTenantsTable
+                    + "         WHERE app_id = ? AND recipe_user_id IN ("
+                    + "             SELECT recipe_user_id"
+                    + "             FROM " + recipeUserAccountInfosTable
+                    + "             WHERE app_id = ? AND primary_user_id IN ("
+                    + "                 SELECT primary_user_id FROM " + recipeUserAccountInfosTable
+                    + "                 WHERE app_id = ? AND recipe_user_id = ? LIMIT 1"
+                    + "             ) AND recipe_user_id <> ?"
+                    + "         )"


Suggested change

+ " SELECT DISTINCT account_info_type, account_info_value"

+ " FROM " + recipeUserTenantsTable

+ " WHERE app_id = ? AND recipe_user_id IN ("

+ " SELECT recipe_user_id"

+ " FROM " + recipeUserAccountInfosTable

+ " WHERE app_id = ? AND primary_user_id IN ("

+ " SELECT primary_user_id FROM " + recipeUserAccountInfosTable

+ " WHERE app_id = ? AND recipe_user_id = ? LIMIT 1"

+ " ) AND recipe_user_id <> ?"

+ " )"

+ " SELECT account_info_type, account_info_value"

+ " FROM " + recipeUserAccountInfosTable

+ " WHERE app_id = ? AND primary_user_id IN ("

+ " SELECT primary_user_id FROM " + recipeUserAccountInfosTable

+ " WHERE app_id = ? AND recipe_user_id = ? LIMIT 1"

+ " ) AND recipe_user_id <> ?"

porcellus · 2026-01-14T13:20:36Z

+                            + " AND account_info_value IN ("
+                            + "     SELECT account_info_value"
+                            + "     FROM " + recipeUserAccountInfosTable
+                            + "     WHERE app_id = ? AND recipe_user_id = ? AND account_info_type = ?"
+                            + " ) AND account_info_value != ?";


Suggested change

+ " AND account_info_value IN ("

+ " SELECT account_info_value"

+ " FROM " + recipeUserAccountInfosTable

+ " WHERE app_id = ? AND recipe_user_id = ? AND account_info_type = ?"

+ " ) AND account_info_value != ?";

+ " AND account_info_value != ?";

porcellus · 2026-01-14T13:24:29Z

+                + "CONSTRAINT " + Utils.getConstraintName(schema, tableName, "account_info_value", "key")
+                + " UNIQUE (app_id, recipe_id, recipe_user_id, account_info_type, third_party_id, third_party_user_id, account_info_value),"


Suggested change

+ "CONSTRAINT " + Utils.getConstraintName(schema, tableName, "account_info_value", "key")

+ " UNIQUE (app_id, recipe_id, recipe_user_id, account_info_type, third_party_id, third_party_user_id, account_info_value),"

porcellus · 2026-01-14T13:28:13Z

+                        + "   AND NOT EXISTS ("
+                        + "     SELECT 1 FROM " + primaryUserTenantsTable + " p"
+                        + "     WHERE p.app_id = r.app_id"
+                        + "       AND p.tenant_id = r.tenant_id"
+                        + "       AND p.account_info_type = r.account_info_type"
+                        + "       AND p.account_info_value = r.account_info_value"
+                        + "       AND p.primary_user_id = ?"
+                        + "   )";


Let's evaluate if we want to change this to match the do update to old value + app level check approach

porcellus · 2026-01-14T13:34:48Z

+                    // indexes
+                    // TODO


porcellus · 2026-01-15T21:58:22Z


                            } else if (isForeignKeyConstraintError(serverMessage, config.getUsersTable(), "tenant_id")) {
-                                throw new TenantOrAppNotFoundException(usersToImport.get(position).tenantIdentifier);
+                                throw new TenantOrAppNotFoundException(usersToImport.get(position).appIdentifier.getAsPublicTenantIdentifier()); // TODO get proper tenant id


porcellus · 2026-01-15T23:17:04Z

+        }
+    }
+
+    public static CanLinkAccountsResult checkIfLoginMethodsCanBeLinked_Transaction(Start start,


This is starting its own transaction, not matching naming conventions.

porcellus · 2026-01-15T23:26:06Z

+                + "primary_user_id CHAR(36) NOT NULL,"
+                + "CONSTRAINT " + Utils.getConstraintName(schema, tableName, null, "pkey")
+                + " PRIMARY KEY (app_id, tenant_id, account_info_type, account_info_value),"
+                + "CONSTRAINT " + Utils.getConstraintName(schema, tableName, "app_id", "fkey")


I think we need the foreign key to include the tenant_id

tamassoltesz · 2026-01-20T14:11:37Z

+        }
+    }
+
+    public static boolean addPrimaryUserAccountInfo_Transaction(Start start, Connection sqlCon, AppIdentifier appIdentifier, String userId) throws


This method is huge. Could you break into smaller ones?

tamassoltesz · 2026-01-20T14:12:27Z

+        }
+    }
+
+    public static CanBecomePrimaryResult checkIfLoginMethodCanBecomePrimary(Start start, AppIdentifier appIdentifier, String recipeUserId)


This method is rather long. Could you break it into smaller ones?

tamassoltesz · 2026-01-20T14:33:27Z

+
+    }
+
+    public static boolean reserveAccountInfoForLinking_Transaction(Start start, Connection sqlCon, AppIdentifier appIdentifier,


Huge method. Those steps X comments would be excellent points for breaking this into smaller ones.

tamassoltesz · 2026-01-20T14:39:40Z

+        }
+    }
+
+    public static void updateAccountInfo_Transaction(Start start, Connection sqlCon, AppIdentifier appIdentifier, String userId, ACCOUNT_INFO_TYPE accountInfoType, String accountInfoValue)


this method is rather long. Please break it down.

…tion-table

sattvikc added 8 commits November 27, 2025 12:59

fix: change to read committed

e5dcad6

fix: new tables for reservation

0125d22

fix: signup

3234b87

fix: refactor account info queries

ad7d6b2

fix: refactor account info queries

0f48674

fix: can and create primary

2930e09

fix: link accounts

98582a7

fix: associate tenant

854a1a3

porcellus requested changes Dec 15, 2025

View reviewed changes

sattvikc added 12 commits December 18, 2025 17:09

fix: tenant disassociation

b6b5b2c

fix: delete user and bug fixes

27f3ded

fix: tp fix

4c211e0

fix: fkey

768ecb1

fix: unlink accounts and bug fixes

c33b8d5

fix: email change

df5c584

fix: bulk import

0d4cd8d

fix: compile

e2868b1

fix: index

ae89e02

fix: using results for can make primary and can link

854414b

fix: bulk primary and link accounts

5e69e02

fix: update plugin version

a050582

porcellus requested changes Jan 6, 2026

View reviewed changes

sattvikc added 4 commits January 7, 2026 19:01

fix: review comments

2f3490f

fix: refactor create primary

cc411fb

fix: account info table

a135aac

fix: bulk import impl

bae0bc6

porcellus requested changes Jan 14, 2026

View reviewed changes

porcellus requested changes Jan 15, 2026

View reviewed changes

porcellus reviewed Jan 15, 2026

View reviewed changes

fix: review comments

77ec671

sattvikc added 3 commits January 19, 2026 16:36

fix: bulk import error handling

5edff93

fix: cleanup

4b2a0c6

fix: cleanup

0675cd7

tamassoltesz requested changes Jan 20, 2026

View reviewed changes

sattvikc and others added 3 commits January 21, 2026 15:43

fix: deadlock tests

e86ed96

fix: deadlock tests

e9019bc

Merge remote-tracking branch 'origin/9.4' into feat/isolation-reserva…

21b3043

…tion-table

		GeneralQueries.linkAccounts_Transaction(this, sqlCon, appIdentifier, recipeUserId, primaryUserId);
		AccountInfoQueries.reserveAccountInfoForLinking_Transaction(this, sqlCon, appIdentifier, recipeUserId, primaryUserId);

		+ "CONSTRAINT " + Utils.getConstraintName(schema, tableName, "account_info_value", "key")
		+ " UNIQUE (app_id, recipe_id, recipe_user_id, account_info_type, third_party_id, third_party_user_id, account_info_value),"


		}

		public static boolean reserveAccountInfoForLinking_Transaction(Start start, Connection sqlCon, AppIdentifier appIdentifier,

Conversation

sattvikc commented Nov 27, 2025

Summary of change

Related issues

Test Plan

Documentation changes

Checklist for important updates

Remaining TODOs for this PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sattvikc Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sattvikc Dec 31, 2025 •

edited

Loading