Make transaction metadata nullable to support existing databases #841

jnmt · 2023-04-19T01:36:35Z

This PR adds a feature to handle null transaction metadata. To support existing databases, we assume that we ask users to add transaction metadata columns (with null values in records) in existing tables. So, as this PR does, we would like to handle those records with null transaction metadata as a sort of 'deemed' committed state.

jnmt · 2023-04-19T01:40:53Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposer.java

+                  // For preparing an update of a deemed-commit state record, we need to use
+                  // version 0 rather than NULL since we want to distinguish the preparation from
+                  // an initial record insertion.
+                  columns.add(IntColumn.of(Attribute.BEFORE_VERSION, 0));


As commented in the code, we don't use the original NULL value for before_version in prepare records
to distinguish them from prepare records for newly-inserted ones.

Sorry, I don't fully get this.
If we don't do this, i.e., if we just use NULL for before_version, what issue would occur?

Thanks for seeking clarification. Simply put, the record cannot be rollbacked correctly since the recovery process falls in the deletion path for the initial record case here.

scalardb/core/src/main/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposer.java

Lines 67 to 69 in b3cf9c9

if (beforeId.hasNullValue() && beforeVersion.hasNullValue()) {

// no record to rollback, so it should be deleted

mutations.add(composeDelete(base, latest));

Ah, right. Thank you!
I think it would be great if you could make the comment more specific about the case.
(i.e., adding the reason why we want to use 0 instead of null for before_version.)

Sure, I will try to revise it with a more specific reason.

@feeblefakie Fixed in d4566f0. PTAL when you get a chance.

jnmt · 2023-04-19T01:44:19Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposer.java

+                if (key.equals(Attribute.VERSION) && v.getIntValue() == 0) {
+                  columns.add(IntColumn.ofNull(Attribute.VERSION));


For rollback, we need to change 0 -> null as the opposite side of prepare.

I think adding a comment for this would be helpful.

komamitsu · 2023-04-19T08:30:31Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposer.java

-    return !value.getName().startsWith(Attribute.BEFORE_PREFIX)
-        && !isValueInKeys(value, primary, clustering);
+  private boolean isBeforeRequired(Column<?> column, Key primary, Optional<Key> clustering) {
+    return !column.getName().startsWith(Attribute.BEFORE_PREFIX)


This might be a silly question, but what if an original column name starts with before_ prefix like before_update_balance (non-primary or clustering key)? This logic seems to return false, but I think before_before_update_balance column should be taken care of.

Oh, good point. Yeah, it should be taken care of if we allow users to use a column name starting with before_. For new projects, at least, prohibiting such column names would not cause serious problems. For existing databases, some users might be using such column names, but I guess it would be rare, and we could ask users to rename them. So, IMHO, we can prohibit user columns with before_ in both cases for now.

@brfrn169 By the way, I think we don't have any guard for those names in the schema-loader, and I was able to create a before_xx column and faced an issue that its before column was not updated in the prepare phase. Is my understanding correct?

Ah, I can reproduce it with the current code.

ScalarDB SQL CLI

0: scalardb> select * from example_app.account2; +----+----------+-----+ | id | before_x | y | +----+----------+-----+ | 1 | 10 | 100 | +----+----------+-----+ 1 row selected (0.005 seconds) 0: scalardb> update example_app.account2 set before_x = 20, y = 200 where id = 1; 1 row affected (0.04 seconds)

psql

postgres=# select id, before_x, y, tx_state, before_before_x, before_y from example_app.account2; id | before_x | y | tx_state | before_before_x | before_y ----+----------+-----+----------+-----------------+---------- 1 | 20 | 200 | 3 | | 100 (1 row)

@komamitsu Thanks! Yeah, I faced the same issue.

I thought we accepted to create the before_xx column as a normal column, and in this case, its before column should be before_before_xx. But, we might miss something to handle that.

The logic to detect the before column is here:

scalardb/core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitUtils.java

Lines 179 to 204 in 44a7baf

/**

* Returns whether the specified column is a part of the before image columns or not.

*

* @param columnName a column name

* @param tableMetadata a transaction table metadata

* @return whether the specified column is a part of the before image columns or not

*/

public static boolean isBeforeImageColumn(String columnName, TableMetadata tableMetadata) {

if (!tableMetadata.getColumnNames().contains(columnName)

|| tableMetadata.getPartitionKeyNames().contains(columnName)

|| tableMetadata.getClusteringKeyNames().contains(columnName)) {

return false;

}

if (BEFORE_IMAGE_META_COLUMNS.containsKey(columnName)) {

return true;

}

if (columnName.startsWith(Attribute.BEFORE_PREFIX)) {

// if the column name without the "before_" prefix exists, it's a part of the before image

// columns

return tableMetadata

.getColumnNames()

.contains(columnName.substring(Attribute.BEFORE_PREFIX.length()));

}

return false;

}

The basic idea is, if there are both xxx and before_xxx (before prefixed) columns in the table, xxx is an after image column, and before_xxx is a before image column.

Ah, okay. It should be a bug. Let me check. Thanks.

Ah, okay we should use ConsensusCommitUtils.isAfterImageColumn() here:

scalardb/core/src/main/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposer.java

Line 155 in 44a7baf

return !value.getName().startsWith(Attribute.BEFORE_PREFIX)

I thought I had fixed it 😞

Let me fix it in another PR.

Oh, we already have the validation to check if a table has transaction meta columns when creating the table 😅 :

scalardb/core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitUtils.java

Lines 46 to 48 in 44a7baf

checkIsNotTransactionMetaColumn(tableMetadata.getColumnNames());

Set<String> nonPrimaryKeyColumns = getNonPrimaryKeyColumns(tableMetadata);

checkBeforeColumnsDoNotAlreadyExist(nonPrimaryKeyColumns, tableMetadata);

@brfrn169 Thanks for the additional info. But I guess it only checks to prevent from being created both xxx and before_xxx as normal columns. We can still create before_xxx column itself...

@jnmt Yes, the following bug fix allows users to create before_ prefixed columns as normal columns:

#844

Torch3333

LGTM, thank you.
I only left minor refactoring suggestions.

core/src/main/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposer.java

core/src/main/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposer.java

komamitsu

LGTM! 👍

feeblefakie

Overall looking good. Thank you!
Left a few questions, so PTAL when you get a chance.

feeblefakie · 2023-04-20T07:40:27Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposer.java

+                  // For preparing an update of a deemed-commit state record, we need to use
+                  // version 0 rather than NULL since we want to distinguish the preparation from
+                  // an initial record insertion.
+                  columns.add(IntColumn.of(Attribute.BEFORE_VERSION, 0));


Sorry, I don't fully get this.
If we don't do this, i.e., if we just use NULL for before_version, what issue would occur?

core/src/main/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposer.java

jnmt · 2023-04-20T10:20:03Z

@Torch3333 Thank you for the feedback! Fixed in b3cf9c9. PTAL if you have time.

Torch3333

LGTM, thanks!

komamitsu · 2023-04-28T02:42:20Z

@jnmt (cc: @brfrn169) Just noticed this PR doesn't have integration test cases for NULL status record. Do you think we should add some integration tests? I think it would require non-straightforward way to add NULL status records (directly using the native JDBC?) at arrangement phase. So I'm not sure those integration tests should be added in this PR or in another PR, though.

jnmt · 2023-04-28T04:18:59Z

@jnmt (cc: @brfrn169) Just noticed this PR doesn't have integration test cases for NULL status record. Do you think we should add some integration tests? I think it would require non-straightforward way to add NULL status records (directly using the native JDBC?) at arrangement phase. So I'm not sure those integration tests should be added in this PR or in another PR, though.

@komamitsu Thanks for the question. Yes, we should add some integration tests since this is a kind of major change in the protocol behavior. We are also thinking about applying the Jepsen tests for it and going to discuss it at some point in time (next or next next week). But, in any case, the integration tests will be added by another PR.

feeblefakie · 2023-04-28T08:03:02Z

@jnmt I think jepsen tests can be done in another PR, but it would be great if you could add integration tests in this PR. 🙇

jnmt · 2023-04-28T08:10:04Z

@jnmt I think jepsen tests can be done in another PR, but it would be great if you could add integration tests in this PR.

@feeblefakie Sure! I will do it in the same PR if it's better.

brfrn169 · 2023-05-08T01:31:26Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposer.java

+                  // deleted as an initial record in a rollback situation. To avoid this and roll
+                  // back to a record with a NULL version (i.e., regarded as committed) correctly,
+                  // we need to use version 0 rather than NULL for before_version.
+                  columns.add(IntColumn.of(Attribute.BEFORE_VERSION, 0));


@feeblefakie I'm wondering if we could remove the tx_version column in the first place. As discussed before, if the tx_id column is unique, we don't really need the tx_version column. If we can remove it, we don't need this special handling, and we can make the code simpler.

From a backward compatibility perspective, even if we remove the tx_version column, we simply don't use the column on existing tables, so I don't think we will run into any problems. For new tables, we will no longer need to create the tx_version column.

What do you think?

@brfrn169 As discussed, we need to use something before columns, and the before version would be good enough for the current decision. So, I kept using the version here and updated comments for clarification in db51113.

jnmt · 2023-05-09T10:04:12Z

...om/scalar/db/transaction/consensuscommit/ConsensusCommitNullMetadataIntegrationTestBase.java

I added integration tests for null-tx-metadata handling (incl. rollback/roll-forward) referencing ConsensusCommitSpecificIntegrationTestBase, but skipped the following cases.

Tests that do not use existing data (e.g., insert a new record and do something)

Tests for conflict / write skew situations

jnmt · 2023-05-09T10:14:44Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/PrepareMutationComposer.java

+                  // deleted as an initial record in a rollback situation. To avoid this and roll
+                  // back to a record with a NULL version (i.e., regarded as committed) correctly,
+                  // we need to use version 0 rather than NULL for before_version.
+                  columns.add(IntColumn.of(Attribute.BEFORE_VERSION, 0));


@brfrn169 As discussed, we need to use something before columns, and the before version would be good enough for the current decision. So, I kept using the version here and updated comments for clarification in db51113.

jnmt

Sorry for re-reviewing, but I updated a little for a better understanding of the logic and added integration tests based on the feedback in db51113. So, it would be great if you could take a look again when you get a chance.

brfrn169

Left several minor comments but LGTM other than that. Thank you!

brfrn169 · 2023-05-10T08:00:46Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/TransactionResult.java

+  public boolean isDeemedAsCommitted() {
+    return getId() == null;
+  }
+
+  public boolean hasBeforeImage() {
+    // We need to check not only before_id but also before_version to determine if the record has
+    // the before image or not since we set before_version to 0 for the prepared record when
+    // updating the record deemed as the committed state (cf. PrepareMutationComposer).
+    return !getBeforeIdColumn().hasNullValue() || !getBeforeVersionColumn().hasNullValue();
+  }


brfrn169 · 2023-05-10T08:01:05Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/TransactionResult.java

+  public TextColumn getBeforeIdColumn() {
+    return (TextColumn) result.getColumns().get(Attribute.BEFORE_ID);
+  }
+
+  public IntColumn getBeforeVersionColumn() {
+    return (IntColumn) result.getColumns().get(Attribute.BEFORE_VERSION);
+  }


It look like we can make these method private?

Good catch! Fixed in d5ff7af.

brfrn169 · 2023-05-10T08:04:32Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposer.java

+                if (key.equals(Attribute.VERSION) && v.getIntValue() == 0) {
+                  columns.add(IntColumn.ofNull(Attribute.VERSION));


I think adding a comment for this would be helpful.

feeblefakie

Overall looking good to me!
Left one minor comment.

feeblefakie · 2023-05-11T07:42:11Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/TransactionResult.java

  public String getId() {
    return getText(Attribute.ID);
  }

  public TransactionState getState() {
-    return TransactionState.getInstance(getInt(Attribute.STATE));
+    int state = getInt(Attribute.STATE);


Although getInt returns 0 for a null value, should it be slightly more appropriate/readable to use isNull since it actually checks if the value is null?

That's right. Thanks. Fixed in d5ff7af.

feeblefakie

LGTM! Thank you!

feeblefakie

LGTM! Thank you!

komamitsu

LGTM! 👍

komamitsu · 2023-05-11T08:25:37Z

...om/scalar/db/transaction/consensuscommit/ConsensusCommitNullMetadataIntegrationTestBase.java

+  private DistributedStorage storage;
+  private Coordinator coordinator;
+  private RecoveryHandler recovery;
+  private CommitHandler commit;


[trivial] This can be a local variable.

Good catch! Thank you! Fixed on 3dd3072.

komamitsu · 2023-05-11T08:50:16Z

...om/scalar/db/transaction/consensuscommit/ConsensusCommitNullMetadataIntegrationTestBase.java

+                .clusteringKey(Key.ofInt(ACCOUNT_TYPE, j))
+                .value(IntColumn.of(BALANCE, INITIAL_BALANCE))
+                .build();
+        storage.put(put);


I see. We can manipulate underlying database by directly using DistributedStorage...

Torch3333

LGTM, thank you!

jnmt added 3 commits March 24, 2023 17:32

Make transaction metadata nullable as deemed commit state

a01c332

Merge branch 'master' into nullable-tx-metadata

58b9eff

Update tests

af78e28

jnmt added the enhancement New feature or request label Apr 19, 2023

jnmt self-assigned this Apr 19, 2023

jnmt commented Apr 19, 2023

View reviewed changes

jnmt requested review from komamitsu, brfrn169, feeblefakie and Torch3333 April 19, 2023 01:48

komamitsu reviewed Apr 19, 2023

View reviewed changes

jnmt requested a review from komamitsu April 20, 2023 01:27

Torch3333 approved these changes Apr 20, 2023

View reviewed changes

komamitsu reviewed Apr 20, 2023

View reviewed changes

core/src/main/java/com/scalar/db/transaction/consensuscommit/RollbackMutationComposer.java Show resolved Hide resolved

komamitsu approved these changes Apr 20, 2023

View reviewed changes

brfrn169 mentioned this pull request Apr 20, 2023

Allow to create before_ prefixed columns in Consensus Commit #844

Merged

Refactoring based on the feedback

b3cf9c9

feeblefakie reviewed Apr 20, 2023

View reviewed changes

Torch3333 approved these changes Apr 20, 2023

View reviewed changes

Update a comment for clarification

d4566f0

jnmt requested a review from feeblefakie April 21, 2023 01:56

komamitsu mentioned this pull request Apr 21, 2023

DLT-13675 Implement Schema Importer for ScalarDB Analytics with PostgreSQL scalar-labs/scalardb-analytics-postgresql#6

Merged

brfrn169 reviewed May 8, 2023

View reviewed changes

Merge branch 'master' into nullable-tx-metadata

9b65d3b

Update based on feedback and add integration tests

db51113

jnmt commented May 9, 2023

View reviewed changes

jnmt requested review from brfrn169, komamitsu and Torch3333 May 9, 2023 10:17

brfrn169 approved these changes May 10, 2023

View reviewed changes

feeblefakie reviewed May 11, 2023

View reviewed changes

Update based on feedback

d5ff7af

jnmt requested a review from feeblefakie May 11, 2023 08:29

feeblefakie approved these changes May 11, 2023

View reviewed changes

komamitsu approved these changes May 11, 2023

View reviewed changes

Torch3333 approved these changes May 12, 2023

View reviewed changes

Fix local variable

3dd3072

jnmt merged commit c27fac6 into master May 12, 2023
12 checks passed

jnmt deleted the nullable-tx-metadata branch May 12, 2023 04:57

brfrn169 pushed a commit that referenced this pull request Jul 7, 2023

Make transaction metadata nullable to support existing databases (#841)

4aedd0e

	if (beforeId.hasNullValue() && beforeVersion.hasNullValue()) {
	// no record to rollback, so it should be deleted
	mutations.add(composeDelete(base, latest));

		if (key.equals(Attribute.VERSION) && v.getIntValue() == 0) {
		columns.add(IntColumn.ofNull(Attribute.VERSION));

	/**
	* Returns whether the specified column is a part of the before image columns or not.
	*
	* @param columnName a column name
	* @param tableMetadata a transaction table metadata
	* @return whether the specified column is a part of the before image columns or not
	*/
	public static boolean isBeforeImageColumn(String columnName, TableMetadata tableMetadata) {
	if (!tableMetadata.getColumnNames().contains(columnName)
	\|\| tableMetadata.getPartitionKeyNames().contains(columnName)
	\|\| tableMetadata.getClusteringKeyNames().contains(columnName)) {
	return false;
	}

	if (BEFORE_IMAGE_META_COLUMNS.containsKey(columnName)) {
	return true;
	}
	if (columnName.startsWith(Attribute.BEFORE_PREFIX)) {
	// if the column name without the "before_" prefix exists, it's a part of the before image
	// columns
	return tableMetadata
	.getColumnNames()
	.contains(columnName.substring(Attribute.BEFORE_PREFIX.length()));
	}
	return false;
	}

	checkIsNotTransactionMetaColumn(tableMetadata.getColumnNames());
	Set<String> nonPrimaryKeyColumns = getNonPrimaryKeyColumns(tableMetadata);
	checkBeforeColumnsDoNotAlreadyExist(nonPrimaryKeyColumns, tableMetadata);

Make transaction metadata nullable to support existing databases #841

Make transaction metadata nullable to support existing databases #841

Conversation

jnmt commented Apr 19, 2023

Choose a reason for hiding this comment

feeblefakie Apr 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Apr 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Apr 20, 2023 • edited

Choose a reason for hiding this comment

brfrn169 Apr 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 Apr 20, 2023 • edited

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

feeblefakie Apr 20, 2023 • edited

Choose a reason for hiding this comment

jnmt commented Apr 20, 2023

Torch3333 left a comment

Choose a reason for hiding this comment

komamitsu commented Apr 28, 2023

jnmt commented Apr 28, 2023 • edited

feeblefakie commented Apr 28, 2023

jnmt commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnmt left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

feeblefakie May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 left a comment

Choose a reason for hiding this comment

feeblefakie Apr 20, 2023 •

edited

brfrn169 Apr 20, 2023 •

edited

brfrn169 Apr 20, 2023 •

edited

brfrn169 Apr 20, 2023 •

edited

brfrn169 Apr 20, 2023 •

edited

feeblefakie Apr 20, 2023 •

edited

jnmt commented Apr 28, 2023 •

edited

feeblefakie May 11, 2023 •

edited