Dear Vignesh,
Thanks for working the project. Here are my comments only for 0001 and 0002.
Sorry if my points have already been discussed, this thread is too huge to
catchup for me :-(.
01.
Do we have to document the function and open to users? Previously it was not.
Another example is pg_get_publication_tables, which is used only by the backend.
02.
```
SELECT last_value, is_called, log_cnt FROM pg_get_sequence_data('test_seq1');
```
I came up with the way that the function returns page_lsn, how do you feel?
```
SELECT last_value, is_called, log_cnt, page_lsn <= pg_current_wal_lsn() as lsn
FROM pg_get_sequence_data('test_seq1');
```
03.
Regarding the tab completion on psql. When I input till "CREATE PUBLICATION pub
FOR ALL TABLES"
and push the tab, the word "WITH" was completed. But I'm afraid it may be too
much because users can input like "FOR ALL TABLES, ALL SEQUENCES". Should we
suggest the word ", ALL SEQUENCES" here or we can ignore?
Same point can be said when "FOR ALL SEQUENCES" was input.
04.
Same as above, "WITH" could be completed when I input till "FOR ALL SEQUENCES".
However, current patch rejects the WITH clause for ALL SEQUENCE publication.
How do you feel? I'm wondering we should stop the suggestion.
05.
is_publishable_class() has a comment atop the function:
```
* Does same checks as check_publication_add_relation() above, but does not
* need relation to be opened and also does not throw errors.
```
But this is not correct becasue check_publication_add_relation() does not allow
sequences. Can you modify the comment accordingly?
06.
```
values[Anum_pg_publication_puballtables - 1] = stmt->for_all_tables;
values[Anum_pg_publication_puballsequences - 1] =
stmt->for_all_sequences;
```
They should be passed as Datum like others.
07.
```
+ /*
+ * indicates that this is special publication which should encompass all
+ * sequences in the database (except for the unlogged and temp ones)
+ */
+ bool puballsequences;
```
Let me my thought here. I initially wondered whether we should synchronize
unlogged sequences, because it is actually possible. However, I found the
article[1]
that primal use-case of the unlogged sequence is to associate with the unlogged
table.
Based on the point, I agree not to include such sequences - they might be a
leaked
sequence.
08.
```
@@ -1902,6 +1952,13 @@ PublicationDropTables(Oid pubid, List *rels, bool
missing_ok)
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("column list must not be
specified in ALTER PUBLICATION ... DROP"));
+ if (RelationGetForm(rel)->relkind == RELKIND_SEQUENCE)
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("relation \"%s\" is not part of
the publication",
+
RelationGetRelationName(rel)),
+
errdetail_relkind_not_supported(RelationGetForm(rel)->relkind));
+
```
Hmm, I feel this is not needed because in the first place sequences cannot be
specified as the target. Other objects like view is not handled here.
09.
```
+ StringInfo pub_type;
+
Form_pg_publication pubform = (Form_pg_publication) GETSTRUCT(tup);
```
I feel this blank is not needed.
10.
```
/*
* 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 *all_tables,
bool
*all_sequences, core_yyscan_t yyscanner)
```
I'm not a native speaker, but I feel object list is "process"'d here, not
"preprocess"'d. Can we rename to process_pub_all_objtype_list?
11.
```
+ /* The WITH clause is not applicable to FOR ALL SEQUENCES publications
*/
+ if (!pubinfo->puballsequences || pubinfo->puballtables)
```
This meant that FOR ALL TABLES/SEQUENCES publication without any options can be
dumped like:
```
CREATE PUBLICATION pub FOR ALL TABLES, ALL SEQUENCES WITH (publish = 'insert,
update, delete, truncate')
```
However, since the WITH clause is set with ALL SEQUENCE, generated script could
raise the WARNING, which may be confusing for users:
```
$ psql -U postgres -f dump.sql
...
psql:dump.sql:24: WARNING: WITH clause parameters do not affect sequence
synchronization
```
One workaround for it is not to output WITH cause for default setting. Thought?
[1]:
https://www.crunchydata.com/blog/postgresql-unlogged-sequences#unlogged-sequences-in-postgres-have-no-performance-gain
Best regards,
Hayato Kuroda
FUJITSU LIMITED
