Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

On Thu, Apr 18, 2024 at 06:17:56PM +0200, Alvaro Herrera wrote:
> On 2024-Apr-18, Michael Paquier wrote:
>> I was also worrying about a need to dump the protocol version to be
>> able to track the relkind in the toc entries, but a45c78e3284b has
>> already done one.  The difference in AM handling between relations
>> without storage and relations with storage pushes the relkind logic
>> more within the internals of pg_backup_archiver.c.
> 
> Hmm, does this mean that every dump taking since a45c78e3284b (April
> 1st) and before this commit will be unrestorable?  This doesn't worry me
> too much, because we aren't even in beta yet ... and I think we don't
> have a strict policy about it.

I've been scanning the history of K_VERS_1_* in the recent years, and
it does not seem that we have a case where we would have needed to
bump the version twice in the same release cycle.  Anyway, yes, any
dump taken since 1_16 has been bumped would fail to restore with this
patch in place.  For an unreleased not-yet-in-beta branch, why should
we care?  Things are not set in stone, like extensions.  If others
have comments about this point, feel free of course.

>> --- a/src/bin/pg_dump/t/002_pg_dump.pl
>> +++ b/src/bin/pg_dump/t/002_pg_dump.pl
>> @@ -4591,11 +4591,9 @@ my %tests = (
>>  			CREATE TABLE dump_test.regress_pg_dump_table_am_child_2
>>  			  PARTITION OF dump_test.regress_pg_dump_table_am_parent FOR VALUES IN (2);',
>>  		regexp => qr/^
>> -			\QSET default_table_access_method = regress_table_am;\E
>> -			(\n(?!SET[^;]+;)[^\n]*)*
>> -			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_parent (\E
>> -			(.*\n)*
>>  			\QSET default_table_access_method = heap;\E
>> +			(.*\n)*
>> +			\QALTER TABLE dump_test.regress_pg_dump_table_am_parent SET ACCESS METHOD regress_table_am;\E
>>  			(\n(?!SET[^;]+;)[^\n]*)*
>>  			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_1 (\E
>>  			(.*\n)*
> 
> This looks strange -- why did you remove matching for the CREATE TABLE
> of the parent table?  That line should appear shortly before the ALTER
> TABLE SET ACCESS METHOD for the same table, shouldn't it?

Yeah, with the ALTER in place that did not seem that mandatory but I
don't mind keeping it, as well.

> Maybe your
> intention was to remove only the SET default_table_access_method
> = regress_table_am line ... but it's not clear to me why we have the
> "SET default_table_access_method = heap" line before the ALTER TABLE SET
> ACCESS METHOD.

This comes from the contents of the dump for
regress_pg_dump_table_am_2, that uses heap as table AM.  A SET is
issued for it before dumping regress_pg_dump_table_am_parent and its
partitions.  One trick that I can think of to make the output parsing
of the test more palatable is to switch the AMs used by the two
partitions, so as we finish with two SET queries before each partition
rather than one before the partitioned table.  See the attached for
the idea.
--
Michael