Here are some review comments for latest patch v20240725-0002
======
doc/src/sgml/ref/create_publication.sgml
nitpick - tweak to the description of the example.
======
src/backend/parser/gram.y
preprocess_pub_all_objtype_list:
nitpick - typo "allbjects_list"
nitpick - reword function header
nitpick - /alltables/all_tables/
nitpick - /allsequences/all_sequences/
nitpick - I think code is safe as-is because makeNode internally does
palloc0, but OTOH adding Assert would be nicer just to remove any
doubts.
======
src/bin/psql/describe.c
1.
+ /* Print any publications */
+ if (pset.sversion >= 180000)
+ {
+ int tuples = 0;
No need to assign value 0 here, because this will be unconditionally
assigned before use anyway.
~~~~
2. describePublications
has_pubviaroot = (pset.sversion >= 130000);
+ has_pubsequence = (pset.sversion >= 18000);
That's a bug! Should be 180000, not 18000.
======
And, please see the attached diffs patch, which implements the
nitpicks mentioned above.
======
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 7dcfe37..783874f 100644
--- a/doc/src/sgml/ref/create_publication.sgml
+++ b/doc/src/sgml/ref/create_publication.sgml
@@ -420,7 +420,7 @@ CREATE PUBLICATION users_filtered FOR TABLE users (user_id,
firstname);
</programlisting></para>
<para>
- Create a publication that synchronizes all the sequences:
+ Create a publication that publishes all sequences for synchronization:
<programlisting>
CREATE PUBLICATION all_sequences FOR ALL SEQUENCES;
</programlisting>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 585f61e..9b3cad1 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -215,9 +215,9 @@ static void processCASbits(int cas_bits, int location,
const char *constrType,
static PartitionStrategy parsePartitionStrategy(char *strategy);
static void preprocess_pubobj_list(List *pubobjspec_list,
core_yyscan_t yyscanner);
-static void preprocess_pub_all_objtype_list(List *allbjects_list,
-
bool *alltables,
-
bool *allsequences,
+static void preprocess_pub_all_objtype_list(List *all_objects_list,
+
bool *all_tables,
+
bool *all_sequences,
core_yyscan_t yyscanner);
static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node
*query);
@@ -19440,39 +19440,42 @@ parsePartitionStrategy(char *strategy)
}
/*
- * Process all_objects_list to check if the options have been specified more
than
- * once and set alltables/allsequences.
+ * Process all_objects_list to set all_tables/all_sequences.
+ * Also, checks if the pub_object_type has been specified more than once.
*/
static void
-preprocess_pub_all_objtype_list(List *all_objects_list, bool *alltables,
- bool
*allsequences, core_yyscan_t yyscanner)
+preprocess_pub_all_objtype_list(List *all_objects_list, bool *all_tables,
+ bool
*all_sequences, core_yyscan_t yyscanner)
{
if (!all_objects_list)
return;
+ Assert(all_tables && *all_tables == false);
+ Assert(all_sequences && *all_sequences == false);
+
foreach_ptr(PublicationAllObjSpec, obj, all_objects_list)
{
if (obj->pubobjtype == PUBLICATION_ALL_TABLES)
{
- if (*alltables)
+ if (*all_tables)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid publication
object list"),
errdetail("TABLES can be
specified only once."),
parser_errposition(obj->location));
- *alltables = true;
+ *all_tables = true;
}
else if (obj->pubobjtype == PUBLICATION_ALL_SEQUENCES)
{
- if (*allsequences)
+ if (*all_sequences)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid publication
object list"),
errdetail("SEQUENCES can be
specified only once."),
parser_errposition(obj->location));
- *allsequences = true;
+ *all_sequences = true;
}
}
}
