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

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>

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

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

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>

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>

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

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

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

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

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

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

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


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

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


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

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


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

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


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

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


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

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


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

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.


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

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


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

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


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

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

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

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


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

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


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

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


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

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

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

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


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

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


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

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


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

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


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

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

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

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

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

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

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

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

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

Fabrízio 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            /*
Mustbe 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

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