Re: Added schema level support for publication.

On Thu, Sep 23, 2021 at 1:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Sep 22, 2021 at 8:52 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Sep 22, 2021 at 3:02 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > ---
> > This patch introduces some new static functions to publicationcmds.c
> > but some have function prototypes but some don't. As far as I checked,
> >
> > ObjectsInPublicationToOids()
> > CheckObjSchemaNotAlreadyInPublication()
> > GetAlterPublicationDelRelations()
> > AlterPublicationSchemas()
> > CheckPublicationAlterTables()
> > CheckPublicationAlterSchemas()
> > LockSchemaList()
> > OpenReliIdList()
> > PublicationAddSchemas()
> > PublicationDropSchemas()
> >
> > are newly introduced but only four functions:
> >
> > OpenReliIdList()
> > LockSchemaList()
> > PublicationAddSchemas()
> > PublicationDropSchemas()
> >
> > have function prototypes. Actually, there already are functions that
> > don't have their prototype in publicationcmds.c. But it seems better
> > to clarify what kind of functions don't need to have a prototype at
> > least in this file.
> >
>
> I think if the function is defined after its use then we declare it at
> the top. Do you prefer to declare all static functions to allow ease
> of usage? Do you have something else in mind?

I prefer to declare all static functions since if we have a function
prototype we don't need to include the change about the function in
future (unrelated) commits that might add a new function which uses
the function and is defined before their declarations. But it seems to
me that the policy varies per file. For instance, all functions in
vacuumlazy.c have their function prototype but functions in
publicationcmds.c seems not. I'm not going to insist on that so please
ignore this comment.

>
> >
> > ---
> > + if ((action == DEFELEM_ADD || action == DEFELEM_SET) && !superuser())
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> > + errmsg("must be superuser to add or
> > set schemas")));
> >
> > Why do we require the superuser privilege only for ADD and SET but not for DROP?
> >
>
> For Add/Set of for all tables of Schema is similar to all tables
> publication requirement. For Drop, I don't think it is mandatory to
> allow only to superuser. The same applies to Alter Publication ...
> Drop table case where you don't need to be table owner whereas, for
> Add, you need to be. We had a discussion on these points in this
> thread. See [1] and some emails prior to it.

Thank you for sharing the link. That makes sense.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/