Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add hooks for session start and session end, take two

  1. [PATCH] A hook for session start

    Yugo Nagata <nagata@sraoss.co.jp> — 2017-07-20T11:47:33Z

    Hi,

Currently, PostgreSQL doen't have a hook triggered at session
start. Although we already have ClientAuthentication_hook,
this is triggered during authentication, so we can not
access the database. 

If we have a hook triggerd only once at session start, we may
do something useful on the session for certain database or user.

For example, one of our clients wanted such feature. He wanted
to handle encription for specific users, though I don't know
the detail.

The attached patch (session_start_hook.patch) implements such
hook. 

Another patch, session_start_sample.patch, is a very simple
example of this hook that changes work_mem values for sessions
of a specific database. 

I would appreciate hearing your opinion on this hook.

Regards,

-- 
Yugo Nagata <nagata@sraoss.co.jp>

  2. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-07-20T12:27:22Z

    On Thu, Jul 20, 2017 at 8:47 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> Hi,
>
> Currently, PostgreSQL doen't have a hook triggered at session
> start. Although we already have ClientAuthentication_hook,
> this is triggered during authentication, so we can not
> access the database.
>
> If we have a hook triggerd only once at session start, we may
> do something useful on the session for certain database or user.
>
> For example, one of our clients wanted such feature. He wanted
> to handle encription for specific users, though I don't know
> the detail.
>
> The attached patch (session_start_hook.patch) implements such
> hook.
>
> Another patch, session_start_sample.patch, is a very simple
> example of this hook that changes work_mem values for sessions
> of a specific database.
>
> I would appreciate hearing your opinion on this hook.
>

I'm not sure your real needs but doesn't it material for improve Event
Triggers???

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

  3. Re: [PATCH] A hook for session start

    Robert Haas <robertmhaas@gmail.com> — 2017-07-21T00:42:45Z

    On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> I'm not sure your real needs but doesn't it material for improve Event
> Triggers???

I've thought about that, too.  One problem is what to do if the user
hits ^C while the event trigger procedure is running.  If you respond
to that by killing the event trigger and letting the user issue
commands, then the event trigger can't be used for security or
auditing purposes because the user might prevent it from doing
whatever it's intended to do with a well-timed interrupt.  If you
ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
can lock users out of the database.  Maybe that's OK.  We could say
"well, if you lock yourself out of the database with your logon
trigger, you get to shut down the database and restart in single user
mode to recover".

A hook, as proposed here, is a lot simpler and lacks these concerns.
Installing code in C into the database is intrinsically risky
anywhere, and not any moreso here than elsewhere.  But it's also less
accessible to the average user.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  4. Re: [PATCH] A hook for session start

    Craig Ringer <craig@2ndquadrant.com> — 2017-07-21T01:53:19Z

    On 21 July 2017 at 08:42, Robert Haas <robertmhaas@gmail.com> wrote:

> On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > I'm not sure your real needs but doesn't it material for improve Event
> > Triggers???
>
> I've thought about that, too.  One problem is what to do if the user
> hits ^C while the event trigger procedure is running.  If you respond
> to that by killing the event trigger and letting the user issue
> commands, then the event trigger can't be used for security or
> auditing purposes because the user might prevent it from doing
> whatever it's intended to do with a well-timed interrupt.  If you
> ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> can lock users out of the database.  Maybe that's OK.  We could say
> "well, if you lock yourself out of the database with your logon
> trigger, you get to shut down the database and restart in single user
> mode to recover".
>
> A hook, as proposed here, is a lot simpler and lacks these concerns.
> Installing code in C into the database is intrinsically risky
> anywhere, and not any moreso here than elsewhere.  But it's also less
> accessible to the average user.
> <http://www.postgresql.org/mailpref/pgsql-hackers>


I'd favour the c hook personally. It's a lot more flexible, and can be used
by an extension to implement trigger-like behaviour if anyone wants it,
including the extension's choice of error handling decisions.

It's also a lot simpler and less intrusive for core. Which is nice where we
don't have something that we don't have anything compelling destined for
core that needs it. (I want to add a bunch of hooks in the logical
replication code in pg11 for similar reasons, and so features like DDL
replication can be prototyped as extensions more practically).

That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve the
same job as a session-start hook, albeit at slightly higher overhead? You
can just test to see if your initial tasks have run yet.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  5. Re: [PATCH] A hook for session start

    Yugo Nagata <nagata@sraoss.co.jp> — 2017-07-21T12:35:01Z

    On Fri, 21 Jul 2017 09:53:19 +0800
Craig Ringer <craig@2ndquadrant.com> wrote:

> On 21 July 2017 at 08:42, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > <fabriziomello@gmail.com> wrote:
> > > I'm not sure your real needs but doesn't it material for improve Event
> > > Triggers???
> >
> > I've thought about that, too.  One problem is what to do if the user
> > hits ^C while the event trigger procedure is running.  If you respond
> > to that by killing the event trigger and letting the user issue
> > commands, then the event trigger can't be used for security or
> > auditing purposes because the user might prevent it from doing
> > whatever it's intended to do with a well-timed interrupt.  If you
> > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > can lock users out of the database.  Maybe that's OK.  We could say
> > "well, if you lock yourself out of the database with your logon
> > trigger, you get to shut down the database and restart in single user
> > mode to recover".
> >
> > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > Installing code in C into the database is intrinsically risky
> > anywhere, and not any moreso here than elsewhere.  But it's also less
> > accessible to the average user.
> > <http://www.postgresql.org/mailpref/pgsql-hackers>
> 
> 
> I'd favour the c hook personally. It's a lot more flexible, and can be used
> by an extension to implement trigger-like behaviour if anyone wants it,
> including the extension's choice of error handling decisions.
> 
> It's also a lot simpler and less intrusive for core. Which is nice where we
> don't have something that we don't have anything compelling destined for
> core that needs it. (I want to add a bunch of hooks in the logical
> replication code in pg11 for similar reasons, and so features like DDL
> replication can be prototyped as extensions more practically).
> 
> That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve the
> same job as a session-start hook, albeit at slightly higher overhead? You
> can just test to see if your initial tasks have run yet.

Thank you for your suggestion. Certainly, we can do the similar job of a
session-start hook using these existing hooks, although these hooks are
triggered when the first query is executed not when the session is started.
Now I come to think that an additional hook is not need.

Thanks,

> 
> -- 
>  Craig Ringer                   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Yugo Nagata <nagata@sraoss.co.jp>

  6. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-07-21T13:31:57Z

    On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> On Fri, 21 Jul 2017 09:53:19 +0800
> Craig Ringer <craig@2ndquadrant.com> wrote:
>
> > On 21 July 2017 at 08:42, Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > <fabriziomello@gmail.com> wrote:
> > > > I'm not sure your real needs but doesn't it material for improve
Event
> > > > Triggers???
> > >
> > > I've thought about that, too.  One problem is what to do if the user
> > > hits ^C while the event trigger procedure is running.  If you respond
> > > to that by killing the event trigger and letting the user issue
> > > commands, then the event trigger can't be used for security or
> > > auditing purposes because the user might prevent it from doing
> > > whatever it's intended to do with a well-timed interrupt.  If you
> > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > > can lock users out of the database.  Maybe that's OK.  We could say
> > > "well, if you lock yourself out of the database with your logon
> > > trigger, you get to shut down the database and restart in single user
> > > mode to recover".
> > >
> > > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > > Installing code in C into the database is intrinsically risky
> > > anywhere, and not any moreso here than elsewhere.  But it's also less
> > > accessible to the average user.
> > > <http://www.postgresql.org/mailpref/pgsql-hackers>
> >
> >
> > I'd favour the c hook personally. It's a lot more flexible, and can be
used
> > by an extension to implement trigger-like behaviour if anyone wants it,
> > including the extension's choice of error handling decisions.
> >
> > It's also a lot simpler and less intrusive for core. Which is nice
where we
> > don't have something that we don't have anything compelling destined for
> > core that needs it. (I want to add a bunch of hooks in the logical
> > replication code in pg11 for similar reasons, and so features like DDL
> > replication can be prototyped as extensions more practically).
> >

I agree with you both...


> > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve
the
> > same job as a session-start hook, albeit at slightly higher overhead?
You
> > can just test to see if your initial tasks have run yet.
>
> Thank you for your suggestion. Certainly, we can do the similar job of a
> session-start hook using these existing hooks, although these hooks are
> triggered when the first query is executed not when the session is
started.
> Now I come to think that an additional hook is not need.
>

As Nagata said hooks proposed by Craing will happens only when the first
query is called so I don't know how it works for session start... are we
missing something?

If we're going to add this hook what about add a session end hook also?

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

  7. Re: [PATCH] A hook for session start

    Yugo Nagata <nagata@sraoss.co.jp> — 2017-07-21T13:58:51Z

    On Fri, 21 Jul 2017 10:31:57 -0300
Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:

> On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> >
> > On Fri, 21 Jul 2017 09:53:19 +0800
> > Craig Ringer <craig@2ndquadrant.com> wrote:
> >
> > > On 21 July 2017 at 08:42, Robert Haas <robertmhaas@gmail.com> wrote:
> > >
> > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > > <fabriziomello@gmail.com> wrote:
> > > > > I'm not sure your real needs but doesn't it material for improve
> Event
> > > > > Triggers???
> > > >
> > > > I've thought about that, too.  One problem is what to do if the user
> > > > hits ^C while the event trigger procedure is running.  If you respond
> > > > to that by killing the event trigger and letting the user issue
> > > > commands, then the event trigger can't be used for security or
> > > > auditing purposes because the user might prevent it from doing
> > > > whatever it's intended to do with a well-timed interrupt.  If you
> > > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > > > can lock users out of the database.  Maybe that's OK.  We could say
> > > > "well, if you lock yourself out of the database with your logon
> > > > trigger, you get to shut down the database and restart in single user
> > > > mode to recover".
> > > >
> > > > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > > > Installing code in C into the database is intrinsically risky
> > > > anywhere, and not any moreso here than elsewhere.  But it's also less
> > > > accessible to the average user.
> > > > <http://www.postgresql.org/mailpref/pgsql-hackers>
> > >
> > >
> > > I'd favour the c hook personally. It's a lot more flexible, and can be
> used
> > > by an extension to implement trigger-like behaviour if anyone wants it,
> > > including the extension's choice of error handling decisions.
> > >
> > > It's also a lot simpler and less intrusive for core. Which is nice
> where we
> > > don't have something that we don't have anything compelling destined for
> > > core that needs it. (I want to add a bunch of hooks in the logical
> > > replication code in pg11 for similar reasons, and so features like DDL
> > > replication can be prototyped as extensions more practically).
> > >
> 
> I agree with you both...
> 
> 
> > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve
> the
> > > same job as a session-start hook, albeit at slightly higher overhead?
> You
> > > can just test to see if your initial tasks have run yet.
> >
> > Thank you for your suggestion. Certainly, we can do the similar job of a
> > session-start hook using these existing hooks, although these hooks are
> > triggered when the first query is executed not when the session is
> started.
> > Now I come to think that an additional hook is not need.
> >
> 
> As Nagata said hooks proposed by Craing will happens only when the first
> query is called so I don't know how it works for session start... are we
> missing something?

Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
session_start hook. If a query is issued a long time since the session start,
the timing the hook happens is largely deviated. It is no problem if we only
want do something once at the session start, but it might be problem if
we want to record the timestamp of the session start, for example.

> 
> If we're going to add this hook what about add a session end hook also?

If someone want the session-start hook, he might want this too.

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


-- 
Yugo Nagata <nagata@sraoss.co.jp>

  8. Re: [PATCH] A hook for session start

    Craig Ringer <craig@2ndquadrant.com> — 2017-07-21T15:10:52Z

    On 21 Jul. 2017 21:58, "Yugo Nagata" <nagata@sraoss.co.jp> wrote:

On Fri, 21 Jul 2017 10:31:57 -0300
Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:

> On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> >
> > On Fri, 21 Jul 2017 09:53:19 +0800
> > Craig Ringer <craig@2ndquadrant.com> wrote:
> >
> > > On 21 July 2017 at 08:42, Robert Haas <robertmhaas@gmail.com> wrote:
> > >
> > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > > <fabriziomello@gmail.com> wrote:
> > > > > I'm not sure your real needs but doesn't it material for improve
> Event
> > > > > Triggers???
> > > >
> > > > I've thought about that, too.  One problem is what to do if the user
> > > > hits ^C while the event trigger procedure is running.  If you
respond
> > > > to that by killing the event trigger and letting the user issue
> > > > commands, then the event trigger can't be used for security or
> > > > auditing purposes because the user might prevent it from doing
> > > > whatever it's intended to do with a well-timed interrupt.  If you
> > > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger
> > > > can lock users out of the database.  Maybe that's OK.  We could say
> > > > "well, if you lock yourself out of the database with your logon
> > > > trigger, you get to shut down the database and restart in single
user
> > > > mode to recover".
> > > >
> > > > A hook, as proposed here, is a lot simpler and lacks these concerns.
> > > > Installing code in C into the database is intrinsically risky
> > > > anywhere, and not any moreso here than elsewhere.  But it's also
less
> > > > accessible to the average user.
> > > > <http://www.postgresql.org/mailpref/pgsql-hackers>
> > >
> > >
> > > I'd favour the c hook personally. It's a lot more flexible, and can be
> used
> > > by an extension to implement trigger-like behaviour if anyone wants
it,
> > > including the extension's choice of error handling decisions.
> > >
> > > It's also a lot simpler and less intrusive for core. Which is nice
> where we
> > > don't have something that we don't have anything compelling destined
for
> > > core that needs it. (I want to add a bunch of hooks in the logical
> > > replication code in pg11 for similar reasons, and so features like DDL
> > > replication can be prototyped as extensions more practically).
> > >
>
> I agree with you both...
>
>
> > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to
serve
> the
> > > same job as a session-start hook, albeit at slightly higher overhead?
> You
> > > can just test to see if your initial tasks have run yet.
> >
> > Thank you for your suggestion. Certainly, we can do the similar job of a
> > session-start hook using these existing hooks, although these hooks are
> > triggered when the first query is executed not when the session is
> started.
> > Now I come to think that an additional hook is not need.
> >
>
> As Nagata said hooks proposed by Craing will happens only when the first
> query is called so I don't know how it works for session start... are we
> missing something?

Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
session_start hook. If a query is issued a long time since the session
start,
the timing the hook happens is largely deviated. It is no problem if we only
want do something once at the session start, but it might be problem if
we want to record the timestamp of the session start, for example.


Don't we have that timestamp already?

What practical use cases are there for acting post-auth but that can't wait
until the user tries to do something?

Can a user do anything remotely interesting or useful without hitting
either ExecutorStart_hook or ProcessUtility_hook? They can parse queries I
guess but you could just set your hook up in the parser instead. If you
hook the parser all they can do is open an idle session and sit there...

So given that you can effectively do it already at the C hook level, if
you're going to do it at all I guess it it'd be more interesting to expose
a convenient event trigger for session start. As others suggested upthread.
So it's easy for DBAs and devs who won't have any idea where to start
writing extensions that register hooks.

But... I think you need a good use case. Such a trigger would have no way
to receive parameters from the user (except custom GUCs) or report any sort
of result other than an error/warning/notice. So what's it going to do that
can't already be decided by pg_hba.cond, pg_authid etc?

  9. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-07-21T15:19:35Z

    On Fri, Jul 21, 2017 at 10:58 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> On Fri, 21 Jul 2017 10:31:57 -0300
> Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
> > On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nagata@sraoss.co.jp>
wrote:
> > >
> > > On Fri, 21 Jul 2017 09:53:19 +0800
> > > Craig Ringer <craig@2ndquadrant.com> wrote:
> > >
> > > > On 21 July 2017 at 08:42, Robert Haas <robertmhaas@gmail.com> wrote:
> > > >
> > > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > > > <fabriziomello@gmail.com> wrote:
> > > > > > I'm not sure your real needs but doesn't it material for improve
> > Event
> > > > > > Triggers???
> > > > >
> > > > > I've thought about that, too.  One problem is what to do if the
user
> > > > > hits ^C while the event trigger procedure is running.  If you
respond
> > > > > to that by killing the event trigger and letting the user issue
> > > > > commands, then the event trigger can't be used for security or
> > > > > auditing purposes because the user might prevent it from doing
> > > > > whatever it's intended to do with a well-timed interrupt.  If you
> > > > > ignore ^C or make it turn into FATAL, then a poorly-crafted
trigger
> > > > > can lock users out of the database.  Maybe that's OK.  We could
say
> > > > > "well, if you lock yourself out of the database with your logon
> > > > > trigger, you get to shut down the database and restart in single
user
> > > > > mode to recover".
> > > > >
> > > > > A hook, as proposed here, is a lot simpler and lacks these
concerns.
> > > > > Installing code in C into the database is intrinsically risky
> > > > > anywhere, and not any moreso here than elsewhere.  But it's also
less
> > > > > accessible to the average user.
> > > > > <http://www.postgresql.org/mailpref/pgsql-hackers>
> > > >
> > > >
> > > > I'd favour the c hook personally. It's a lot more flexible, and can
be
> > used
> > > > by an extension to implement trigger-like behaviour if anyone wants
it,
> > > > including the extension's choice of error handling decisions.
> > > >
> > > > It's also a lot simpler and less intrusive for core. Which is nice
> > where we
> > > > don't have something that we don't have anything compelling
destined for
> > > > core that needs it. (I want to add a bunch of hooks in the logical
> > > > replication code in pg11 for similar reasons, and so features like
DDL
> > > > replication can be prototyped as extensions more practically).
> > > >
> >
> > I agree with you both...
> >
> >
> > > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to
serve
> > the
> > > > same job as a session-start hook, albeit at slightly higher
overhead?
> > You
> > > > can just test to see if your initial tasks have run yet.
> > >
> > > Thank you for your suggestion. Certainly, we can do the similar job
of a
> > > session-start hook using these existing hooks, although these hooks
are
> > > triggered when the first query is executed not when the session is
> > started.
> > > Now I come to think that an additional hook is not need.
> > >
> >
> > As Nagata said hooks proposed by Craing will happens only when the first
> > query is called so I don't know how it works for session start... are we
> > missing something?
>
> Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
> session_start hook. If a query is issued a long time since the session
start,
> the timing the hook happens is largely deviated. It is no problem if we
only
> want do something once at the session start, but it might be problem if
> we want to record the timestamp of the session start, for example.
>
> >
> > If we're going to add this hook what about add a session end hook also?
>
> If someone want the session-start hook, he might want this too.
>

Well if someone wants here are the patches... I just did a minor fix and
cleanup in your previous session_start sample and provide both samples into
the same patch.

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

  10. Re: [PATCH] A hook for session start

    Robert Haas <robertmhaas@gmail.com> — 2017-07-21T16:59:04Z

    On Fri, Jul 21, 2017 at 11:10 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> Don't we have that timestamp already?
>
> What practical use cases are there for acting post-auth but that can't wait
> until the user tries to do something?

Have, yes; record, no.

> Can a user do anything remotely interesting or useful without hitting either
> ExecutorStart_hook or ProcessUtility_hook? They can parse queries I guess
> but you could just set your hook up in the parser instead. If you hook the
> parser all they can do is open an idle session and sit there...

That's an exceedingly-weak argument for rejecting this patch.  The
fact that you can probably hack around the lack of a hook for most
reasonable use cases is not an argument for having a hook that does
what people actually want to do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  11. Re: [PATCH] A hook for session start

    Jim Mlodgenski <jimmy76@gmail.com> — 2017-07-21T17:14:13Z

    >
> > Can a user do anything remotely interesting or useful without hitting
> either
> > ExecutorStart_hook or ProcessUtility_hook? They can parse queries I guess
> > but you could just set your hook up in the parser instead. If you hook
> the
> > parser all they can do is open an idle session and sit there...
>
> That's an exceedingly-weak argument for rejecting this patch.  The
> fact that you can probably hack around the lack of a hook for most
> reasonable use cases is not an argument for having a hook that does
> what people actually want to do.
>
>
When I first saw this thread, my initial thought of a use case is to
prepare some key application queries so they are there and ready to go.
That would need to be before the ExecutorStart_hook or ProcessUtility_hook
if an app would just want to execute the prepared statement.

  12. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-07-21T19:14:19Z

    On Fri, Jul 21, 2017 at 12:19 PM, Fabrízio de Royes Mello <
fabriziomello@gmail.com> wrote:
>
>
> On Fri, Jul 21, 2017 at 10:58 AM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> >
> > On Fri, 21 Jul 2017 10:31:57 -0300
> > Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
> >
> > > On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nagata@sraoss.co.jp>
wrote:
> > > >
> > > > On Fri, 21 Jul 2017 09:53:19 +0800
> > > > Craig Ringer <craig@2ndquadrant.com> wrote:
> > > >
> > > > > On 21 July 2017 at 08:42, Robert Haas <robertmhaas@gmail.com>
wrote:
> > > > >
> > > > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello
> > > > > > <fabriziomello@gmail.com> wrote:
> > > > > > > I'm not sure your real needs but doesn't it material for
improve
> > > Event
> > > > > > > Triggers???
> > > > > >
> > > > > > I've thought about that, too.  One problem is what to do if the
user
> > > > > > hits ^C while the event trigger procedure is running.  If you
respond
> > > > > > to that by killing the event trigger and letting the user issue
> > > > > > commands, then the event trigger can't be used for security or
> > > > > > auditing purposes because the user might prevent it from doing
> > > > > > whatever it's intended to do with a well-timed interrupt.  If
you
> > > > > > ignore ^C or make it turn into FATAL, then a poorly-crafted
trigger
> > > > > > can lock users out of the database.  Maybe that's OK.  We could
say
> > > > > > "well, if you lock yourself out of the database with your logon
> > > > > > trigger, you get to shut down the database and restart in
single user
> > > > > > mode to recover".
> > > > > >
> > > > > > A hook, as proposed here, is a lot simpler and lacks these
concerns.
> > > > > > Installing code in C into the database is intrinsically risky
> > > > > > anywhere, and not any moreso here than elsewhere.  But it's
also less
> > > > > > accessible to the average user.
> > > > > > <http://www.postgresql.org/mailpref/pgsql-hackers>
> > > > >
> > > > >
> > > > > I'd favour the c hook personally. It's a lot more flexible, and
can be
> > > used
> > > > > by an extension to implement trigger-like behaviour if anyone
wants it,
> > > > > including the extension's choice of error handling decisions.
> > > > >
> > > > > It's also a lot simpler and less intrusive for core. Which is nice
> > > where we
> > > > > don't have something that we don't have anything compelling
destined for
> > > > > core that needs it. (I want to add a bunch of hooks in the logical
> > > > > replication code in pg11 for similar reasons, and so features
like DDL
> > > > > replication can be prototyped as extensions more practically).
> > > > >
> > >
> > > I agree with you both...
> > >
> > >
> > > > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to
serve
> > > the
> > > > > same job as a session-start hook, albeit at slightly higher
overhead?
> > > You
> > > > > can just test to see if your initial tasks have run yet.
> > > >
> > > > Thank you for your suggestion. Certainly, we can do the similar job
of a
> > > > session-start hook using these existing hooks, although these hooks
are
> > > > triggered when the first query is executed not when the session is
> > > started.
> > > > Now I come to think that an additional hook is not need.
> > > >
> > >
> > > As Nagata said hooks proposed by Craing will happens only when the
first
> > > query is called so I don't know how it works for session start... are
we
> > > missing something?
> >
> > Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as
> > session_start hook. If a query is issued a long time since the session
start,
> > the timing the hook happens is largely deviated. It is no problem if we
only
> > want do something once at the session start, but it might be problem if
> > we want to record the timestamp of the session start, for example.
> >
> > >
> > > If we're going to add this hook what about add a session end hook
also?
> >
> > If someone want the session-start hook, he might want this too.
> >
>
> Well if someone wants here are the patches... I just did a minor fix and
cleanup in your previous session_start sample and provide both samples into
the same patch.
>

I made a mistake on previous patch... now the attached three patches in
their correct orders.

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

  13. Re: [PATCH] A hook for session start

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-08-01T19:36:23Z

    On 7/21/17 13:14, Jim Mlodgenski wrote:
> When I first saw this thread, my initial thought of a use case is to
> prepare some key application queries so they are there and ready to go.
> That would need to be before the ExecutorStart_hook or
> ProcessUtility_hook if an app would just want to execute the prepared
> statement.

Isn't that what the preprepare extension does already?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  14. Re: [PATCH] A hook for session start

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-08-01T19:36:59Z

    On 7/20/17 07:47, Yugo Nagata wrote:
> Another patch, session_start_sample.patch, is a very simple
> example of this hook that changes work_mem values for sessions
> of a specific database. 

I think test modules should go into src/test/modules/ instead of contrib.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  15. Re: [PATCH] A hook for session start

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-08-01T19:37:40Z

    On 7/21/17 12:59, Robert Haas wrote:
> That's an exceedingly-weak argument for rejecting this patch.  The
> fact that you can probably hack around the lack of a hook for most
> reasonable use cases is not an argument for having a hook that does
> what people actually want to do.

Still nobody has presented a concrete use case so far.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  16. Re: [PATCH] A hook for session start

    Andres Freund <andres@anarazel.de> — 2017-08-01T19:42:33Z

    On 2017-08-01 15:37:40 -0400, Peter Eisentraut wrote:
> On 7/21/17 12:59, Robert Haas wrote:
> > That's an exceedingly-weak argument for rejecting this patch.  The
> > fact that you can probably hack around the lack of a hook for most
> > reasonable use cases is not an argument for having a hook that does
> > what people actually want to do.
> 
> Still nobody has presented a concrete use case so far.

Citus for example starts a background worker (performing
e.g. distributed deadlock detection) if the citus extension exists. We
atm need annoying hacks to do so when the first query is executed.

- Andres

  17. Re: [PATCH] A hook for session start

    Robert Haas <robertmhaas@gmail.com> — 2017-08-01T19:55:17Z

    On Tue, Aug 1, 2017 at 3:37 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 7/21/17 12:59, Robert Haas wrote:
>> That's an exceedingly-weak argument for rejecting this patch.  The
>> fact that you can probably hack around the lack of a hook for most
>> reasonable use cases is not an argument for having a hook that does
>> what people actually want to do.
>
> Still nobody has presented a concrete use case so far.

I've been asked for this at EDB, too.  Inserting a row into some table
on each logon, for example.

A quick Google search found 6 previous requests for this feature, some
of which describe intended use cases:

https://www.postgresql.org/message-id/4EBC6852.5030605@fuzzy.cz (2011)
https://www.postgresql.org/message-id/CAHyXU0wrsYShxmwBxZSGYoiBJa%3DgzEJ17iAeRvaf_vA%2BcoH_qA%40mail.gmail.com
(2011)
https://www.postgresql.org/message-id/BAY104-W513CF26C0046C9D28747B8D1DD0@phx.gbl
(2009, in Spanish)
https://www.postgresql.org/message-id/758d5e7f0803130227m558d32cdl7159bed00d21f084%40mail.gmail.com
(2008)
https://www.postgresql.org/message-id/001a01c48077%240b118e60%240200030a%40gendron.ca
(2004)
https://www.postgresql.org/message-id/F96SgcOrBLSAQV6uPDV00000a2b@hotmail.com
(2000)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  18. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-10-05T19:14:21Z

    On Tue, Aug 1, 2017 at 4:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Aug 1, 2017 at 3:37 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
> > On 7/21/17 12:59, Robert Haas wrote:
> >> That's an exceedingly-weak argument for rejecting this patch.  The
> >> fact that you can probably hack around the lack of a hook for most
> >> reasonable use cases is not an argument for having a hook that does
> >> what people actually want to do.
> >
> > Still nobody has presented a concrete use case so far.
>
> I've been asked for this at EDB, too.  Inserting a row into some table
> on each logon, for example.
>
> A quick Google search found 6 previous requests for this feature, some
> of which describe intended use cases:
>
> https://www.postgresql.org/message-id/4EBC6852.5030605@fuzzy.cz (2011)
>
https://www.postgresql.org/message-id/CAHyXU0wrsYShxmwBxZSGYoiBJa%3DgzEJ17iAeRvaf_vA%2BcoH_qA%40mail.gmail.com
> (2011)
>
https://www.postgresql.org/message-id/BAY104-W513CF26C0046C9D28747B8D1DD0@phx.gbl
> (2009, in Spanish)
>
https://www.postgresql.org/message-id/758d5e7f0803130227m558d32cdl7159bed00d21f084%40mail.gmail.com
> (2008)
>
https://www.postgresql.org/message-id/001a01c48077%240b118e60%240200030a%40gendron.ca
> (2004)
>
https://www.postgresql.org/message-id/F96SgcOrBLSAQV6uPDV00000a2b@hotmail.com
> (2000)
>

Hi all,

I'm sending a new rebased patches and added tests to src/tests/modules as
suggested before.

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

  19. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-10-05T19:18:59Z

    On Thu, Oct 5, 2017 at 4:14 PM, Fabrízio de Royes Mello <
fabriziomello@gmail.com> wrote:
>
>
>
> On Tue, Aug 1, 2017 at 4:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Tue, Aug 1, 2017 at 3:37 PM, Peter Eisentraut
> > <peter.eisentraut@2ndquadrant.com> wrote:
> > > On 7/21/17 12:59, Robert Haas wrote:
> > >> That's an exceedingly-weak argument for rejecting this patch.  The
> > >> fact that you can probably hack around the lack of a hook for most
> > >> reasonable use cases is not an argument for having a hook that does
> > >> what people actually want to do.
> > >
> > > Still nobody has presented a concrete use case so far.
> >
> > I've been asked for this at EDB, too.  Inserting a row into some table
> > on each logon, for example.
> >
> > A quick Google search found 6 previous requests for this feature, some
> > of which describe intended use cases:
> >
> > https://www.postgresql.org/message-id/4EBC6852.5030605@fuzzy.cz (2011)
> >
https://www.postgresql.org/message-id/CAHyXU0wrsYShxmwBxZSGYoiBJa%3DgzEJ17iAeRvaf_vA%2BcoH_qA%40mail.gmail.com
> > (2011)
> >
https://www.postgresql.org/message-id/BAY104-W513CF26C0046C9D28747B8D1DD0@phx.gbl
> > (2009, in Spanish)
> >
https://www.postgresql.org/message-id/758d5e7f0803130227m558d32cdl7159bed00d21f084%40mail.gmail.com
> > (2008)
> >
https://www.postgresql.org/message-id/001a01c48077%240b118e60%240200030a%40gendron.ca
> > (2004)
> >
https://www.postgresql.org/message-id/F96SgcOrBLSAQV6uPDV00000a2b@hotmail.com
> > (2000)
> >
>
> Hi all,
>
> I'm sending a new rebased patches and added tests to src/tests/modules as
suggested before.
>

Also added for the next commitfest:

https://commitfest.postgresql.org/15/1318/

Att,

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

  20. Re: [PATCH] A hook for session start

    Nico Williams <nico@cryptonector.com> — 2017-10-05T20:30:06Z

    On Fri, Jul 21, 2017 at 11:10:52PM +0800, Craig Ringer wrote:
> What practical use cases are there for acting post-auth but that can't wait
> until the user tries to do something?

Creating TEMP schema that triggers and functions might need.

Doing CREATE TEMP TABLE IF NOT EXISTS in triggers slows things down.

It'd be super nice if PostgreSQL had some sort of persistent TEMP
schema option, where you can have schema elements that are persistent
in that they're always there, but where the data is all TEMP.  Oracle
has this and they call it GLOBAL TEMP IIRC.  There would be some
caveats, such as not being able to have FKs between these sorts of
persistent temp tables and persistent tables.

In the absence of such a feature, a session hook/trigger is a great
workaround.

> Can a user do anything remotely interesting or useful without hitting
> either ExecutorStart_hook or ProcessUtility_hook? They can parse queries I
> guess but you could just set your hook up in the parser instead. If you
> hook the parser all they can do is open an idle session and sit there...

In any other hook you'd have to check whether the session setup work you
wanted to do has been done.  That could be potentially slow.

I actually have an all SQL implementation of session/begin/commit
triggers.  The session triggers in that implementation only run on the
first DML statement, which could be too late for OP's purpose.

Nico
--

  21. Re: [PATCH] A hook for session start

    Nico Williams <nico@cryptonector.com> — 2017-10-05T20:31:40Z

    On Tue, Aug 01, 2017 at 03:36:23PM -0400, Peter Eisentraut wrote:
> On 7/21/17 13:14, Jim Mlodgenski wrote:
> > When I first saw this thread, my initial thought of a use case is to
> > prepare some key application queries so they are there and ready to go.
> > That would need to be before the ExecutorStart_hook or
> > ProcessUtility_hook if an app would just want to execute the prepared
> > statement.
> 
> Isn't that what the preprepare extension does already?

more generic facility -> more useful

My use case is to pre-create TEMP schema elements that VIEWs, FUNCTIONs,
and TRIGGERs, might need.

Nico
--

  22. Re: [PATCH] A hook for session start

    Pavel Stehule <pavel.stehule@gmail.com> — 2017-10-06T02:52:09Z

    2017-10-05 22:31 GMT+02:00 Nico Williams <nico@cryptonector.com>:

> On Tue, Aug 01, 2017 at 03:36:23PM -0400, Peter Eisentraut wrote:
> > On 7/21/17 13:14, Jim Mlodgenski wrote:
> > > When I first saw this thread, my initial thought of a use case is to
> > > prepare some key application queries so they are there and ready to go.
> > > That would need to be before the ExecutorStart_hook or
> > > ProcessUtility_hook if an app would just want to execute the prepared
> > > statement.
> >
> > Isn't that what the preprepare extension does already?
>
> more generic facility -> more useful
>
> My use case is to pre-create TEMP schema elements that VIEWs, FUNCTIONs,
> and TRIGGERs, might need.
>

It is better to work on GLOBAL TEMP tables.

Current TEMP tables, if you do it for any session has pretty significant
overhead  - with possible risk of performance lost (system catalog bloat).

pretty significant performance issue of my customers are related to temp
tables usage (under high load)

So often creating local temp tables is antipattern (in Postgres)
unfortunately.

I am not sure, if we should to support this case more :( Probably is
better, so it is hard to use local TEMP tables.

Regards

Pavel

>
> Nico
> --
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

  23. Re: [PATCH] A hook for session start

    Craig Ringer <craig@2ndquadrant.com> — 2017-10-06T03:04:38Z

    On 6 October 2017 at 10:52, Pavel Stehule <pavel.stehule@gmail.com> wrote:

> It is better to work on GLOBAL TEMP tables.
>
> Current TEMP tables, if you do it for any session has pretty significant
> overhead  - with possible risk of performance lost (system catalog bloat).
>
> pretty significant performance issue of my customers are related to temp
> tables usage (under high load)

I've seen the same thing too. Especially when combined with logical
decoding, where IIRC we mark transactions as having catalog changes
due to temp tables.

Sometimes the catalog bloat can be truly horrible when a user has
hundreds of plpgsql functions that all like to make temp tables.

-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  24. Re: [PATCH] A hook for session start

    Nico Williams <nico@cryptonector.com> — 2017-10-06T04:48:37Z

    On Fri, Oct 06, 2017 at 04:52:09AM +0200, Pavel Stehule wrote:
> 2017-10-05 22:31 GMT+02:00 Nico Williams <nico@cryptonector.com>:
> > On Tue, Aug 01, 2017 at 03:36:23PM -0400, Peter Eisentraut wrote:
> > > On 7/21/17 13:14, Jim Mlodgenski wrote:
> > > > When I first saw this thread, my initial thought of a use case is to
> > > > prepare some key application queries so they are there and ready to go.
> > > > That would need to be before the ExecutorStart_hook or
> > > > ProcessUtility_hook if an app would just want to execute the prepared
> > > > statement.
> > >
> > > Isn't that what the preprepare extension does already?
> >
> > more generic facility -> more useful
> >
> > My use case is to pre-create TEMP schema elements that VIEWs, FUNCTIONs,
> > and TRIGGERs, might need.
> 
> It is better to work on GLOBAL TEMP tables.

I don't disagree.

In fact, I was scoping out what it might take to do that just yesterday.

I've too thoughts on that: either a new relpersistence kind that is very
similar to persistent, but which always uses temp heaps, or a modifier
for the persistent kind that says to use temp heaps.  Either way it
looks like it should be fairly straightforward (but then, i've only
ever written one thing for PG, earlier this week, the ALWAYS DEFERRED
thing).

> Current TEMP tables, if you do it for any session has pretty significant
> overhead  - with possible risk of performance lost (system catalog bloat).

Because of the DDLs for them?

> So often creating local temp tables is antipattern (in Postgres)
> unfortunately.

I do it plenty, but sometimes I use an UNLOGGED table with a txid column
in the PK set to txid_current(), then I clean up where I can.  It'd be
nice to have COMMIT triggers for cleaning up such rows, among other
things.  I've implemented that using DDL event triggers, but to perform
well it needs to be a native feature.

> I am not sure, if we should to support this case more :( Probably is
> better, so it is hard to use local TEMP tables.

No, I want GLOBAL TEMP tables.

Nico
--

  25. Re: [PATCH] A hook for session start

    Nico Williams <nico@cryptonector.com> — 2017-10-06T04:50:42Z

    On Fri, Oct 06, 2017 at 11:04:38AM +0800, Craig Ringer wrote:
> On 6 October 2017 at 10:52, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 
> > It is better to work on GLOBAL TEMP tables.
> >
> > Current TEMP tables, if you do it for any session has pretty significant
> > overhead  - with possible risk of performance lost (system catalog bloat).
> >
> > pretty significant performance issue of my customers are related to temp
> > tables usage (under high load)
> 
> I've seen the same thing too. Especially when combined with logical
> decoding, where IIRC we mark transactions as having catalog changes
> due to temp tables.
> 
> Sometimes the catalog bloat can be truly horrible when a user has
> hundreds of plpgsql functions that all like to make temp tables.

I agree that we should have GLOBAL TEMP tables, but also we should have
a pg_temp_catalog where all TEMP schema elements go...  (That, I'm sure,
would be a lot of work.)

  26. Re: [PATCH] A hook for session start

    Pavel Stehule <pavel.stehule@gmail.com> — 2017-10-06T16:37:57Z

    2017-10-06 6:48 GMT+02:00 Nico Williams <nico@cryptonector.com>:

> On Fri, Oct 06, 2017 at 04:52:09AM +0200, Pavel Stehule wrote:
> > 2017-10-05 22:31 GMT+02:00 Nico Williams <nico@cryptonector.com>:
> > > On Tue, Aug 01, 2017 at 03:36:23PM -0400, Peter Eisentraut wrote:
> > > > On 7/21/17 13:14, Jim Mlodgenski wrote:
> > > > > When I first saw this thread, my initial thought of a use case is
> to
> > > > > prepare some key application queries so they are there and ready
> to go.
> > > > > That would need to be before the ExecutorStart_hook or
> > > > > ProcessUtility_hook if an app would just want to execute the
> prepared
> > > > > statement.
> > > >
> > > > Isn't that what the preprepare extension does already?
> > >
> > > more generic facility -> more useful
> > >
> > > My use case is to pre-create TEMP schema elements that VIEWs,
> FUNCTIONs,
> > > and TRIGGERs, might need.
> >
> > It is better to work on GLOBAL TEMP tables.
>
> I don't disagree.
>
> In fact, I was scoping out what it might take to do that just yesterday.
>
> I've too thoughts on that: either a new relpersistence kind that is very
> similar to persistent, but which always uses temp heaps, or a modifier
> for the persistent kind that says to use temp heaps.  Either way it
> looks like it should be fairly straightforward (but then, i've only
> ever written one thing for PG, earlier this week, the ALWAYS DEFERRED
> thing).
>
> > Current TEMP tables, if you do it for any session has pretty significant
> > overhead  - with possible risk of performance lost (system catalog
> bloat).
>
> Because of the DDLs for them?
>

yes - pg_attribute, pg_class, pg_stats are bloating - and when these tables
are bloated, then DDL is slow.



> > So often creating local temp tables is antipattern (in Postgres)
> > unfortunately.
>
> I do it plenty, but sometimes I use an UNLOGGED table with a txid column
> in the PK set to txid_current(), then I clean up where I can.  It'd be
> nice to have COMMIT triggers for cleaning up such rows, among other
> things.  I've implemented that using DDL event triggers, but to perform
> well it needs to be a native feature.
>
> > I am not sure, if we should to support this case more :( Probably is
> > better, so it is hard to use local TEMP tables.
>
> No, I want GLOBAL TEMP tables.
>

me too :) - and lot of customer and users.

There is a workaround - you can use a array instead temp tables in 50%. But
it is not a solution in other 50%.

I though about it, but I have other on my top priority. GLOBAL TEMP TABLE
is on 90% unlogged table. But few fields should be session based instead
shared persistent - statistics, rows in pg_class, filenode.

When we talked about this topic, there are two issues:

a) probably not too hard issue - some internal data can be in session sys
cache.

b) the session sys data should be visible on SQL level too (for some tools
and consistency) - it is hard task.

Regards

Pavel


> Nico
> --
>

  27. Re: [PATCH] A hook for session start

    Nico Williams <nico@cryptonector.com> — 2017-10-06T18:39:29Z

    On Fri, Oct 06, 2017 at 06:37:57PM +0200, Pavel Stehule wrote:
> 2017-10-06 6:48 GMT+02:00 Nico Williams <nico@cryptonector.com>:
> > On Fri, Oct 06, 2017 at 04:52:09AM +0200, Pavel Stehule wrote:
> > > Current TEMP tables, if you do it for any session has pretty significant
> > > overhead  - with possible risk of performance lost (system catalog
> > bloat).
> >
> > Because of the DDLs for them?
> 
> yes - pg_attribute, pg_class, pg_stats are bloating - and when these tables
> are bloated, then DDL is slow.

:(

> > No, I want GLOBAL TEMP tables.
> 
> me too :) - and lot of customer and users.

> I though about it, but I have other on my top priority. GLOBAL TEMP TABLE
> is on 90% unlogged table. But few fields should be session based instead
> shared persistent - statistics, rows in pg_class, filenode.

Unlogged tables don't provide isolation between sessions the way temp
tables do, so I don't see the connection.

But the necessary components (temp heaps and such) are all there, and I
suspect a PoC could be done fairly quickly.  But there are some
subtleties like that FKs between GLOBAL TEMP and persistent tables must
not be allowed (in either direction), so a complete implementation will
take significant work.

The work looks like:

 - add syntax (trivial)

 - add new kind of persistence (lots of places to touch, but it's mostly
   mechanical)

 - redirect all references to global temp table contents to temp
   heaps/indexes/whatever

 - add logic to prevent FKs between persistent and global temp tables

 - what else?

> When we talked about this topic, there are two issues:
> 
> a) probably not too hard issue - some internal data can be in session sys
> cache.
> 
> b) the session sys data should be visible on SQL level too (for some tools
> and consistency) - it is hard task.

Can you expand on this?

Nico
--

  28. Re: [PATCH] A hook for session start

    Pavel Stehule <pavel.stehule@gmail.com> — 2017-10-06T18:51:53Z

    2017-10-06 20:39 GMT+02:00 Nico Williams <nico@cryptonector.com>:

> On Fri, Oct 06, 2017 at 06:37:57PM +0200, Pavel Stehule wrote:
> > 2017-10-06 6:48 GMT+02:00 Nico Williams <nico@cryptonector.com>:
> > > On Fri, Oct 06, 2017 at 04:52:09AM +0200, Pavel Stehule wrote:
> > > > Current TEMP tables, if you do it for any session has pretty
> significant
> > > > overhead  - with possible risk of performance lost (system catalog
> > > bloat).
> > >
> > > Because of the DDLs for them?
> >
> > yes - pg_attribute, pg_class, pg_stats are bloating - and when these
> tables
> > are bloated, then DDL is slow.
>
> :(
>
> > > No, I want GLOBAL TEMP tables.
> >
> > me too :) - and lot of customer and users.
>
> > I though about it, but I have other on my top priority. GLOBAL TEMP TABLE
> > is on 90% unlogged table. But few fields should be session based instead
> > shared persistent - statistics, rows in pg_class, filenode.
>
> Unlogged tables don't provide isolation between sessions the way temp
> tables do, so I don't see the connection.
>
> But the necessary components (temp heaps and such) are all there, and I
> suspect a PoC could be done fairly quickly.  But there are some
> subtleties like that FKs between GLOBAL TEMP and persistent tables must
> not be allowed (in either direction), so a complete implementation will
> take significant work.
>
> The work looks like:
>
>  - add syntax (trivial)
>
>  - add new kind of persistence (lots of places to touch, but it's mostly
>    mechanical)
>
>  - redirect all references to global temp table contents to temp
>    heaps/indexes/whatever
>
>  - add logic to prevent FKs between persistent and global temp tables
>
>  - what else?
>
> > When we talked about this topic, there are two issues:
> >
> > a) probably not too hard issue - some internal data can be in session sys
> > cache.
> >
> > b) the session sys data should be visible on SQL level too (for some
> tools
> > and consistency) - it is hard task.
>
> Can you expand on this?
>

If global temporary tables should be effective, then you have not have
modify system catalogue after creating. But lot of processes requires it -
ANALYZE, query planning.

>
> Nico
> --
>

  29. Re: [PATCH] A hook for session start

    Nico Williams <nico@cryptonector.com> — 2017-10-06T19:36:54Z

    On Fri, Oct 06, 2017 at 08:51:53PM +0200, Pavel Stehule wrote:
> 2017-10-06 20:39 GMT+02:00 Nico Williams <nico@cryptonector.com>:
> > On Fri, Oct 06, 2017 at 06:37:57PM +0200, Pavel Stehule wrote:
> > > When we talked about this topic, there are two issues:
> > >
> > > a) probably not too hard issue - some internal data can be in session sys
> > > cache.
> > >
> > > b) the session sys data should be visible on SQL level too (for some
> > tools
> > > and consistency) - it is hard task.
> >
> > Can you expand on this?
> 
> If global temporary tables should be effective, then you have not have
> modify system catalogue after creating. But lot of processes requires it -
> ANALYZE, query planning.

But the nice thing about them is that you need only create them once, so
leave them in the catalog.  Stats about them should not be gathered nor
stored, since they could be different per-session.

  30. Re: [PATCH] A hook for session start

    Pavel Stehule <pavel.stehule@gmail.com> — 2017-10-07T03:44:00Z

    2017-10-06 21:36 GMT+02:00 Nico Williams <nico@cryptonector.com>:

> On Fri, Oct 06, 2017 at 08:51:53PM +0200, Pavel Stehule wrote:
> > 2017-10-06 20:39 GMT+02:00 Nico Williams <nico@cryptonector.com>:
> > > On Fri, Oct 06, 2017 at 06:37:57PM +0200, Pavel Stehule wrote:
> > > > When we talked about this topic, there are two issues:
> > > >
> > > > a) probably not too hard issue - some internal data can be in
> session sys
> > > > cache.
> > > >
> > > > b) the session sys data should be visible on SQL level too (for some
> > > tools
> > > > and consistency) - it is hard task.
> > >
> > > Can you expand on this?
> >
> > If global temporary tables should be effective, then you have not have
> > modify system catalogue after creating. But lot of processes requires it
> -
> > ANALYZE, query planning.
>
> But the nice thing about them is that you need only create them once, so
> leave them in the catalog.  Stats about them should not be gathered nor
> stored, since they could be different per-session.
>

Unfortunately one field from pg_class are not static - reltuples should be
per session.

But it can be moved to different table

Regards

Pavel

  31. Re: [PATCH] A hook for session start

    Nico Williams <nico@cryptonector.com> — 2017-10-07T04:49:28Z

    On Sat, Oct 07, 2017 at 05:44:00AM +0200, Pavel Stehule wrote:
> 2017-10-06 21:36 GMT+02:00 Nico Williams <nico@cryptonector.com>:
> > But the nice thing about them is that you need only create them once, so
> > leave them in the catalog.  Stats about them should not be gathered nor
> > stored, since they could be different per-session.
> 
> Unfortunately one field from pg_class are not static - reltuples should be
> per session.

It's "only an estimate" "used by the query planner".  We could estimate
zero for global temp tables, and the query planner can get the true
value from an internal temp table.

> But it can be moved to different table

That too, if it's OK.

Nico
--

  32. Re: [PATCH] A hook for session start

    Pavel Stehule <pavel.stehule@gmail.com> — 2017-10-07T04:53:31Z

    2017-10-07 6:49 GMT+02:00 Nico Williams <nico@cryptonector.com>:

> On Sat, Oct 07, 2017 at 05:44:00AM +0200, Pavel Stehule wrote:
> > 2017-10-06 21:36 GMT+02:00 Nico Williams <nico@cryptonector.com>:
> > > But the nice thing about them is that you need only create them once,
> so
> > > leave them in the catalog.  Stats about them should not be gathered nor
> > > stored, since they could be different per-session.
> >
> > Unfortunately one field from pg_class are not static - reltuples should
> be
> > per session.
>
> It's "only an estimate" "used by the query planner".  We could estimate
> zero for global temp tables, and the query planner can get the true
> value from an internal temp table.
>

It can be solution.


> > But it can be moved to different table
>
> That too, if it's OK.
>
> Nico
> --
>

  33. Re: [PATCH] A hook for session start

    Robert Haas <robertmhaas@gmail.com> — 2017-10-11T14:28:05Z

    On Fri, Oct 6, 2017 at 3:36 PM, Nico Williams <nico@cryptonector.com> wrote:
>> If global temporary tables should be effective, then you have not have
>> modify system catalogue after creating. But lot of processes requires it -
>> ANALYZE, query planning.
>
> But the nice thing about them is that you need only create them once, so
> leave them in the catalog.  Stats about them should not be gathered nor
> stored, since they could be different per-session.

The topic of this thread seems to have drifted quite far from the
subject line, but here are some links to previous discussions of
global temporary tables.

https://www.postgresql.org/message-id/flat/u2o603c8f071004231952i36642ae6u9d6a7eae6eb6ff32%40mail.gmail.com
https://www.postgresql.org/message-id/AANLkTim=5af41BKFvZ=ofVJ465kxQkJdjHQZUDz3k1d9@mail.gmail.com
https://www.postgresql.org/message-id/flat/CAFj8pRC2h6qhHsFbcE7b_7SagiS6o%3D5J2UvCwCb05Ka1XFv_Ng%40mail.gmail.com
https://www.postgresql.org/message-id/flat/CA%2B0EDdACCx8w4nK-wdj-eodbJn4juChnHoUWVMM3u3uhLVPnJw%40mail.gmail.com

I would strongly recommend reading through those carefully before
undertaking anything here.  This is not a straightforward project...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  34. Re: [PATCH] A hook for session start

    Aleksandr Parfenov <a.parfenov@postgrespro.ru> — 2017-11-02T07:42:50Z

    The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hi,

Unfortunately, patches 0001 and 0002 don't apply to current master.

The new status of this patch is: Waiting on Author

  35. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-02T23:36:45Z

    On Thu, Nov 2, 2017 at 5:42 AM, Aleksandr Parfenov <
a.parfenov@postgrespro.ru> wrote:
>
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:       not tested
> Spec compliant:           not tested
> Documentation:            not tested
>
> Hi,
>
> Unfortunately, patches 0001 and 0002 don't apply to current master.
>
> The new status of this patch is: Waiting on Author
>

Thanks for your review. Rebased versions attached.

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

  36. Re: [PATCH] A hook for session start

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-03T13:19:05Z

    On Thu, Nov 2, 2017 at 11:36 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> On Thu, Nov 2, 2017 at 5:42 AM, Aleksandr Parfenov
> <a.parfenov@postgrespro.ru> wrote:
>> Unfortunately, patches 0001 and 0002 don't apply to current master.
>>
>> The new status of this patch is: Waiting on Author
>
> Thanks for your review. Rebased versions attached.

Looking at this thread, there are clearly arguments in favor of having
a session hook after authentication. One use case mentioned by Robert
is inserting data into a table when a user logs in. I can imagine that
something like that could be applied to a session ending.

     /*
+     * Setup handler to session end hook
+     */
+    if (IsUnderPostmaster)
+        on_proc_exit(do_session_end_hook, 0);
I think that it would be better to place that in ShutdownPostgres.
This way it is possible to take actions before any resource is shut
down.

Passing the database name and user name does not look much useful to
me. You can have access to this data already with CurrentUserId and
MyDatabaseId.
-- 
Michael

  37. Re: [PATCH] A hook for session start

    Aleksandr Parfenov <a.parfenov@postgrespro.ru> — 2017-11-03T13:43:05Z

    README file in patch 0003 is a copy of README from test_pg_dump module
without any changes.

-- 
Aleksandr Parfenov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

  38. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-03T13:55:53Z

    On Fri, Nov 3, 2017 at 11:43 AM, Aleksandr Parfenov <
a.parfenov@postgrespro.ru> wrote:
>
> README file in patch 0003 is a copy of README from test_pg_dump module
> without any changes.
>

Thanks, I'll fix it.

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

  39. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-03T13:55:58Z

    On Fri, Nov 3, 2017 at 11:19 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
>
>      /*
> +     * Setup handler to session end hook
> +     */
> +    if (IsUnderPostmaster)
> +        on_proc_exit(do_session_end_hook, 0);
> I think that it would be better to place that in ShutdownPostgres.
> This way it is possible to take actions before any resource is shut
> down.
>

Hmmm... ok but I have some doubt... ShutdownPostgres make sure to abort any
active transaction (AbortOutOfAnyTransaction) and release all locks
(LockReleaseAll). If we hook session end at this point we'll do that after
this two steps and after that run again... something like:

...
    /* Make sure we've killed any active transaction */
    AbortOutOfAnyTransaction();

    /*
     * User locks are not released by transaction end, so be sure to release
     * them explicitly.
     */
    LockReleaseAll(USER_LOCKMETHOD, true);

    if (session_end_hook) {
        (*session_end_hook) (port->database_name, port->user_name);

        /* Make sure session end hook doesn't leave trash behind */
        AbortOutOfAnyTransaction();
        LockReleaseAll(USER_LOCKMETHOD, true);
    }
...


> Passing the database name and user name does not look much useful to
> me. You can have access to this data already with CurrentUserId and
> MyDatabaseId.
>

This way we don't need to convert oid to names inside hook code.

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

  40. Re: [PATCH] A hook for session start

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-04T03:23:32Z

    On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>> Passing the database name and user name does not look much useful to
>> me. You can have access to this data already with CurrentUserId and
>> MyDatabaseId.
>
> This way we don't need to convert oid to names inside hook code.

Well, arguments of hooks are useful if they are used. Now if I look at
the two examples mainly proposed in this thread, be it in your set of
patches or the possibility to do some SQL transaction, I see nothing
using them. So I'd vote for keeping an interface minimal.
-- 
Michael

  41. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-04T18:14:46Z

    On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
>
> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> >> Passing the database name and user name does not look much useful to
> >> me. You can have access to this data already with CurrentUserId and
> >> MyDatabaseId.
> >
> > This way we don't need to convert oid to names inside hook code.
>
> Well, arguments of hooks are useful if they are used. Now if I look at
> the two examples mainly proposed in this thread, be it in your set of
> patches or the possibility to do some SQL transaction, I see nothing
> using them. So I'd vote for keeping an interface minimal.
>

Maybe the attached patch wit improved test module can illustrate better the
feature.

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

  42. Re: [PATCH] A hook for session start

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-07T03:15:49Z

    On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
>> <fabriziomello@gmail.com> wrote:
>> >> Passing the database name and user name does not look much useful to
>> >> me. You can have access to this data already with CurrentUserId and
>> >> MyDatabaseId.
>> >
>> > This way we don't need to convert oid to names inside hook code.
>>
>> Well, arguments of hooks are useful if they are used. Now if I look at
>> the two examples mainly proposed in this thread, be it in your set of
>> patches or the possibility to do some SQL transaction, I see nothing
>> using them. So I'd vote for keeping an interface minimal.
>>
>
> Maybe the attached patch with improved test module can illustrate better the
> feature.

I was going to to hack something like that. That's interesting for the
use case Robert has mentioned.

Well, in the case of the end session hook, those variables are passed
to the hook by being directly taken from the context from MyProcPort:
+               (*session_end_hook) (MyProcPort->database_name,
MyProcPort->user_name);
In the case of the start hook, those are directly taken from the
command outer caller, but similarly MyProcPort is already set, so
those are strings available (your patch does so in the end session
hook)... Variables in hooks are useful if those are not available
within the memory context, and refer to a specific state where the
hook is called. For example, take the password hook, this uses the
user name and the password because those values are not available
within the session context. The same stands for other hooks as well.
Keeping the interface minimal helps in readability and maintenance.
See for the attached example that can be applied on top of 0003, which
makes use of the session context, the set regression tests does not
pass but this shows how I think those hooks had better be shaped.

+       (*session_end_hook) (MyProcPort->database_name, MyProcPort->user_name);
+
+       /* Make don't leave any active transactions and/or locks behind */
+       AbortOutOfAnyTransaction();
+       LockReleaseAll(USER_LOCKMETHOD, true);
Let's leave this work to people actually implementing the hook contents.
-- 
Michael

  43. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-07T12:58:54Z

    On Tue, Nov 7, 2017 at 1:15 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
>
> On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > On Sat, Nov 4, 2017 at 1:23 AM, Michael Paquier <
michael.paquier@gmail.com>
> > wrote:
> >> On Fri, Nov 3, 2017 at 1:55 PM, Fabrízio de Royes Mello
> >> <fabriziomello@gmail.com> wrote:
> >> >> Passing the database name and user name does not look much useful to
> >> >> me. You can have access to this data already with CurrentUserId and
> >> >> MyDatabaseId.
> >> >
> >> > This way we don't need to convert oid to names inside hook code.
> >>
> >> Well, arguments of hooks are useful if they are used. Now if I look at
> >> the two examples mainly proposed in this thread, be it in your set of
> >> patches or the possibility to do some SQL transaction, I see nothing
> >> using them. So I'd vote for keeping an interface minimal.
> >>
> >
> > Maybe the attached patch with improved test module can illustrate
better the
> > feature.
>
> I was going to to hack something like that. That's interesting for the
> use case Robert has mentioned.
>
> Well, in the case of the end session hook, those variables are passed
> to the hook by being directly taken from the context from MyProcPort:
> +               (*session_end_hook) (MyProcPort->database_name,
> MyProcPort->user_name);
> In the case of the start hook, those are directly taken from the
> command outer caller, but similarly MyProcPort is already set, so
> those are strings available (your patch does so in the end session
> hook)... Variables in hooks are useful if those are not available
> within the memory context, and refer to a specific state where the
> hook is called. For example, take the password hook, this uses the
> user name and the password because those values are not available
> within the session context. The same stands for other hooks as well.
> Keeping the interface minimal helps in readability and maintenance.
> See for the attached example that can be applied on top of 0003, which
> makes use of the session context, the set regression tests does not
> pass but this shows how I think those hooks had better be shaped.
>

Makes sense... fixed.


> +       (*session_end_hook) (MyProcPort->database_name,
MyProcPort->user_name);
> +
> +       /* Make don't leave any active transactions and/or locks behind */
> +       AbortOutOfAnyTransaction();
> +       LockReleaseAll(USER_LOCKMETHOD, true);
> Let's leave this work to people actually implementing the hook contents.
>

Fixed.

Attached new version. I unify all three patches in just one because this is
a small patch and it's most part is test code.

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

  44. Re: [PATCH] A hook for session start

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-08T02:47:40Z

    On Tue, Nov 7, 2017 at 9:58 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> On Tue, Nov 7, 2017 at 1:15 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>> On Sun, Nov 5, 2017 at 3:14 AM, Fabrízio de Royes Mello
>> <fabriziomello@gmail.com> wrote:
>> I was going to to hack something like that. That's interesting for the
>> use case Robert has mentioned.
>>
>> Well, in the case of the end session hook, those variables are passed
>> to the hook by being directly taken from the context from MyProcPort:
>> +               (*session_end_hook) (MyProcPort->database_name,
>> MyProcPort->user_name);
>> In the case of the start hook, those are directly taken from the
>> command outer caller, but similarly MyProcPort is already set, so
>> those are strings available (your patch does so in the end session
>> hook)... Variables in hooks are useful if those are not available
>> within the memory context, and refer to a specific state where the
>> hook is called. For example, take the password hook, this uses the
>> user name and the password because those values are not available
>> within the session context. The same stands for other hooks as well.
>> Keeping the interface minimal helps in readability and maintenance.
>> See for the attached example that can be applied on top of 0003, which
>> makes use of the session context, the set regression tests does not
>> pass but this shows how I think those hooks had better be shaped.
>>
>
> Makes sense... fixed.

Thanks for considering it.

>> +       (*session_end_hook) (MyProcPort->database_name,
>> MyProcPort->user_name);
>> +
>> +       /* Make don't leave any active transactions and/or locks behind */
>> +       AbortOutOfAnyTransaction();
>> +       LockReleaseAll(USER_LOCKMETHOD, true);
>> Let's leave this work to people actually implementing the hook contents.
>>
>
> Fixed.
>
> Attached new version. I unify all three patches in just one because this is
> a small patch and it's most part is test code.

Thanks. This makes sense to me.

+   /* Hook just normal backends */
+   if (session_end_hook && MyBackendId != InvalidBackendId)
+       (*session_end_hook) ();
I have been wondering about the necessity of this restriction.
Couldn't it be useful to just let the people implementing the hook do
any decision-making? Tracking some non-backend shutdowns may be useful
as well for logging purposes.

The patch is beginning to take shape (I really like the test module
you are implementing here!), still needs a bit more work.

+CREATE ROLE session_hook_usr1 LOGIN;
+CREATE ROLE session_hook_usr2 LOGIN;
Roles created during regression tests should be prefixed with regress_.

+   if (prev_session_start_hook)
+       prev_session_start_hook();
+
+   if (session_start_hook_enabled)
+       (void) register_session_hook("START");
Shouldn't both be reversed?

+/* sample sessoin end hook function */
Typo here.

+CREATE DATABASE session_hook_db;
[...]
+   if (!strcmp(dbname, "session_hook_db"))
+   {
Creating a database is usually avoided in sql files as those can be
run on existing servers using installcheck. I am getting that this
restriction is in place so as it is possible to create an initial
connection to the server so as the base table to log the activity is
created. That's a fine assumption to me. Still, I think that the
following changes should be done:
- Let's restrict the logging to a role name instead of a database
name, and let's parametrize it with a setting in the temporary
configuration file. Let's not bother about multiple role support with
a list, for the sake of tests and simplicity only defining one role
looks fine to me. Comments in the code should be clear about the
dependency.
- The GUCs test_session_hooks.start_enabled and
test_session_hooks.end_enabled are actually redundant with the
database restriction (well, role restriction per previous comment), so
let's remove them. Please let's also avoid ALTER SYSTEM calls in tests
as it would impact existing installations with installcheck.

Impossible to tell committer's feeling about this test suite, but my
opinion is to keep it as that's a good template example about what can
be done with those two hooks.
-- 
Michael

  45. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-08T17:42:43Z

    On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
>
> +   /* Hook just normal backends */
> +   if (session_end_hook && MyBackendId != InvalidBackendId)
> +       (*session_end_hook) ();
> I have been wondering about the necessity of this restriction.
> Couldn't it be useful to just let the people implementing the hook do
> any decision-making? Tracking some non-backend shutdowns may be useful
> as well for logging purposes.
>

Also makes sense... I move down this check to hook code.


> The patch is beginning to take shape (I really like the test module
> you are implementing here!), still needs a bit more work.
>

Thanks... and your review is helping a lot!!!


> +CREATE ROLE session_hook_usr1 LOGIN;
> +CREATE ROLE session_hook_usr2 LOGIN;
> Roles created during regression tests should be prefixed with regress_.
>

Fixed.


> +   if (prev_session_start_hook)
> +       prev_session_start_hook();
> +
> +   if (session_start_hook_enabled)
> +       (void) register_session_hook("START");
> Shouldn't both be reversed?
>

Fixed.


> +/* sample sessoin end hook function */
> Typo here.
>

Fixed.


> +CREATE DATABASE session_hook_db;
> [...]
> +   if (!strcmp(dbname, "session_hook_db"))
> +   {
> Creating a database is usually avoided in sql files as those can be
> run on existing servers using installcheck. I am getting that this
> restriction is in place so as it is possible to create an initial
> connection to the server so as the base table to log the activity is
> created. That's a fine assumption to me. Still, I think that the
> following changes should be done:
> - Let's restrict the logging to a role name instead of a database
> name, and let's parametrize it with a setting in the temporary
> configuration file. Let's not bother about multiple role support with
> a list, for the sake of tests and simplicity only defining one role
> looks fine to me. Comments in the code should be clear about the
> dependency.

Makes sense and simplify the test code. Fixed.


> - The GUCs test_session_hooks.start_enabled and
> test_session_hooks.end_enabled are actually redundant with the
> database restriction (well, role restriction per previous comment), so
> let's remove them. Please let's also avoid ALTER SYSTEM calls in tests
> as it would impact existing installations with installcheck.
>

Also simplify the test code. Fixed.


> Impossible to tell committer's feeling about this test suite, but my
> opinion is to keep it as that's a good template example about what can
> be done with those two hooks.
>

+1

Attached new version.

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

  46. Re: [PATCH] A hook for session start

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-09T02:09:20Z

    On Thu, Nov 9, 2017 at 2:42 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>> - Let's restrict the logging to a role name instead of a database
>> name, and let's parametrize it with a setting in the temporary
>> configuration file. Let's not bother about multiple role support with
>> a list, for the sake of tests and simplicity only defining one role
>> looks fine to me. Comments in the code should be clear about the
>> dependency.
>
> Makes sense and simplify the test code. Fixed.

+   if (!strcmp(username, "regress_sess_hook_usr2"))
+   {
+       const char *dbname;
[...]
+++ b/src/test/modules/test_session_hooks/session_hooks.conf
@@ -0,0 +1 @@
+shared_preload_libraries = 'test_session_hooks'
Don't you think that this should be a GUC? My previous comment
outlined that. I won't fight hard on that point in any case, don't
worry. I just want to make things clear :)
-- 
Michael

  47. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-09T17:32:42Z

    On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
>
> On Thu, Nov 9, 2017 at 2:42 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > On Wed, Nov 8, 2017 at 12:47 AM, Michael Paquier <
michael.paquier@gmail.com>
> > wrote:
> >> - Let's restrict the logging to a role name instead of a database
> >> name, and let's parametrize it with a setting in the temporary
> >> configuration file. Let's not bother about multiple role support with
> >> a list, for the sake of tests and simplicity only defining one role
> >> looks fine to me. Comments in the code should be clear about the
> >> dependency.
> >
> > Makes sense and simplify the test code. Fixed.
>
> +   if (!strcmp(username, "regress_sess_hook_usr2"))
> +   {
> +       const char *dbname;
> [...]
> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
> @@ -0,0 +1 @@
> +shared_preload_libraries = 'test_session_hooks'
> Don't you think that this should be a GUC? My previous comment
> outlined that. I won't fight hard on that point in any case, don't
> worry. I just want to make things clear :)
>

Ooops... my fault... fixed!

Thanks again!!

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

  48. Re: [PATCH] A hook for session start

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-09T23:08:50Z

    On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
>> @@ -0,0 +1 @@
>> +shared_preload_libraries = 'test_session_hooks'
>> Don't you think that this should be a GUC? My previous comment
>> outlined that. I won't fight hard on that point in any case, don't
>> worry. I just want to make things clear :)
>
> Ooops... my fault... fixed!
>
> Thanks again!!

+/* GUC variables */
+static char *session_hook_username = NULL;
This causes the module to crash if test_session_hooks.username is not
set. I would recommend just assigning a default value, say "postgres".
-- 
Michael

  49. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-10T15:49:14Z

    On Thu, Nov 9, 2017 at 9:08 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
>
> On Fri, Nov 10, 2017 at 2:32 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > On Thu, Nov 9, 2017 at 12:09 AM, Michael Paquier <
michael.paquier@gmail.com>
> > wrote:
> >> +++ b/src/test/modules/test_session_hooks/session_hooks.conf
> >> @@ -0,0 +1 @@
> >> +shared_preload_libraries = 'test_session_hooks'
> >> Don't you think that this should be a GUC? My previous comment
> >> outlined that. I won't fight hard on that point in any case, don't
> >> worry. I just want to make things clear :)
> >
> > Ooops... my fault... fixed!
> >
> > Thanks again!!
>
> +/* GUC variables */
> +static char *session_hook_username = NULL;
> This causes the module to crash if test_session_hooks.username is not
> set. I would recommend just assigning a default value, say "postgres".
>

Fixed.

New version attached.

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

  50. Re: [PATCH] A hook for session start

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-11T08:48:42Z

    On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> New version attached.

Thanks.

+++ b/src/test/modules/Makefile
          test_extensions \
+         test_session_hooks \
          test_parser
Better if that's in alphabetical order.

That's a nit though, so I am switching the patch as ready for committer.
-- 
Michael

  51. Re: [PATCH] A hook for session start

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-11T12:35:35Z

    On Sat, Nov 11, 2017 at 6:48 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:
>
> On Sat, Nov 11, 2017 at 12:49 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > New version attached.
>
> Thanks.
>
> +++ b/src/test/modules/Makefile
>           test_extensions \
> +         test_session_hooks \
>           test_parser
> Better if that's in alphabetical order.
>

Fixed.


> That's a nit though, so I am switching the patch as ready for committer.
>

Thank you so much for your review.

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

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

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-17T03:38:01Z

    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.1510800078@sss.pgh.pa.us
https://www.postgresql.org/message-id/flat/20171116121823.GI4628%40tamriel.snowman.net#20171116121823.GI4628@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

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

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2017-11-17T05:33:11Z

    On Fri, Nov 17, 2017 at 9:08 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> 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.1510800078@sss.pgh.pa.us
> https://www.postgresql.org/message-id/flat/20171116121823.GI4628%40tamriel.snowman.net#20171116121823.GI4628@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

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

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-17T12:41:46Z

    On Fri, Nov 17, 2017 at 3:33 AM, Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:
>
> On Fri, Nov 17, 2017 at 9:08 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> 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

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

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-11-19T21:38:47Z

    
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.1510800078@sss.pgh.pa.us
> https://www.postgresql.org/message-id/flat/20171116121823.GI4628%40tamriel.snowman.net#20171116121823.GI4628@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 Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-11-19T21:49:49Z

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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

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

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2017-11-19T22:56:29Z

    
On 11/19/2017 04:49 PM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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 Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-20T02:08:01Z

    On Mon, Nov 20, 2017 at 7:56 AM, Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
> On 11/19/2017 04:49 PM, Tom Lane wrote:
>> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> 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.

I think that this is close to the concept of BackendType in pgstat.h.

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

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-11-20T03:11:08Z

    Michael Paquier <michael.paquier@gmail.com> 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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-20T03:27:31Z

    On Mon, Nov 20, 2017 at 12:11 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-11-20T03:38:45Z

    Michael Paquier <michael.paquier@gmail.com> writes:
> On Mon, Nov 20, 2017 at 12:11 PM, Tom Lane <tgl@sss.pgh.pa.us> 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

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

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-20T12:55:18Z

    On Mon, Nov 20, 2017 at 1:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Michael Paquier <michael.paquier@gmail.com> writes:
> > On Mon, Nov 20, 2017 at 12:11 PM, Tom Lane <tgl@sss.pgh.pa.us> 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.
>

Something like it ??

src/include/utils/proctype.h

typedef enum
{
    ClientBackendProcess = -1,
    CheckerProcess = 0,
    BootstrapProcess,
    StartupProcess,
    BgWriterProcess,
    CheckpointerProcess,
    WalWriterProcess,
    WalReceiverProcess,
    WalSenderProcess,
    AutovacuumLauncherProcess,
    AutovacuumWorkerProcess,
    BackgroundWorkerProcess,
    StatsCollectorProcess,
    ArchiverProcess,
    LogicalReplicationLauncherProcess,
    LogicalReplicationWorkerProcess,
    LoggerProcess,

    NUM_PROCTYPES            /* Must be last! */
} ProcType;

extern ProcType MyProcType;


Should "MyProcType" be defined in globals.c, right?



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

Do we need the notion of process types groups? If no the idea of generalize
AuxProcType seems better.

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-11-20T16:56:28Z

    =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello@gmail.com> 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

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

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2017-11-20T19:09:54Z

    On Mon, Nov 20, 2017 at 2:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello@gmail.com> 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

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-28T01:59:06Z

    On Tue, Nov 21, 2017 at 4:09 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> 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