> On Thu, Dec 1, 2022 at 6:14 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Wed, Nov 23, 2022 at 1:53 AM Steve Chavez <steve@supabase.io> wrote:
> > > So from my side this all looks good!
> >
> > Thank you for your feedback.
> >
> > The next revision of the patch is attached. It contains code
> > improvements, comments and documentation. I'm going to also write
> > sode tests. pg_db_role_setting doesn't seem to be well-covered with
> > tests. I will probably need to write a new module into
> > src/tests/modules to check now placeholders interacts with dynamically
> > defined GUCs.
>
> Another revision of patch is attached. It's fixed now that USER SET
> values can't be used for PGC_SUSET parameters. Tests are added. That
> require new module test_pg_db_role_setting to check dynamically
> defined GUCs.
I've looked through the last version of a patch. The tests in v3
failed due to naming mismatches. I fixed this in v4 (PFA).
The other thing that may seem unexpected: is whether the value should
apply to the ordinary user only, encoded in the parameter name. The
pro of this is that it doesn't break catalog compatibility by a
separate field for GUC permissions a concept that doesn't exist today
(and maybe not needed at all). Also, it makes the patch more
minimalistic in the code. This is also fully compatible with the
previous parameters naming due to parentheses being an unsupported
symbol for the parameter name.
I've also tried to revise the comments and docs a little bit to
reflect the changes.
The CI-enabled build of patch v4 for reference is at
https://github.com/pashkinelfe/postgres/tree/placeholders-in-alter-role-v4
Overall the patch looks useful and good enough to be committed.
Kind regards,
Pavel Borisov,
Supabase
