Support DDLs related to distributed schemas from any node - part 4 / sequences

[onurctirtir]

TL;DR;

• Preserve sequence range, nextval, and initial value behavior when creating distributed-schema tables from workers.
• Preserve sequence range, nextval, and initial value behavior when altering distributed-schema tables from workers.
• Reset local group id when removing a node, not when we stop syncing metadata to it.

Things that we might want to refactor in a follow-up PR:

• Make the rest of the code-base be using SetLocalEnableDDLPropagation() as well.
• Get rid of extra args of AlterSequenceMinMax(), I don't think they're needed at all.
• Sequence-processing related parts in PostprocessAlterTableStmt should be moved into a separate function etc.

---

Long Summary:

In Postgres, a column can be associated with a sequence in three ways:

• Explicit default using nextval('sequence_name')
• Serial pseudo-types (smallserial, serial, bigserial)
• Identity columns (GENERATED ... AS IDENTITY)

Such columns can be used in distributed tables in three scenarios:

• The column existed before the table was distributed
• The column was added after distribution
• The column was altered to use a sequence after distribution

When distributed tables are created or altered on the coordinator, we enforce
the following behavior:

• The coordinator is always allowed to use the full sequence range.
• If the sequence is bigint-based, the allowed sequence range is split across
  workers based on node group ids.
  Otherwise, workers are effectively prevented from using the sequence by
  setting current value to the maximum value.
  See AlterSequenceMinMax().
• Also, for columns using explicit nextval defaults, worker-side defaults are
  rewritten to worker_nextval() to provide a clearer error when invoked on
  workers.
  For serial and identity columns, the default expression cannot be rewritten,
  but the same sequence range restrictions still apply.
• Furthermore, since the sequence originally present on the coordinator, its
  last_value and is_called state are implicitly preserved on the coordinator
  after it's disributed.

And we want to preserve the same behavior when distributed tables are created or
altered from workers, hence the code changes introduced in this PR.
Specifically, when creating a distributed-schema table from a worker:

• We make sure to always use nextval() on the coordinator when the column has a
  default expression that uses nextval(), regardless of the data type.
• We make sure to execute AlterSequenceMinMax() on the local worker node in
  addition to remote workers.
  That way, we can use the appropriate sequence range on the local worker as
  well if it's bigint-based, or we can prevent the local worker from using the
  sequence otherwise, by setting the current value to the maximum value.
• We also sync the sequence's last_value and is_called state to the
  coordinator's sequence.
  That way, we can continue using the full range starting from the current value
  on the coordinator after distribution, as it's the case when creating
  distributed tables from the coordinator.

Finally, the UDFs that we send to other nodes to implement some parts of these
changes now need to know whether it's called on the coordinator or a worker,
namely worker_apply_sequence_command() and
citus_internal.adjust_identity_column_seq_settings(). For these UDFs to properly
check whether it's called on the coordinator or a worker, we always need to have
the node group id to be already assigned.
For coordinator and metadata-synced workers, this is always the case. Note that,
by default, citus_add_node() always syncs metadata to the workers when adding
them into the metadata. However, although not recommended,
user can later execute stop_metadata_sync_to_node() for a worker to stop
metadata syncing to that worker.

In that case, before this PR, we were resetting the node group id to 0 for
workers with metadata sync stopped, where, 0 is assumed to be the coordinator's
node group id almost always in the code-base. So with this PR, we defer
resetting the group id until the worker is actually removed from the metadata.

Ideally, we should not be sending commands to create / alter sequences to the
workers without a metadata because we never use sequences in shard-level queries
and DMLs, but when querying shell tables. However, changing this would require
more work and deferring resetting the node group id until the worker is removed
from the metadata anyways seems like a correct thing to do.

Also, to preserve compatibility in mixed-version clusters, we keep the older
variant of worker_apply_sequence_command(), i.e., the one that takes 2
parameters rather than 4, and worker_adjust_identity_column_seq_ranges(), which
is the ancestor of citus_internal.adjust_identity_column_seq_settings(). And
when creating or altering a distributed table from the coordinator, we keep
sending these older variants to the workers. This is because, the most
significant difference between using the older variants and the newer variants
is that the newer variants also send last_value and is_called state of the
sequence to the node that they're sent to. However, as we don't actually need to
do that when creating or altering a distributed table from the coordinator, and
since using them from the coordinator could cause failures in mixed-version
clusters, e.g., when one of the workers is still on the older version of Citus,
we keep using the older variants when creating or altering a distributed table
from the coordinator.

[codecov]

Codecov Report

❌ Patch coverage is 94.11765% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.89%. Comparing base (4dc11ba) to head (9782fda).
⚠️ Report is 5 commits behind head on feature/ddl-from-any-node-schema-based-sharding.

Additional details and impacted files

@@                                Coverage Diff                                @@
##           feature/ddl-from-any-node-schema-based-sharding    #8486    +/-   ##
=================================================================================
  Coverage                                            88.88%   88.89%            
=================================================================================
  Files                                                  286      286            
  Lines                                                63261    63504   +243     
  Branches                                              7937     7965    +28     
=================================================================================
+ Hits                                                 56230    56451   +221     
- Misses                                                4745     4757    +12     
- Partials                                              2286     2296    +10     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[onurctirtir]

Note to reviewers

[onurctirtir]

this is for mixed-version clusters

[onurctirtir]

this is for mixed-version clusters

[onurctirtir]

See the changes at src/backend/distributed/metadata/metadata_sync.c.
This is moved from stop_metadata_sync_to_node() to here, see the PR description for why.

[onurctirtir]

As described in PR description, we don't remove ancestors / older versions of these UDFs for mixed-version clusters.

[onurctirtir]

now we're not anymore sending LocalGroupIdUpdateCommand() during stop_metadata_sync_to_node(), so removed

[onurctirtir]

All the tests below are also tested by appending the following right after the marked comment lines, in order to make sure sequence ranges / nextval calls / column default expressions would not change in any nodes even if we created / altered all the sequences below from the coordinator - all test output was identical.

\c - - - :master_port

[codeforall]

We are using the tupleDescriptor after closing the relation and later in the loop we execute ALTER Table on the same relation (from ExecuteAndLogUtilityCommand()). So effectively ALTER table could trigger the relcache invalidation and the relcache entry may get rebuilt and the TupleDesc pointer becomes a dangling reference.
My be we could consider retaining the AccessShareLock on the relation to the end of the transaction (relation_close(tableRelation, NoLock))

[onurctirtir]

Fixed, now we're copying the tuple desc and using it later in the loop.

[codeforall]

Do we disallow descending sequence? As in PostgreSQL user can create -ve value sequences like
CREATE SEQUENCE s AS bigint INCREMENT -1 MINVALUE -100 MAXVALUE -1 START -1;
Is this intentional?

[onurctirtir]

seems I wasn't even aware of that Postgres allows consuming negative values from sequences, let me fix that, thank you!

[codeforall]

Typo in function name in comment, Should be SendCommandToRemoteWorkersWithMetadata instead of SendCommandToCoordinator

[codeforall]

Do we want to enable the DDL Propagation state afterwards? Or is this intended?

[onurctirtir]

Normally, I'd also expect us to enable it back later in such code-paths, however, even in the earlier version where we were directly sending commands to all remote nodes, we were not sending commands to enable it back later. That's why I intentionally decided to not send enable commands at the end for the coordinator as well, in order to preserve the behavior.
Previously we were generating same set of commands and sending them to all nodes, but now we're generating commands for the remote workers and the coordinator, and sending them separately.

	List *commandList = list_make1(DISABLE_DDL_PROPAGATION);
        ...
        ...
	TableDDLCommand *tableDDLCommand = NULL;
	foreach_declared_ptr(tableDDLCommand, tableDDLCommands)
	{
		Assert(CitusIsA(tableDDLCommand, TableDDLCommand));
		commandList = lappend(commandList, GetTableDDLCommand(tableDDLCommand));
	}

	const char *command = NULL;
	foreach_declared_ptr(command, commandList)
	{
		SendCommandToRemoteNodesWithMetadata(command);
	}

Although I'm saying these, probably it wouldn't make a difference if we've sent commands to enable it back because we're sending those commands via the citus internal metadata connections and such connection never care about whether DDL propagation is enabled or not.

[codeforall]

To guard against scenario, if the extension is upgraded on the coordinator while workers are still on the old version. Would it make sense to WRAP these new function creation in DISABLE_DDL_PROPAGATION/ENABLE_DDL_PROPAGATION ?

[onurctirtir]

no, we don't really need that

[codeforall]

If an ALTER TABLE statement contains multiple subcommands that each add or alter a column with a nextval() default (e.g., ALTER TABLE t ADD COLUMN a bigint DEFAULT nextval('s1'), ADD COLUMN b bigint DEFAULT nextval('s2')), sequenceOidToAdjustRangesForLocalWorker will only contain the last subcommand's sequence. That means earlier columns' sequences won't have their ranges adjusted.

[onurctirtir]

As we do for most ALTER TABLE subcommands, we already don't allow issuing such with other subcommands:

citus/src/backend/distributed/commands/table.c

Lines 3348 to 3361 in 366fd64

	/*
	* we only allow adding a serial column if it is the only subcommand
	* and it has no constraints
	*/
	if (commandList->length > 1 || column->constraints)
	{
	ereport(ERROR, (errcode(
	ERRCODE_FEATURE_NOT_SUPPORTED),
	errmsg(
	"cannot execute ADD COLUMN commands involving "
	"serial pseudotypes with other subcommands/constraints"),
	errhint(
	"You can issue each subcommand separately")));
	}

citus/src/backend/distributed/commands/table.c

Lines 3427 to 3441 in 366fd64

	/*
	* we only allow adding a column with non_const default
	* if its the only subcommand and has no other constraints
	*/
	if (commandList->length > 1 ||
	columnConstraints->length > 1)
	{
	ereport(ERROR,
	(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
	errmsg(
	"cannot execute ADD COLUMN .. DEFAULT nextval('..')"
	" command with other subcommands/constraints"),
	errhint(
	"You can issue each subcommand separately")));
	}

citus/src/backend/distributed/commands/table.c

Lines 3481 to 3495 in 366fd64

	/*
	* we only allow altering a column's default to non_const expr
	* if its the only subcommand
	*/
	if (commandList->length > 1)
	{
	ereport(ERROR, (errcode(
	ERRCODE_FEATURE_NOT_SUPPORTED),
	errmsg(
	"cannot execute ALTER COLUMN COLUMN .. SET DEFAULT "
	"nextval('..') command with other subcommands"),
	errhint(
	"You can issue each subcommand separately")));
	}
	}

But let me still add some asserts and some comments to make it even clearer.