Hi, On Fri, Feb 16, 2024 at 08:17:41PM +0100, Magnus Hagander wrote: > On Fri, Jan 19, 2024 at 7:20 AM Bertrand Drouvot > <bertranddrouvot...@gmail.com> wrote: > > > > Hi, > > > > On Thu, Jan 18, 2024 at 04:01:33PM +0100, Magnus Hagander wrote: > > > On Mon, Jan 15, 2024 at 11:17 AM Bertrand Drouvot > > > > Did you forget to share the new revision (aka v4)? I can only see the > > > > "reorder_parallel_worker_bestart.patch" attached. > > > > > > I did. Here it is, and also including that suggested docs fix as well > > > as a rebase on current master. > > > > > > > Thanks! > > > > Just a few comments: > > > > 1 === > > > > + The authentication method used for authenticating the connection, or > > + NULL for background processes. > > > > What about? "NULL for background processes (except for parallel workers > > which > > inherit this information from their leader process)" > > Ugh. That doesn't read very well at all to me. Maybe just "NULL for > background processes without a user"?
Not sure, as I think it could be NULL for background processes that provided a user in BackgroundWorkerInitializeConnection() too. > > 2 === > > > > + cycle before they were assigned a database role. Contains the same > > + value as the identity part in <xref linkend="system-user" />, or > > NULL > > + for background processes. > > > > Same comment about parallel workers. > > > > 3 === > > > > +# pg_stat_activity should contain trust and empty string for trust auth > > +$res = $node->safe_psql( > > + 'postgres', > > + "SELECT auth_method, auth_identity='' FROM pg_stat_activity WHERE > > pid=pg_backend_pid()", > > + connstr => "user=scram_role"); > > +is($res, 'trust|t', 'Users with trust authentication should show correctly > > in pg_stat_activity'); > > + > > +# pg_stat_activity should contain NULL for auth of background processes > > +# (test is a bit out of place here, but this is where the other > > pg_stat_activity.auth* tests are) > > +$res = $node->safe_psql( > > + 'postgres', > > + "SELECT auth_method IS NULL, auth_identity IS NULL FROM > > pg_stat_activity WHERE backend_type='checkpointer'", > > +); > > +is($res, 't|t', 'Background processes should show NULL for auth in > > pg_stat_activity'); > > > > What do you think about testing the parallel workers cases too? (I'm not > > 100% > > sure it's worth the extra complexity though). > > I'm leaning towards not worth that. Okay, I'm fine with that too. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com