Re: Support EXCEPT for ALL SEQUENCES publications - Mailing list pgsql-hackers

Hi Shlok.

I took a look at v26*. The last review I did was v24, so I refer to
that one several times in this post.

//////////
PATCH v26-0001
//////////

======
src/sgml/catalogs.sgml

1.
The changes that were previously here in v24-0001 to say "tables or
sequences" instead of just "tables" are now missing.

Why? I suspect it is an accidental omission.

======
2.
Docs fail to build.

When I try to build the docs, I get the following error:
postgres.sgml:6606: element link: validity error : IDREF attribute
linkend references an unknown ID
"sql-createpublication-params-for-except-table"

~

AFAICT, that's the same as the catalogs.sgml problem above, because in
v24-0001 that reference name was changed in catalogs.sgml. But now
(since catalogs.sgml was omitted (accidentally?) from the patch it's
using the old reference name, which is now gone.

======
src/backend/catalog/pg_publication.c

check_publication_add_relation:

3.
  * error if not.
  */
 static void
-check_publication_add_relation(PublicationRelInfo *pri)
+check_publication_add_relation(PublicationRelInfo *pri, char pubrelkind)

Previous patch v24 described the new `pubrelkind` parameter, but now
that description is missing. Why was it removed?

======
src/test/regress/expected/publication.out:

4.
+-- fail - EXCEPT (SEQUENCE) clause specifies a temporary sequence
+CREATE TEMPORARY SEQUENCE regress_seq_temp;
+CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
(SEQUENCE regress_seq_temp);
+ERROR:  cannot specify "pg_temp.regress_seq_temp" in the publication
EXCEPT (SEQUENCE) clause
+DETAIL:  This operation is not supported for temporary sequences.

The test for temporary sequences used to exist in v24-0001 but it
seems to have been removed.

Even if you think some extra tests are excessive, IMO it's probably
better to keep them for now and remove them much later just prior to
the patch being pushed.

~~~

5.
+-- fail - EXCEPT (TABLE) clause specifies a sequence object
+CREATE PUBLICATION regress_pub_should_fail FOR ALL TABLES EXCEPT
(TABLE regress_seq0);
+ERROR:  cannot specify relation "public.regress_seq0" in the
publication EXCEPT clause
+DETAIL:  This operation is not supported for sequences.
+-- fail - EXCEPT (SEQUENCE) clause specifies a table object
+CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
(SEQUENCE regress_tab1);
+ERROR:  cannot specify sequence "public.regress_tab1" in the
publication EXCEPT clause
+DETAIL:  This operation is not supported for tables.

Those errors are misleading, and the ERROR/DETAIL are contradictory, because:
regress_seq0 is NOT a relation.
regress_tab1 is NOT a sequence.

Previously (in v24-0001), those error messages were worded in such a
way (below) that this was not a problem:
ERROR:  cannot specify "public.regress_seq0" in the publication EXCEPT
(TABLE) clause
ERROR:  cannot specify "public.regress_tab1" in the publication EXCEPT
(SEQUENCE) clause

//////////
PATCH v26-0002
//////////

======
src/test/regress/expected/publication.out:

1.
 -- fail - EXCEPT (SEQUENCE) clause specifies a temporary sequence
 CREATE TEMPORARY SEQUENCE regress_seq_temp;
 CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
(SEQUENCE regress_seq_temp);
 ERROR:  cannot specify "pg_temp.regress_seq_temp" in the publication
EXCEPT (SEQUENCE) clause
 DETAIL:  This operation is not supported for temporary sequences.
+ALTER PUBLICATION regress_pub_forallsequences_except SET ALL
SEQUENCES EXCEPT (SEQUENCE regress_seq_temp);
+ERROR:  cannot specify "pg_temp.regress_seq_temp" in the publication
EXCEPT (SEQUENCE) clause
+DETAIL:  This operation is not supported for temporary sequences.

The above test used to exist in v24-0002, but has been removed in
v26-0002. IMO testing the exclusion of temporary sequences should
remain for now (same as review comment #4 for patch 0001).

~~~

2.
 -- fail - EXCEPT (TABLE) clause specifies a sequence object
 CREATE PUBLICATION regress_pub_should_fail FOR ALL TABLES EXCEPT
(TABLE regress_seq0);
 ERROR:  cannot specify relation "public.regress_seq0" in the
publication EXCEPT clause
 DETAIL:  This operation is not supported for sequences.
+ALTER PUBLICATION regress_pub_forallsequences_except SET ALL TABLES
EXCEPT (TABLE regress_seq0);
+ERROR:  cannot specify relation "public.regress_seq0" in the
publication EXCEPT clause
+DETAIL:  This operation is not supported for sequences.
 -- fail - EXCEPT (SEQUENCE) clause specifies a table object
 CREATE PUBLICATION regress_pub_should_fail FOR ALL SEQUENCES EXCEPT
(SEQUENCE regress_tab1);
 ERROR:  cannot specify sequence "public.regress_tab1" in the
publication EXCEPT clause
 DETAIL:  This operation is not supported for tables.
+ALTER PUBLICATION regress_pub_forallsequences_except SET ALL
SEQUENCES EXCEPT (SEQUENCE regress_tab1);
+ERROR:  cannot specify sequence "public.regress_tab1" in the
publication EXCEPT clause
+DETAIL:  This operation is not supported for tables.

Same review comment as #6 above. These messages are confusing, but in
v24 they were not.

======
Kind Regards,
Peter Smith.
Fujitsu Australia