Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Tue, Nov 21, 2017 at 4:09 AM, Fabrízio de Royes Mello
 wrote:
> Due to some "Blackfriday" commitments I'll be able to work again with this
> patch on next week.

Okay, this has proved to require broader changes than thought first. I
am marking the patch as returned with feedback.
-- 
Michael

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Mon, Nov 20, 2017 at 2:56 PM, Tom Lane  wrote:
>
> =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?=  writes:
> > typedef enum
> > {
> > ClientBackendProcess = -1,
> > CheckerProcess = 0,
> > BootstrapProcess,
>
> Uh, why would you do that (start from -1)?  It makes it impossible to
> build an array indexed by the enum, which might be useful --- converting
> enum values to strings is one obvious application.  Breaks your
> "NUM_PROCTYPES" thing, too.
>

I agree... I just copy and paste AuxProcType with some kind of
generalization.


> What I'd do is probably
>
> UnknownProcess = 0,
> PostmasterProc,
> StandaloneBackendProc,
> ClientBackendProc,
> BootstrapProc,
> ...
> NUM_PROCTYPES/* Must be last! */
>
> The value of reserving "UnknownProcess = 0" is that a freshly started
> process would be correctly not-identified if the variable starts out 0.
> (I'd be rather tempted to teach fork_process to reset it to 0 immediately
> after forking, too, so that postmaster children couldn't inadvertently
> retain the PostmasterProc setting.)
>

Makes sense...


> I'm not entirely sure whether standalone backends ought to get their
> own code, or whether it's better for them to share ClientBackendProc.
> It's something we'd have to work out as we went through the code
> making the patch.  How many places would want to distinguish, versus
> how many would have to test for two values?
>

Maybe for the first version we use just "ClientBackendProc" and refactor
all the code necessary to generalize AuxProcType. Then we can step into
into. Seems reasonable?


> > extern ProcType MyProcType;
>
> "PGProcType", maybe?  "ProcType" alone seems pretty generic.
> "ServerProcType" is another possibility for the typedef name,
> to emphasize that what we are classifying is the postmaster
> and its children.
>

"ServerProcType" seems a good name.


Due to some "Blackfriday" commitments I'll be able to work again with this
patch on next week.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Tom Lane]

=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?=  writes:
> typedef enum
> {
> ClientBackendProcess = -1,
> CheckerProcess = 0,
> BootstrapProcess,

Uh, why would you do that (start from -1)?  It makes it impossible to
build an array indexed by the enum, which might be useful --- converting
enum values to strings is one obvious application.  Breaks your
"NUM_PROCTYPES" thing, too.

What I'd do is probably

UnknownProcess = 0,
PostmasterProc,
StandaloneBackendProc,
ClientBackendProc,
BootstrapProc,
...
NUM_PROCTYPES/* Must be last! */

The value of reserving "UnknownProcess = 0" is that a freshly started
process would be correctly not-identified if the variable starts out 0.
(I'd be rather tempted to teach fork_process to reset it to 0 immediately
after forking, too, so that postmaster children couldn't inadvertently
retain the PostmasterProc setting.)

I'm not entirely sure whether standalone backends ought to get their
own code, or whether it's better for them to share ClientBackendProc.
It's something we'd have to work out as we went through the code
making the patch.  How many places would want to distinguish, versus
how many would have to test for two values?

> extern ProcType MyProcType;

"PGProcType", maybe?  "ProcType" alone seems pretty generic.
"ServerProcType" is another possibility for the typedef name,
to emphasize that what we are classifying is the postmaster
and its children.

regards, tom lane

Re: [HACKERS] [PATCH] A hook for session start

[Tom Lane]

Michael Paquier  writes:
> On Mon, Nov 20, 2017 at 12:11 PM, Tom Lane  wrote:
>> The stuff related to AuxProcType is in miscadmin.h, so one possibility
>> is to put the new enum there.  But I could see inventing a whole new
>> header for this, "utils/proctype.h" or so.

> I am on board for a new, dedicated, header, miscadmin.h having already
> a lot of things. So this API could just be located in its own file say
> in src/backend/utils/misc?

Maybe I'm missing something, but it seems like the only .c infrastructure
the header would need is a backend-global variable "MyProcType" or so.
That could perfectly well live in globals.c.

The model I'm imagining at the moment is that we generalize AuxProcType
to classify all PG processes not just "aux" processes.  The new header
would need to contain the enum typedef, an extern for the global
variable, and a bunch of test macros --- we'd move AmBootstrapProcess(),
IsAutoVacuumWorkerProcess(), and the rest of those into that header.
Whatever else pgstat is doing could probably stay in pgstat.c.  The
other infrastructure needed is for each kind of process to set MyProcType
as soon as possible during process start, but that's inherently something
not very centralizable.

Alternatively, we keep AuxProcType as-is, and invent a new enum that
classifies everything else, with one of its values being "AuxProc".
I'm not sure that sort of two-level scheme has any advantage, but
it's hard to tell for sure without doing the work to make a patch.

regards, tom lane

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Mon, Nov 20, 2017 at 12:11 PM, Tom Lane  wrote:
> This is really about consolidating a whole bunch of ad-hoc stuff.
> I don't think pgstat has any particular pride of place here.  It
> should be one consumer of a common API.
>
> The stuff related to AuxProcType is in miscadmin.h, so one possibility
> is to put the new enum there.  But I could see inventing a whole new
> header for this, "utils/proctype.h" or so.

I am on board for a new, dedicated, header, miscadmin.h having already
a lot of things. So this API could just be located in its own file say
in src/backend/utils/misc?
-- 
Michael

Re: [HACKERS] [PATCH] A hook for session start

[Tom Lane]

Michael Paquier  writes:
> I was the one suggesting to Fabrizio to look at how backend types are
> evaluated in pgstat.c after an off-list discussion. Agreed that this
> result is fragile as this makes two places dependent on the process
> types. Why not simply moving all the business of pgstat_bestart()
> evaluating which st_backendType to use into a common routine and have
> pgstat.c and have this utility use this API? pgstat.h already includes
> BackendType as an enum to use so this could live with a routine
> available in pgstat.c. Or would it be cleaner to have a new thing
> dedicated to process-related information, say as
> src/backend/utils/misc/process_info.c? It is indeed strange to have a
> session-related feature depend on something with statistics.

This is really about consolidating a whole bunch of ad-hoc stuff.
I don't think pgstat has any particular pride of place here.  It
should be one consumer of a common API.

The stuff related to AuxProcType is in miscadmin.h, so one possibility
is to put the new enum there.  But I could see inventing a whole new
header for this, "utils/proctype.h" or so.

regards, tom lane

Re: [HACKERS] [PATCH] A hook for session start

[Andrew Dunstan]

On 11/19/2017 04:49 PM, Tom Lane wrote:
> Andrew Dunstan  writes:
>> I think this:
>> #define IsClientBackend() \
>>     (MyBackendId != InvalidBackendId &&    \
>>  !IsAutoVacuumLauncherProcess() &&    \
>>  !IsAutoVacuumWorkerProcess() && \
>>  !am_walsender && \
>>  !IsBackgroundWorker)
>> probably belongs somewhere more central. Surely this isn't the only
>> place that we want to be able to run such a test?
> Hm.  It also seems awfully awkward.  Perhaps it's not being used anyplace
> performance-critical, but regardless of speed it seems like a modularity
> violation, in that client backends have to be explicitly aware of
> everything that isn't a "client backend".
>
> Maybe it's time to invent something corresponding to AuxProcType
> for non "aux" processes, or else to fold all types of Postgres
> processes into the same enum.



Yes, agreed, The above is pretty ugly and likely to be quite fragile.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [PATCH] A hook for session start

[Tom Lane]

Andrew Dunstan  writes:
> I think this:

> #define IsClientBackend() \
>     (MyBackendId != InvalidBackendId &&    \
>  !IsAutoVacuumLauncherProcess() &&    \
>  !IsAutoVacuumWorkerProcess() && \
>  !am_walsender && \
>  !IsBackgroundWorker)

> probably belongs somewhere more central. Surely this isn't the only
> place that we want to be able to run such a test?

Hm.  It also seems awfully awkward.  Perhaps it's not being used anyplace
performance-critical, but regardless of speed it seems like a modularity
violation, in that client backends have to be explicitly aware of
everything that isn't a "client backend".

Maybe it's time to invent something corresponding to AuxProcType
for non "aux" processes, or else to fold all types of Postgres
processes into the same enum.

regards, tom lane

Re: [HACKERS] [PATCH] A hook for session start

[Andrew Dunstan]

On 11/16/2017 10:38 PM, Fabrízio de Royes Mello wrote:
> Hi all,
>
> Attached new version of the patch fixing issues about installcheck and
> some assertions reported before (based on Michael Paquier code):
>
> https://www.postgresql.org/message-id/flat/30479.1510800078%40sss.pgh.pa.us#30479.1510800...@sss.pgh.pa.us
> https://www.postgresql.org/message-id/flat/20171116121823.GI4628%40tamriel.snowman.net#20171116121823.gi4...@tamriel.snowman.net
>
> Regards,



I think this:

#define IsClientBackend() \
    (MyBackendId != InvalidBackendId &&    \
 !IsAutoVacuumLauncherProcess() &&    \
 !IsAutoVacuumWorkerProcess() && \
 !am_walsender && \
 !IsBackgroundWorker)


probably belongs somewhere more central. Surely this isn't the only
place that we want to be able to run such a test?

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [PATCH] A hook for session start

[Fabrízio de Royes Mello]

On Fri, Nov 17, 2017 at 3:33 AM, Ashutosh Sharma 
wrote:
>
> On Fri, Nov 17, 2017 at 9:08 AM, Fabrízio de Royes Mello
>  wrote:
> > Hi all,
> >
> > Attached new version of the patch fixing issues about installcheck and
some
> > assertions reported before (based on Michael Paquier code):
> >
>
> The assertion failure which i reported earlier -[1] is fixed with v11
> patch. FYI, in my earlier email i wrongly mentioned that the assertion
> failure is happening in bgwroker process (logical replication worker)
> infact it was seen in the backend process.
>

I saw it in "Autovacuum Launcher", more specifically during "session end
hook" so I realize that the code to check if the current backend is a
client backend isn't safe so I've fixed it.


> By installcheck, did you mean installcheck-force because installcheck
> rule is not defined in the Makefile.
>

I meant to simple disable 'installcheck' because we need the
shared_preload_libraries has been properly configured. But you can use
'installcheck-force' if you manually config and install the test module.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] [PATCH] A hook for session start

[Ashutosh Sharma]

On Fri, Nov 17, 2017 at 9:08 AM, Fabrízio de Royes Mello
 wrote:
> Hi all,
>
> Attached new version of the patch fixing issues about installcheck and some
> assertions reported before (based on Michael Paquier code):
>

The assertion failure which i reported earlier -[1] is fixed with v11
patch. FYI, in my earlier email i wrongly mentioned that the assertion
failure is happening in bgwroker process (logical replication worker)
infact it was seen in the backend process.

By installcheck, did you mean installcheck-force because installcheck
rule is not defined in the Makefile.

[1] - 
https://www.postgresql.org/message-id/CAE9k0P%3DtX_egPEX9NzPrroumXt5%3DbOQBiP98CaLzHOyXk7%2Bq7Q%40mail.gmail.com

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

> https://www.postgresql.org/message-id/flat/30479.1510800078%40sss.pgh.pa.us#30479.1510800...@sss.pgh.pa.us
> https://www.postgresql.org/message-id/flat/20171116121823.GI4628%40tamriel.snowman.net#20171116121823.gi4...@tamriel.snowman.net
>
> Regards,
>
> --
> Fabrízio de Royes Mello
> Consultoria/Coaching PostgreSQL
>>> Timbira: http://www.timbira.com.br
>>> Blog: http://fabriziomello.github.io
>>> Linkedin: http://br.linkedin.com/in/fabriziomello
>>> Twitter: http://twitter.com/fabriziomello
>>> Github: http://github.com/fabriziomello