Fix PANIC error in ALTER TABLE xxx EXPAND PARTITION PREPARE

[SmartKeyerror]

The PANIC issue is bringed in by commit: Expand partition table leaves in parallel., here's the setp to reproduce it:

1. Build gpdb without cassert, such as

./configure --with-perl --with-python --with-libxml --with-gssapi --enable-debug --disable-cassert --prefix=/usr/local/gpdb

2. Run commands:

postgres=# create extension if not exists gp_debug_numsegments;
NOTICE:  extension "gp_debug_numsegments" already exists, skipping
CREATE EXTENSION
postgres=# select gp_debug_set_create_table_default_numsegments(1);
 gp_debug_set_create_table_default_numsegments 
-----------------------------------------------
 1
(1 row)

postgres=# begin;
BEGIN
postgres=# ALTER TABLE partition_test EXPAND PARTITION PREPARE;
ALTER TABLE
postgres=# select * from partition_test;
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.

If we exec ALTER TABLE partition_test EXPAND PARTITION PREPARE in a transaction and have an access to it next, sunch select * from partition_test or select * from gp_distribution_policy, QD or QE will PANIC cause wrong relation->rd_cdbpolicy.

The root cause is that we used GpPolicy data that may become invalid in function ATExecExpandPartitionTablePrepare:

static void
ATExecExpandPartitionTablePrepare(Relation rel)
{
        // ......
	if (GpPolicyIsRandomPartitioned(rel_dist) || rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
	{
		GpPolicyReplace(relid, root_dist);
		rel->rd_cdbpolicy = root_dist;        // here
	}
}

In fact, the ALTER TABLE command will invoke ATExecCmd(), which will call CommandCounterIncrement() to increase command numbers and invalid relation cache entry by CommandEndInvalidationMessages(), which invoke RelationClearRelation() eventually. The function RelationClearRelation() will physically blow away a relation cache entry, or reset it and rebuild it from scratch, also include the GpPolicy of a relation.

And in the inner of GpPolicyReplace(), it'll invalid the relation cache entry by CatalogTupleUpdate by send a message using SI (Share Invalid), which will be processed by ProcessInvalidationMessages in CommandCounterIncrement().

Since the relation in ATExecExpandPartitionTablePrepare is fetched by relation_open(), which could be a cache result in relation cache. If we update the rd_cdbpolicy, it could update the relation cache entry's rd_cdbpolicy to the new one, which not we wanted.

If the relation's GpPolicy is pointer to new policy, RelationClearRelation() will not swap the GpPolicy with a new relation entry, it still pointer to old memory address. And then, the release of memory will cause a null pointer exception.

[kainwen]

LGTM.

Good job!

1. Please create a GitHub issue to record the details
2. reword a bit the commit message and add comments in the code.
3. I think it is a good chance for you to try to find out why with-cassert not (or not easy) to reproduce?

[ashwinstar]

Please create a GitHub issue to record the details

We can though not sure is necessary. We should try to capture most of context in commit message, change set and test associated with it. I understand many times github issue exists or mailing list discussion exist which has lot of context and referring it in code or commit message helps. In this case none exist and just to capture the context opening the issue while fixing seems overwork. Instead in this case can easily capture in commit context itself.

I think it is a good chance for you to try to find out why with-cassert not (or not easy) to reproduce?

Interesting, yes need to think, I fell chances of failure with asserts enabled will be higher as CLOBBER_FREED_MEMORY and RELCACHE_FORCE_RELEASE are defined with asserts enabled.

[SmartKeyerror]

I think this codes are much wordy, but if we extraction this logic to a function, it's a bit weird:

/*
 * Change num segments of given relation, and make the policy between on-disk
 * catalog and on-memory relation cache consistently.
 */
void
GpPolicyReplaceNumSegments(Relation rel, const int new_numsegments)
{
	GpPolicy	 *new_policy;
	MemoryContext oldcontext;

	oldcontext = MemoryContextSwitchTo(GetMemoryChunkContext(rel));

	new_policy = GpPolicyCopy(rel->rd_cdbpolicy);
	new_policy->numsegments = new_numsegments;

	GpPolicyReplace(RelationGetRelid(rel), new_policy);

	/* We should make the policy between on-disk catalog and on-memory relation cache consistently */
	rel->rd_cdbpolicy = new_policy;
	MemoryContextSwitchTo(oldcontext);
}

It is not at the same level as GpPolicyReplace, just an encapsulation for it. So I think it's not appropriate to extraction the repeated code to a single function, which will be used rarely.

[ashwinstar]

Agree its okay to not extract the same in separate function

[ashwinstar]

Overall LGTM, just need to address that last comment

[ashwinstar]

Agree its okay to not extract the same in separate function

[ashwinstar]

👍 Thanks for interactively incorporating the review comments

[kainwen]

LGTM.

When merging this, please reword the commit message a bit. Might consider adding why with-cassert not produce the issue?

Thanks for you contribution!

[SmartKeyerror]

@kainwen Actually, the version build with --enable-cassert also can reproduce this problem, if we use the example I posted in this pr, but it not failed in pipeline.