Re: Added schema level support for publication. - Mailing list pgsql-hackers
| From | vignesh C |
|---|---|
| Subject | Re: Added schema level support for publication. |
| Date | |
| Msg-id | CALDaNm3kiupSCWLFk-E=M7Y7SF0t9q_3NEv=zQP6L-ZkO4Oitw@mail.gmail.com Whole thread Raw |
| In response to | Re: Added schema level support for publication. (Tom Lane <tgl@sss.pgh.pa.us>) |
| Responses |
Re: Added schema level support for publication.
|
| List | pgsql-hackers |
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 something like 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
{ ....}
;
I needed pubobj_expr to support the existing syntaxes supported by relation_expr and also to handle CURRENT_SCHEMA support in case of the "FOR ALL TABLES IN SCHEMA" feature. I changed the name to any_name to support objects like "sch1.t1".
I felt if a user specified "FOR ALL TABLES", the user should not be allowed to combine it with "FOR TABLE" and "FOR ALL TABLES IN SCHEMA" as "FOR ALL TABLES" anyway will include all the tables.
Should we support the similar syntax in case of alter publication, like "ALTER PUBLICATION pub1 ADD TABLE t1,t2, ALL TABLES IN SCHEMA sch1, sch2" or shall we keep these separate like "ALTER PUBLICATION pub1 ADD TABLE t1, t2" and "ALTER PUBLICATION pub1 ADD ALL TABLES IN SCHEMA sch1, sch2". I preferred to keep it separate as we have kept ADD/DROP separately which cannot be combined currently. I have not mentioned SEQUENCE handling separately, the sequence will be extended in similar lines. I thought of throwing an error at post processing, the first option that Tom Lane had suggested if the user specified syntax like: create publication pub1 for t1,t2;
Thoughts?
[1] - https://www.postgresql.org/message-id/877603.1629120678%40sss.pgh.pa.us
>
> 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 something like 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
{ ....}
;
I needed pubobj_expr to support the existing syntaxes supported by relation_expr and also to handle CURRENT_SCHEMA support in case of the "FOR ALL TABLES IN SCHEMA" feature. I changed the name to any_name to support objects like "sch1.t1".
I felt if a user specified "FOR ALL TABLES", the user should not be allowed to combine it with "FOR TABLE" and "FOR ALL TABLES IN SCHEMA" as "FOR ALL TABLES" anyway will include all the tables.
Should we support the similar syntax in case of alter publication, like "ALTER PUBLICATION pub1 ADD TABLE t1,t2, ALL TABLES IN SCHEMA sch1, sch2" or shall we keep these separate like "ALTER PUBLICATION pub1 ADD TABLE t1, t2" and "ALTER PUBLICATION pub1 ADD ALL TABLES IN SCHEMA sch1, sch2". I preferred to keep it separate as we have kept ADD/DROP separately which cannot be combined currently. I have not mentioned SEQUENCE handling separately, the sequence will be extended in similar lines. I thought of throwing an error at post processing, the first option that Tom Lane had suggested if the user specified syntax like: create publication pub1 for t1,t2;
Thoughts?
[1] - https://www.postgresql.org/message-id/877603.1629120678%40sss.pgh.pa.us
Regards,
Vignesh
pgsql-hackers by date: