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
