On 3/24/22 17:33, Peter Eisentraut wrote:
>
> On 17.03.22 20:11, Tomas Vondra wrote:
>> But the comment describes the error for the whole block, which looks
>> like this:
>>
>> -- error: replica identity "a" not included in the column list
>> ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (b, c);
>> UPDATE testpub_tbl5 SET a = 1;
>> ERROR: cannot update table "testpub_tbl5"
>> DETAIL: Column list used by the publication does not cover the replica
>> identity.
>>
>> So IMHO the comment is correct.
>
> Ok, that makes sense. I read all the comments in the test file again.
> There were a couple that I think could use tweaking; see attached file.
> The ones with "???" didn't make sense to me: The first one is before a
> command that doesn't seem to change anything, the second one I didn't
> understand the meaning. Please take a look.
>
Thanks, the proposed changes seem reasonable. As for the two unclear
tests/comments:
-- make sure changing the column list is updated in SET TABLE (???)
CREATE TABLE testpub_tbl7 (a int primary key, b text, c text);
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl7 (a, b);
\d+ testpub_tbl7
-- ok: we'll skip this table (???)
ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl7 (a, b);
\d+ testpub_tbl7
-- ok: update the column list
ALTER PUBLICATION testpub_fortable SET TABLE testpub_tbl7 (a, c);
\d+ testpub_tbl7
The goal of this test is to verify that we handle column lists correctly
in SET TABLE. That is, if the column list matches the currently set one,
we should just skip the table in SET TABLE. If it's different, we need
to update the catalog. That's what the first comment is trying to say.
It's true we can't really check we skip the table in the SetObject code,
but we can at least ensure there's no error and the column list remains
the same.
And we're not replicating any data in regression tests, so it might
happen we discard the new column list, for example. Hence the second
test, which ensures we end up with the modified column list.
Attached is a patch, rebased on top of the sequence decoding stuff I
pushed earlier today, also including the comments rewording, and
renaming the "transform" function.
I'll go over it again and get it pushed soon, unless someone objects.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
