Re: Synchronizing slots from primary to standby

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. I will
prepare a top-up patch for your comments.

> 1 ===
>
> +        * We do not update the 'synced' column from true to false here
>
> Worth to mention from which system view the 'synced' column belongs to?

Sure, I will change it.

> 2 === (Nit)
>
> +#define MIN_WORKER_NAPTIME_MS  200
> +#define MAX_WORKER_NAPTIME_MS  30000   /* 30s */
>
> [MIN|MAX]_SLOTSYNC_WORKER_NAPTIME_MS instead? It is used only in slotsync.c so
> more a Nit.

Okay, will change it,

> 3 ===
>
>         res = walrcv_exec(wrconn, query, SLOTSYNC_COLUMN_COUNT, slotRow);
> -
>         if (res->status != WALRCV_OK_TUPLES)
>
> Line removal intended?

I feel the current style is better, where we do not have space between
the function call and return value checking.

> 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. Thanks for catching
this.

> 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.

[1]: https://www.postgresql.org/message-id/CAD21AoB2ipSzQb5-o5pEYKie4oTPJTsYR1ip9_wRVrF6HbBWDQ%40mail.gmail.com

thanks
Shveta