Here are my review comments for the patch v20240703-0002
======
doc/src/sgml/ref/create_publication.sgml
nitpick - consider putting the "FOR ALL SEQUENCES" para last, because
eventually when more sequence syntax is added IMO it will be better to
describe all the TABLES together, and then describe all the SEQUENCES
together.
nitpick - /synchronizing changes/synchronizes changes/
Question: Was there a reason you chose wording "synchronizes changes"
instead of having same "replicates changes" wording of FOR ALL TABLES?
======
src/backend/catalog/system_views.sql
1.
Should there be some new test for the view? Otherwise, AFAICT this
patch has no tests that will exercise the new function
pg_get_publication_sequences.
======
src/backend/commands/publicationcmds.c
2.
+ errmsg("must be superuser to create FOR ALL %s publication",
+ stmt->for_all_tables ? "TABLES" : "SEQUENCES")));
nitpick - the combined error message may be fine, but I think
translators will prefer the substitution to be the full "FOR ALL
TABLES" and "FOR ALL SEQUENCES" instead of just the keywords that are
different.
======
src/backend/parser/gram.y
3.
Some of these new things maybe could be named better?
'preprocess_allpubobjtype_list' => 'preprocess_pub_all_objtype_list'
'AllPublicationObjSpec *allpublicationobjectspec;' =>
'PublicationAllObjSpec *publicationallobjectspec;'
(I didn't include these in nitpicks diffs because you probably have
better ideas than I do for good names)
~~~
nitpick - typo in comment /SCHEMAS/SEQUENCES/
preprocess_allpubobjtype_list:
nitpick - typo /allbjects_list/all_objects_list/
nitpick - simplify /allpubob/obj/
nitpick - add underscores in the enums
======
src/bin/pg_dump/pg_dump.c
4.
+ if (pubinfo->puballtables || pubinfo->puballsequences)
+ {
+ appendPQExpBufferStr(query, " FOR ALL");
+ if (pubinfo->puballtables && pubinfo->puballsequences)
+ appendPQExpBufferStr(query, " TABLES, SEQUENCES");
+ else if (pubinfo->puballtables)
+ appendPQExpBufferStr(query, " TABLES");
+ else
+ appendPQExpBufferStr(query, " SEQUENCES");
+ }
nitpick - it seems over-complicated; See nitpicks diff for my suggestion.
======
src/include/nodes/parsenodes.h
nitpick - put underscores in the enum values
~~
5.
- bool for_all_tables; /* Special publication for all tables in db */
+ List *for_all_objects; /* Special publication for all objects in
+ * db */
Is this OK? Saying "for all objects" seemed misleading.
======
src/test/regress/sql/publication.sql
nitpick - some small changes to comments, e.g. writing keywords in uppercase
~~~
6.
I asked this before in a previous review [1-#17] -- I didn't
understand the point of the sequence 'testpub_seq0' since nobody seems
to be doing anything with it. Should it just be removed? Or is there a
missing test case to use it?
~~~
7.
Other things to consider:
(I didn't include these in my attached diff)
* could use a single CREATE SEQUENCE stmt instead of multiple
* could use a single DROP PUBLICATION stmt instead of multiple
* shouldn't all publication names ideally have a 'regress_' prefix?
======
99.
Please refer to the attached nitpicks diff which has implementation
for the nitpicks cited above.
======
[1]
https://www.postgresql.org/message-id/CAHut%2BPvrk75vSDkaXJVmhhZuuqQSY98btWJV%3DBMZAnyTtKRB4g%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_publication.sgml
b/doc/src/sgml/ref/create_publication.sgml
index 0c1b469..9891fef 100644
--- a/doc/src/sgml/ref/create_publication.sgml
+++ b/doc/src/sgml/ref/create_publication.sgml
@@ -122,16 +122,6 @@ CREATE PUBLICATION <replaceable
class="parameter">name</replaceable>
</listitem>
</varlistentry>
- <varlistentry id="sql-createpublication-params-for-all-sequences">
- <term><literal>FOR ALL SEQUENCES</literal></term>
- <listitem>
- <para>
- Marks the publication as one that synchronizing changes for all sequences
- in the database, including sequences created in the future.
- </para>
- </listitem>
- </varlistentry>
-
<varlistentry id="sql-createpublication-params-for-all-tables">
<term><literal>FOR ALL TABLES</literal></term>
<listitem>
@@ -173,6 +163,16 @@ CREATE PUBLICATION <replaceable
class="parameter">name</replaceable>
</listitem>
</varlistentry>
+ <varlistentry id="sql-createpublication-params-for-all-sequences">
+ <term><literal>FOR ALL SEQUENCES</literal></term>
+ <listitem>
+ <para>
+ Marks the publication as one that synchronizes changes for all sequences
+ in the database, including sequences created in the future.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="sql-createpublication-params-with">
<term><literal>WITH ( <replaceable
class="parameter">publication_parameter</replaceable> [= <replaceable
class="parameter">value</replaceable>] [, ... ] )</literal></term>
<listitem>
diff --git a/src/backend/commands/publicationcmds.c
b/src/backend/commands/publicationcmds.c
index 35c7dda..a2ccbf3 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -751,8 +751,9 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt
*stmt)
if ((stmt->for_all_tables || stmt->for_all_sequences) && !superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to create FOR ALL %s
publication",
- stmt->for_all_tables ? "TABLES"
: "SEQUENCES")));
+ errmsg("must be superuser to create a %s
publication",
+ stmt->for_all_tables ? "FOR ALL
TABLES" :
+ "FOR ALL SEQUENCES")));
rel = table_open(PublicationRelationId, RowExclusiveLock);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 382cf5c..760d700 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10565,7 +10565,7 @@ AlterOwnerStmt: ALTER AGGREGATE aggregate_with_argtypes
OWNER TO RoleSpec
* pub_obj_type is one of:
*
* TABLES
- * SCHEMAS
+ * SEQUENCES
*
* CREATE PUBLICATION FOR pub_obj [, ...] [WITH options]
*
@@ -10707,13 +10707,13 @@ AllPublicationObjSpec:
TABLES
{
$$ =
makeNode(AllPublicationObjSpec);
- $$->pubobjtype =
PUBLICATION_ALLTABLES;
+ $$->pubobjtype =
PUBLICATION_ALL_TABLES;
$$->location = @1;
}
| SEQUENCES
{
$$ =
makeNode(AllPublicationObjSpec);
- $$->pubobjtype =
PUBLICATION_ALLSEQUENCES;
+ $$->pubobjtype =
PUBLICATION_ALL_SEQUENCES;
$$->location = @1;
}
;
@@ -19439,41 +19439,41 @@ parsePartitionStrategy(char *strategy)
}
/*
- * Process allbjects_list to check if the options have been specified more than
+ * Process all_objects_list to check if the options have been specified more
than
* once and set alltables/allsequences.
*/
static void
-preprocess_allpubobjtype_list(List *allbjects_list, bool *alltables,
+preprocess_allpubobjtype_list(List *all_objects_list, bool *alltables,
bool *allsequences,
core_yyscan_t yyscanner)
{
bool alltables_specified = false;
bool allsequences_specified = false;
- if (!allbjects_list)
+ if (!all_objects_list)
return;
- foreach_ptr(AllPublicationObjSpec, allpubob, allbjects_list)
+ foreach_ptr(AllPublicationObjSpec, obj, all_objects_list)
{
- if (allpubob->pubobjtype == PUBLICATION_ALLTABLES)
+ if (obj->pubobjtype == PUBLICATION_ALL_TABLES)
{
if (alltables_specified)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid publication
object list"),
errdetail("TABLES can be
specified only once."),
-
parser_errposition(allpubob->location));
+
parser_errposition(obj->location));
alltables_specified = true;
*alltables = true;
}
- else if (allpubob->pubobjtype == PUBLICATION_ALLSEQUENCES)
+ else if (obj->pubobjtype == PUBLICATION_ALL_SEQUENCES)
{
if (allsequences_specified)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid publication
object list"),
errdetail("SEQUENCES can be
specified only once."),
-
parser_errposition(allpubob->location));
+
parser_errposition(obj->location));
allsequences_specified = true;
*allsequences = true;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 0f55a1b..da713c2 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4326,16 +4326,12 @@ dumpPublication(Archive *fout, const PublicationInfo
*pubinfo)
appendPQExpBuffer(query, "CREATE PUBLICATION %s",
qpubname);
- if (pubinfo->puballtables || pubinfo->puballsequences)
- {
- appendPQExpBufferStr(query, " FOR ALL");
- if (pubinfo->puballtables && pubinfo->puballsequences)
- appendPQExpBufferStr(query, " TABLES, SEQUENCES");
- else if (pubinfo->puballtables)
- appendPQExpBufferStr(query, " TABLES");
- else
- appendPQExpBufferStr(query, " SEQUENCES");
- }
+ if (pubinfo->puballtables && pubinfo->puballsequences)
+ appendPQExpBufferStr(query, " FOR ALL TABLES, SEQUENCES");
+ else if (pubinfo->puballtables)
+ appendPQExpBufferStr(query, " FOR ALL TABLES");
+ else if (pubinfo->puballsequences)
+ appendPQExpBufferStr(query, " FOR ALL SEQUENCES");
appendPQExpBufferStr(query, " WITH (publish = '");
if (pubinfo->pubinsert)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 1c50940..50efbe5 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -4167,8 +4167,8 @@ typedef struct PublicationObjSpec
*/
typedef enum AllPublicationObjType
{
- PUBLICATION_ALLTABLES, /* All tables */
- PUBLICATION_ALLSEQUENCES, /* All sequences */
+ PUBLICATION_ALL_TABLES, /* All tables */
+ PUBLICATION_ALL_SEQUENCES, /* All sequences */
} AllPublicationObjType;
typedef struct AllPublicationObjSpec
diff --git a/src/test/regress/expected/publication.out
b/src/test/regress/expected/publication.out
index 579c693..d0200d5 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -228,9 +228,10 @@ Tables:
DROP TABLE testpub_tbl3, testpub_tbl3a;
DROP PUBLICATION testpub3, testpub4;
---- sequences publication
+--- Tests for publications with SEQUENCES
CREATE SEQUENCE testpub_seq0;
CREATE SEQUENCE pub_test.testpub_seq1;
+-- FOR ALL SEQUENCES
SET client_min_messages = 'ERROR';
CREATE PUBLICATION testpub_forallsequences FOR ALL SEQUENCES;
RESET client_min_messages;
@@ -255,7 +256,7 @@ Publications:
regress_publication_user | f | t | t | t | t
| t | f
(1 row)
---- combination of all tables and all sequences publication
+--- FOR ALL specifying both TABLES and SEQUENCES
SET client_min_messages = 'ERROR';
CREATE PUBLICATION testpub_for_allsequences_alltables FOR ALL SEQUENCES,
TABLES;
RESET client_min_messages;
@@ -275,13 +276,13 @@ SELECT pubname, puballtables, puballsequences FROM
pg_publication WHERE pubname
DROP SEQUENCE testpub_seq0, pub_test.testpub_seq1;
DROP PUBLICATION testpub_forallsequences;
DROP PUBLICATION testpub_for_allsequences_alltables;
--- fail - specifying tables more than once;
+-- fail - FOR ALL specifying TABLES more than once
CREATE PUBLICATION testpub_for_allsequences_alltables FOR ALL SEQUENCES,
TABLES, TABLES;
ERROR: invalid publication object list
LINE 1: ...or_allsequences_alltables FOR ALL SEQUENCES, TABLES, TABLES;
^
DETAIL: TABLES can be specified only once.
--- fail - specifying sequences more than once;
+-- fail - FOR ALL specifying SEQUENCES more than once
CREATE PUBLICATION testpub_for_allsequences_alltables FOR ALL SEQUENCES,
TABLES, SEQUENCES;
ERROR: invalid publication object list
LINE 1: ...allsequences_alltables FOR ALL SEQUENCES, TABLES, SEQUENCES;
diff --git a/src/test/regress/sql/publication.sql
b/src/test/regress/sql/publication.sql
index ad0af9d..c28d554 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -117,10 +117,11 @@ RESET client_min_messages;
DROP TABLE testpub_tbl3, testpub_tbl3a;
DROP PUBLICATION testpub3, testpub4;
---- sequences publication
+--- Tests for publications with SEQUENCES
CREATE SEQUENCE testpub_seq0;
CREATE SEQUENCE pub_test.testpub_seq1;
+-- FOR ALL SEQUENCES
SET client_min_messages = 'ERROR';
CREATE PUBLICATION testpub_forallsequences FOR ALL SEQUENCES;
RESET client_min_messages;
@@ -129,7 +130,7 @@ SELECT pubname, puballtables, puballsequences FROM
pg_publication WHERE pubname
\d+ pub_test.testpub_seq1
\dRp+ testpub_forallsequences
---- combination of all tables and all sequences publication
+--- FOR ALL specifying both TABLES and SEQUENCES
SET client_min_messages = 'ERROR';
CREATE PUBLICATION testpub_for_allsequences_alltables FOR ALL SEQUENCES,
TABLES;
RESET client_min_messages;
@@ -141,10 +142,10 @@ DROP SEQUENCE testpub_seq0, pub_test.testpub_seq1;
DROP PUBLICATION testpub_forallsequences;
DROP PUBLICATION testpub_for_allsequences_alltables;
--- fail - specifying tables more than once;
+-- fail - FOR ALL specifying TABLES more than once
CREATE PUBLICATION testpub_for_allsequences_alltables FOR ALL SEQUENCES,
TABLES, TABLES;
--- fail - specifying sequences more than once;
+-- fail - FOR ALL specifying SEQUENCES more than once
CREATE PUBLICATION testpub_for_allsequences_alltables FOR ALL SEQUENCES,
TABLES, SEQUENCES;
-- Tests for partitioned tables
