Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Wed, Aug 25, 2021 at 1:19 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Aug 23, 2021 at 11:16 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Tue, Aug 17, 2021 at 6:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > Amit Kapila <amit.kapila16@gmail.com> writes:
> > > > On Tue, Aug 17, 2021 at 6:40 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > > >> On Mon, Aug 16, 2021 at 11:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > >>> Abstractly it'd be
> > > >>>
> > > >>> createpub := CREATE PUBLICATION pubname FOR cpitem [, ... ] [ WITH ... ]
> > > >>>
> > > >>> cpitem := ALL TABLES |
> > > >>>     TABLE name |
> > > >>>     ALL TABLES IN SCHEMA name |
> > > >>>     ALL SEQUENCES |
> > > >>>     SEQUENCE name |
> > > >>>     ALL SEQUENCES IN SCHEMA name |
> > > >>>     name
> > > >>>
> > > >>> The grammar output would need some post-analysis to attribute the
> > > >>> right type to bare "name" items, but that doesn't seem difficult.
> > >
> > > >> That last bare "name" cpitem. looks like it would permit the following syntax:
> > > >> CREATE PUBLICATION pub FOR a,b,c;
> > > >> Was that intentional?
> > >
> > > > I think so.
> > >
> > > I had supposed that we could throw an error at the post-processing stage,
> > > or alternatively default to assuming that such names are tables.
> > >
> > > Now you could instead make the grammar work like
> > >
> > > cpitem := ALL TABLES |
> > >           TABLE name [, ...] |
> > >           ALL TABLES IN SCHEMA name [, ...] |
> > >           ALL SEQUENCES |
> > >           SEQUENCE name [, ...] |
> > >           ALL SEQUENCES IN SCHEMA name [, ...]
> > >
> > > which would result in a two-level-list data structure.  I'm not sure
> > > that this is better, as any sort of mistake would result in a very
> > > uninformative generic "syntax error" from Bison.  Errors out of a
> > > post-processing stage could be more specific than that.
> >
> > I preferred the implementation in the lines Tom Lane had proposed at [1]. Is it ok if the implementation is
somethinglike below: 
> > CreatePublicationStmt:
> > CREATE PUBLICATION name FOR pub_obj_list opt_definition
> > {
> > CreatePublicationStmt *n = makeNode(CreatePublicationStmt);
> > n->pubname = $3;
> > n->options = $6;
> > n->pubobjects = (List *)$5;
> > $$ = (Node *)n;
> > }
> > ;
> > pub_obj_list: PublicationObjSpec
> > { $$ = list_make1($1); }
> > | pub_obj_list ',' PublicationObjSpec
> > { $$ = lappend($1, $3); }
> > ;
> > /* FOR TABLE and FOR ALL TABLES IN SCHEMA specifications */
> > PublicationObjSpec: TABLE pubobj_expr
> > { ....}
> > | ALL TABLES IN_P SCHEMA pubobj_expr
> > { ....}
> > | pubobj_expr
> > { ....}
> > ;
> > pubobj_expr:
> > any_name
> > { ....}
> > | any_name '*'
> > { ....}
> > | ONLY any_name
> > { ....}
> > | ONLY '(' any_name ')'
> > { ....}
> > | CURRENT_SCHEMA
> > { ....}
> > ;
>
> "FOR ALL TABLES” (that includes all tables in the database) is missing
> in this syntax?

"FOR ALL TABLES" is present in CreatePublicationStmt rule, sorry for
not including all of CreatePublicationStmt rule in the previous mail,
I thought of keeping the contents shorter:

CreatePublicationStmt:
CREATE PUBLICATION name opt_definition
{....}
| CREATE PUBLICATION name FOR ALL TABLES opt_definition
{....}
| CREATE PUBLICATION name FOR pub_obj_list opt_definition
{....}
;

It is not in  pub_obj_list as the user will be able to specify either
of "FOR ALL TABLES" or "FOR TABLE/ FOR ALL TABLES IN SCHEMA" along
with create publication.

> >
> > I needed pubobj_expr to support the existing syntaxes supported by relation_expr and also to handle CURRENT_SCHEMA
supportin case of the "FOR ALL TABLES IN SCHEMA" feature. I changed the name to any_name to support objects like
"sch1.t1".
>
> I think that relation_expr also accepts objects like "sch1.t1", no?

Earlier syntax only supported relations, the relations were parsed
into RangeVar datatype. The new feature supports schema for which only
the schema name is required. To keep the parsing rule common, I used
any_name which will store the dotted name into a list for both
relation and schema. I will later convert it into rangevar for
relation and schema oid for schema names during the processing and
create the publication. I felt relation_expr was able to handle dotted
names because of qualified_name having "ColId indirection", here
indirection rule takes care of handling the dotted names.

> > I felt if a user specified "FOR ALL TABLES", the user should not be allowed to combine it with "FOR TABLE" and "FOR
ALLTABLES IN SCHEMA" as "FOR ALL TABLES" anyway will include all the tables. 
>
> I think so too.
>
> > Should we support the similar syntax in case of alter publication, like "ALTER PUBLICATION pub1 ADD TABLE t1,t2,
ALLTABLES IN SCHEMA sch1, sch2" or shall we keep these separate like "ALTER PUBLICATION pub1 ADD TABLE t1, t2"  and
"ALTERPUBLICATION pub1 ADD ALL TABLES IN SCHEMA sch1, sch2". I preferred to keep it separate as we have kept ADD/DROP
separatelywhich cannot be combined currently. 
>
> If we support the former syntax, the latter two syntaxes are also
> supported. Why do we want to support only the latter separate two
> syntaxes?

We can support either syntax, I was not sure which one is better. If
alter also should support similar syntax I can do it as a separate
patch so as to not increase the main patch size. Thoughts?

Attached v21 patch has the changes based on the new syntax and fixes
few of the other review comments provided by reviewers.

Regards,
Vignesh