Re: Column Filtering in Logical Replication

On Mon, Sep 27, 2021 at 4:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Sep 25, 2021 at 1:15 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Fri, Sep 24, 2021 at 6:55 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > > On 2021-Sep-23, Amit Kapila wrote:
> > >
> > > > Alvaro, do you have any thoughts on these proposed grammar changes?
> > >
> > > Yeah, I think pubobj_name remains a problem in that you don't know its
> > > return type -- could be a String or a RangeVar, and the user of that
> > > production can't distinguish.  So you're still (unnecessarily, IMV)
> > > stashing an object of undetermined type into ->object.
> > >
> > > I think you should get rid of both pubobj_name and pubobj_expr and do
> > > somethine like this:
> > >
> > > /* FOR TABLE and FOR ALL TABLES IN SCHEMA specifications */
> > > PublicationObjSpec:     TABLE ColId
> > >                                         {
> > >                                                 $$ = makeNode(PublicationObjSpec);
> > >                                                 $$->pubobjtype = PUBLICATIONOBJ_TABLE;
> > >                                                 $$->rangevar = makeRangeVarFromQualifiedName($1, NULL, @1, yyscanner);
> > >                                         }
> > >                         | TABLE ColId indirection
> > >                                         {
> > >                                                 $$ = makeNode(PublicationObjSpec);
> > >                                                 $$->pubobjtype = PUBLICATIONOBJ_TABLE;
> > >                                                 $$->rangevar = makeRangeVarFromQualifiedName($1, $2, @1, yyscanner);
> > >                                         }
> > >                         | ALL TABLES IN_P SCHEMA ColId
> > >                                         {
> > >                                                 $$ = makeNode(PublicationObjSpec);
> > >                                                 $$->pubobjtype = PUBLICATIONOBJ_REL_IN_SCHEMA;
> > >                                                 $$->name = $4;
> > >                                         }
> > >                         | ALL TABLES IN_P SCHEMA CURRENT_SCHEMA /* XXX should this be "IN_P CURRENT_SCHEMA"? */
> > >                                         {
> > >                                                 $$ = makeNode(PublicationObjSpec);
> > >                                                 $$->pubobjtype = PUBLICATIONOBJ_CURRSCHEMA;
> > >                                                 $$->name = $4;
> > >                                         }
> > >                         | ColId
> > >                                         {
> > >                                                 $$ = makeNode(PublicationObjSpec);
> > >                                                 $$->name = $1;
> > >                                                 $$->pubobjtype = PUBLICATIONOBJ_CONTINUATION;
> > >                                         }
> > >                         | ColId indirection
> > >                                         {
> > >                                                 $$ = makeNode(PublicationObjSpec);
> > >                                                 $$->rangevar = makeRangeVarFromQualifiedName($1, $2, @1, yyscanner);
> > >                                                 $$->pubobjtype = PUBLICATIONOBJ_CONTINUATION;
> > >                                         }
> > >                         | CURRENT_SCHEMA
> > >                                         {
> > >                                                 $$ = makeNode(PublicationObjSpec);
> > >                                                 $$->pubobjtype = PUBLICATIONOBJ_CURRSCHEMA;
> > >                                         }
> > >                 ;
> >
> > Apart from the issue that Hou San pointed, I found one issue with
> > introduction of PUBLICATIONOBJ_CURRSCHEMA, I was not able to
> > differentiate if it is table or schema in the following cases:
> > CREATE PUBLICATION pub1 FOR ALL TABLES IN SCHEMA CURRENT_SCHEMA;
> > CREATE PUBLICATION pub1 FOR ALL TABLES IN SCHEMA sch1, CURRENT_SCHEMA;
> > CREATE PUBLICATION pub1 FOR table t1, CURRENT_SCHEMA;
> > The differentiation is required to differentiate and add a schema or a table.
> >
>
> I am not sure what makes you say that we can't distinguish the above
> cases when there is already a separate rule for CURRENT_SCHEMA? I
> think you can distinguish by tracking the previous objects as we are
> already doing in the patch. But one thing that is not clear to me is
> is the reason to introduce a new type PUBLICATIONOBJ_CURRSCHEMA when
> we use PUBLICATIONOBJ_REL_IN_SCHEMA and PUBLICATIONOBJ_CONTINUATION to
> distinguish all cases of CURRENT_SCHEMA. Alvaro might have something
> in mind for this which is not apparent and that might have caused
> confusion to you as well?

It is difficult to identify this case:
1) create publication pub1 for all tables in schema CURRENT_SCHEMA;
2) create publication pub1 for CURRENT_SCHEMA;

Here case 1 should succeed and case 2 should throw error:
Since the object type will be set to PUBLICATIONOBJ_CURRSCHEMA in both
cases, we cannot differentiate between them:
1) ALL TABLES IN_P SCHEMA CURRENT_SCHEMA /* XXX should this be "IN_P
CURRENT_SCHEMA"? */
                                        {
                                                $$ =
makeNode(PublicationObjSpec);
                                                $$->pubobjtype =
PUBLICATIONOBJ_CURRSCHEMA;
                                                $$->name = $4;
                                        }
2) CURRENT_SCHEMA
                                        {
                                                $$ =
makeNode(PublicationObjSpec);
                                                $$->pubobjtype =
PUBLICATIONOBJ_CURRSCHEMA;
                                        }

I felt it will work, if we set object type to
PUBLICATIONOBJ_CONTINUATION in 2nd case(CURRENT_SCHEMA) and setting
object type to PUBLICATIONOBJ_REL_IN_SCHEMA or
PUBLICATIONOBJ_CURRSCHEMA in 1st case( ALL TABLES IN_P SCHEMA
CURRENT_SCHEMA).
Thoughts?

Regards,
Vignesh