On Sun, Jun 22, 2025 at 8:05 AM vignesh C <vignes...@gmail.com> wrote:
>
> Thanks for the comment, the attached v20250622 version patch has the
> changes for the same.
>
Thanks for the patches, please find my review comments for patches 001 and 002:
1) patch-001 :pg_sequence_state()
+ /* open and lock sequence */
+ init_sequence(seq_relid, &elm, &seqrel);
/ open/ Open
~~~
patch-0002:
2)
- /* FOR ALL TABLES requires superuser */
- if (stmt->for_all_tables && !superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser to create FOR ALL TABLES publication")));
+ if (!superuser())
I think we can retain the original comment here with required modification :
/* FOR ALL TABLES and FOR ALL SEQUENCES requires superuser */
~~~
3) ALL TABLES vs ALL SEQUENCES
For the command:
CREATE PUBLICATION FOR ALL TABLES, [...]
- only "SEQUENCES" is allowed after ',', and TABLE or TABLES IN
SCHEMA are not allowed. Which aligns with the fact that "all tables"
is inclusive of "FOR TABLE" and "FOR TABLES IN SCHEMA".
Therefore, adding and dropping of tables from a "ALL TABLES"
publication is also not allowed. e.g.,
```
postgres=# alter publication test add table t1;
ERROR: publication "test" is defined as FOR ALL TABLES
DETAIL: Tables cannot be added to or dropped from FOR ALL TABLES publications.
postgres=# alter publication test add tables in schema public;
ERROR: publication "test" is defined as FOR ALL TABLES
DETAIL: Schemas cannot be added to or dropped from FOR ALL TABLES publications.
```
However, for ALL SEQUENCES, the behavior seems inconsistent. CREATE
PUBLICATION doesn’t allow adding TABLE or TABLES IN SCHEMA along with
ALL SEQUENCES, but ALTER PUBLICATION does.
e.g., for a all sequence publication 'pubs', below succeeds :
postgres=# alter publication pubs add table t1;
ALTER PUBLICATION
Is this expected?
If adding tables is allowed using ALTER PUBLICATION, perhaps it should
also be permitted during CREATE PUBLICATION or disallowed in both
cases. Thoughts?
~~~
4) Consider a publication 'pubs' on all sequences and 'n1' is a sequence, now -
postgres=# alter publication pubs drop table n1;
ERROR: relation "n1" is not part of the publication
This error message can be misleading, as the relation n1 is part of
the publication - it's just a sequence, not a table.
It might be more accurate to add a DETAIL line similar to ADD case:
postgres=# alter publication pubs add table n1;
ERROR: cannot add relation "n1" to publication
DETAIL: This operation is not supported for sequences.
~~~
--
Thanks.
Nisha
