Re: Synchronizing slots from primary to standby - Mailing list pgsql-hackers

Hi,

On Thu, Feb 22, 2024 at 04:01:34PM +0530, shveta malik wrote:
> On Thu, Feb 22, 2024 at 3:44 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Hi,
> >
> > Thanks!
> >
> > Some random comments about v92_001 (Sorry if it has already been discussed
> > up-thread):
> 
> Thanks for the feedback. The patch is pushed 15 minutes back.

Yeah, saw that after I send the comments ;-)

> I will
> prepare a top-up patch for your comments.

Thanks!

> > 4 ===
> >
> > +       if (wal_level < WAL_LEVEL_LOGICAL)
> > +       {
> > +               ereport(ERROR,
> > +                               errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > +                               errmsg("slot synchronization requires wal_level >= \"logical\""));
> > +               return false;
> > +       }
> >
> > I think the return is not needed here as it won't be reached due to the "ERROR".
> > Or should we use "elevel" instead of "ERROR"?
> 
> It was suggested to raise ERROR for wal_level validation, please see
> [1]. But yes, I will  remove the return value.

Yeah, thanks, ERROR makes sense here.

> > 5 ===
> >
> > +        * operate as a superuser. This is safe because the slot sync worker does
> > +        * not interact with user tables, eliminating the risk of executing
> > +        * arbitrary code within triggers.
> >
> > Right. I did not check but if we are using operators in our remote SPI calls
> > then it would be worth to ensure they are coming from the pg_catalog schema?
> > Using something like "OPERATOR(pg_catalog.=)" using "=" as an example.
> 
> Can you please elaborate this one, I am not sure if I understood it.

Suppose that in synchronize_slots() the query would be:

    const char *query = "SELECT slot_name, plugin, confirmed_flush_lsn,"
        " restart_lsn, catalog_xmin, two_phase, failover,"
        " database, conflict_reason"
        " FROM pg_catalog.pg_replication_slots"
        " WHERE failover and NOT temporary and 1 = 1";

Then my comment is to rewrite it to:

    const char *query = "SELECT slot_name, plugin, confirmed_flush_lsn,"
        " restart_lsn, catalog_xmin, two_phase, failover,"
        " database, conflict_reason"
        " FROM pg_catalog.pg_replication_slots"
        " WHERE failover and NOT temporary and 1 OPERATOR(pg_catalog.=) 1";

to ensure the operator "=" is coming from the pg_catalog schema.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com