Subject: pgsql: Add support event triggers on authenticated login

From: Alexander Korotkov <[email-masked]>
Date: Mon, 16 Oct 2023 00:18:33 +0000

Add support event triggers on authenticated login

This commit introduces trigger on login event, allowing to fire some actions
right on the user connection.  This can be useful for logging or connection
check purposes as well as for some personalization of environment.  Usage
details are described in the documentation included, but shortly usage is
the same as for other triggers: create function returning event_trigger and
then create event trigger on login event.

In order to prevent the connection time overhead when there are no triggers
the commit introduces pg_database.dathasloginevt flag, which indicates database
has active login triggers.  This flag is set by CREATE/ALTER EVENT TRIGGER
command, and unset at connection time when no active triggers found.

Author: Konstantin Knizhnik, Mikhail Gribkov
Discussion: https://postgr.es/m/0d46d29f-4558-3af9-9c85-7774e14a7709%40postgrespro.ru
Reviewed-by: Pavel Stehule, Takayuki Tsunakawa, Greg Nancarrow, Ivan Panchenko
Reviewed-by: Daniel Gustafsson, Teodor Sigaev, Robert Haas, Andres Freund
Reviewed-by: Tom Lane, Andrey Sokolov, Zhihong Yu, Sergey Shinderuk
Reviewed-by: Gregory Stark, Nikita Malakhov, Ted Yu

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/e83d1b0c40ccda8955f1245087f0697652c4df86

Modified Files
--------------
doc/src/sgml/bki.sgml                          |   2 +-
doc/src/sgml/catalogs.sgml                     |  13 ++
doc/src/sgml/ecpg.sgml                         |   2 +
doc/src/sgml/event-trigger.sgml                |  94 ++++++++++++
src/backend/commands/dbcommands.c              |  17 ++-
src/backend/commands/event_trigger.c           | 179 +++++++++++++++++++++--
src/backend/storage/lmgr/lmgr.c                |  38 +++++
src/backend/tcop/postgres.c                    |   4 +
src/backend/utils/cache/evtcache.c             |   2 +
src/backend/utils/init/globals.c               |   2 +
src/backend/utils/init/postinit.c              |   1 +
src/bin/pg_dump/pg_dump.c                      |   5 +
src/bin/psql/tab-complete.c                    |   4 +-
src/include/catalog/catversion.h               |   2 +-
src/include/catalog/pg_database.dat            |   2 +-
src/include/catalog/pg_database.h              |   3 +
src/include/commands/event_trigger.h           |   1 +
src/include/miscadmin.h                        |   2 +
src/include/storage/lmgr.h                     |   2 +
src/include/tcop/cmdtaglist.h                  |   1 +
src/include/utils/evtcache.h                   |   3 +-
src/test/authentication/t/005_login_trigger.pl | 189 +++++++++++++++++++++++++
src/test/recovery/t/001_stream_rep.pl          |  26 ++++
src/test/regress/expected/event_trigger.out    |  45 ++++++
src/test/regress/sql/event_trigger.sql         |  26 ++++
25 files changed, 644 insertions(+), 21 deletions(-)

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 12 Jan 2024 07:28:26 +0530

> Add support event triggers on authenticated login

Hi, I'm seeing a compiler warning with CFLAGS -O3 but not with -O2.

In file included from dbcommands.c:20:
dbcommands.c: In function ‘createdb’:
../../../src/include/postgres.h:104:16: warning: ‘src_hasloginevt’ may
be used uninitialized in this function [-Wmaybe-uninitialized]
  104 |         return (Datum) (X ? 1 : 0);
      |                ^~~~~~~~~~~~~~~~~~~
dbcommands.c:683:25: note: ‘src_hasloginevt’ was declared here
  683 |         bool            src_hasloginevt;
      |                         ^~~~~~~~~~~~~~~

The configure command I used is ./configure --prefix=$PWD/inst/
CFLAGS="-ggdb3 -O3" > install.log && make -j 8 install > install.log
2>&1 &:

CONFIGURE =  '--prefix=/home/ubuntu/postgres/inst/' 'CFLAGS=-ggdb3 -O3'
CC = gcc
CPPFLAGS = -D_GNU_SOURCE
CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3
-Wcast-function-type -Wshadow=compatible-local -Wformat-security
-fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -ggdb3 -O3
CFLAGS_SL = -fPIC

The compiler version is:
gcc --version
gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Tom Lane <[email-masked]>
Date: Thu, 11 Jan 2024 21:55:19 -0500

> be used uninitialized in this function [-Wmaybe-uninitialized]

Hmm, I also see that at -O3 (not at -O2) when using Fedora 39's
gcc 13.2.1, but *not* when using RHEL8's gcc 8.5.0.

I'm not sure how excited I am about curing that, though, because gcc
13.2.1 spews several other totally baseless warnings (see attached).
Some of them match up with warnings we're seeing on buildfarm member
serinus, which I seem to recall that Andres had tracked to a known gcc
bug.

			regards, tom lane

In file included from dbcommands.c:20:
In function 'BoolGetDatum',
    inlined from 'createdb' at dbcommands.c:1379:52:
../../../src/include/postgres.h:104:16: warning: 'src_hasloginevt' may be used uninitialized [-Wmaybe-uninitialized]
  104 |         return (Datum) (X ? 1 : 0);
      |                ^~~~~~~~~~~~~~~~~~~
dbcommands.c: In function 'createdb':
dbcommands.c:683:25: note: 'src_hasloginevt' was declared here
  683 |         bool            src_hasloginevt;
      |                         ^~~~~~~~~~~~~~~
In file included from ../../../../src/include/executor/instrument.h:16,
                 from pgstat_io.c:19:
pgstat_io.c: In function 'pgstat_count_io_op_time':
pgstat_io.c:149:60: warning: array subscript 2 is above array bounds of 'instr_time[2][4][8]' [-Warray-bounds=]
  149 |                 INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op],
      |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../../../../src/include/portability/instr_time.h:179:11: note: in definition of macro 'INSTR_TIME_ADD'
  179 |         ((x).ticks += (y).ticks)
      |           ^
pgstat_io.c:27:25: note: while referencing 'pending_times'
   27 |         instr_time      pending_times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
      |                         ^~~~~~~~~~~~~
pgstat_io.c:149:60: warning: array subscript 2 is above array bounds of 'instr_time[2][4][8]' [-Warray-bounds=]
  149 |                 INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op],
      |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../../../../src/include/portability/instr_time.h:179:11: note: in definition of macro 'INSTR_TIME_ADD'
  179 |         ((x).ticks += (y).ticks)
      |           ^
pgstat_io.c:27:25: note: while referencing 'pending_times'
   27 |         instr_time      pending_times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
      |                         ^~~~~~~~~~~~~
pgstat_io.c:149:60: warning: array subscript 2 is above array bounds of 'instr_time[2][4][8]' [-Warray-bounds=]
  149 |                 INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op],
      |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../../../../src/include/portability/instr_time.h:179:11: note: in definition of macro 'INSTR_TIME_ADD'
  179 |         ((x).ticks += (y).ticks)
      |           ^
pgstat_io.c:27:25: note: while referencing 'pending_times'
   27 |         instr_time      pending_times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
      |                         ^~~~~~~~~~~~~
pgstat_io.c:149:60: warning: array subscript 2 is above array bounds of 'instr_time[2][4][8]' [-Warray-bounds=]
  149 |                 INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op],
      |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../../../../src/include/portability/instr_time.h:179:11: note: in definition of macro 'INSTR_TIME_ADD'
  179 |         ((x).ticks += (y).ticks)
      |           ^
pgstat_io.c:27:25: note: while referencing 'pending_times'
   27 |         instr_time      pending_times[IOOBJECT_NUM_TYPES][IOCONTEXT_NUM_TYPES][IOOP_NUM_TYPES];
      |                         ^~~~~~~~~~~~~
In file included from ../../../../src/include/access/htup_details.h:22,
                 from pl_exec.c:21:
In function 'assign_simple_var',
    inlined from 'exec_set_found' at pl_exec.c:8349:2:
../../../../src/include/varatt.h:230:36: warning: array subscript 0 is outside array bounds of 'char[0]' [-Warray-bounds=]
  230 |         (((varattrib_1b_e *) (PTR))->va_tag)
      |                                    ^
../../../../src/include/varatt.h:94:12: note: in definition of macro 'VARTAG_IS_EXPANDED'
   94 |         (((tag) & ~1) == VARTAG_EXPANDED_RO)
      |            ^~~
../../../../src/include/varatt.h:284:57: note: in expansion of macro 'VARTAG_1B_E'
  284 | #define VARTAG_EXTERNAL(PTR)                            VARTAG_1B_E(PTR)
      |                                                         ^~~~~~~~~~~
../../../../src/include/varatt.h:301:57: note: in expansion of macro 'VARTAG_EXTERNAL'
  301 |         (VARATT_IS_EXTERNAL(PTR) && !VARTAG_IS_EXPANDED(VARTAG_EXTERNAL(PTR)))
      |                                                         ^~~~~~~~~~~~~~~
pl_exec.c:8537:17: note: in expansion of macro 'VARATT_IS_EXTERNAL_NON_EXPANDED'
 8537 |                 VARATT_IS_EXTERNAL_NON_EXPANDED(DatumGetPointer(newvalue)))
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'exec_set_found':
cc1: note: source object is likely at address zero

---

From: Andres Freund <[email-masked]>
Date: Wed, 7 Feb 2024 12:31:38 -0800

Hi,

> gcc 13.2.1, but *not* when using RHEL8's gcc 8.5.0.

It's visible here with gcc >= 10. That's enough versions that I think we
should care.  Interestingly enough, it seems to have recently have gotten
fixed in gcc master (14 to be).

> serinus, which I seem to recall that Andres had tracked to a known gcc bug.

Some, but I don't think all.

>   149 |                 INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op],

Huh, I don't see that with any version of gcc I tried.

>   284 | #define VARTAG_EXTERNAL(PTR)                            VARTAG_1B_E(PTR)

This I see.  If I hint to the compiler that var->datatype->typlen != 1 when
called from exec_set_found(), the warning vanishes. E.g. with

	if (var->datatype->typlen == -1)
		__builtin_unreachable();

I see one more warning:

[1390/2375 42  58%] Compiling C object src/backend/postgres_lib.a.p/utils_adt_jsonb_util.c.o
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c: In function 'compareJsonbContainers':
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c:296:34: warning: 'va.type' may be used uninitialized [-Wmaybe-uninitialized]
  296 |                         res = (va.type > vb.type) ? 1 : -1;
      |                                ~~^~~~~
../../../../../home/andres/src/postgresql/src/backend/utils/adt/jsonb_util.c:204:33: note: 'va' declared here
  204 |                 JsonbValue      va,
      |                                 ^~

I can't really blame the compiler here.  There's a fairly lengthy comment
explaining that va.type/vb.type are set, and it took me a while to understand:

			/*
			 * It's safe to assume that the types differed, and that the va
			 * and vb values passed were set.
			 *
			 * If the two values were of the same container type, then there'd
			 * have been a chance to observe the variation in the number of
			 * elements/pairs (when processing WJB_BEGIN_OBJECT, say). They're
			 * either two heterogeneously-typed containers, or a container and
			 * some scalar type.
			 *
			 * We don't have to consider the WJB_END_ARRAY and WJB_END_OBJECT
			 * cases here, because we would have seen the corresponding
			 * WJB_BEGIN_ARRAY and WJB_BEGIN_OBJECT tokens first, and
			 * concluded that they don't match.
			 */

It's not surprising that the compiler can't understand that you can't get
ra = WJB_DONE, rb = WJB_END_ARRAY or such.

Greetings,

Andres Freund

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 8 Feb 2024 22:00:56 +0200

> fixed in gcc master (14 to be).

I managed to reproduce this warning locally.  Fixed.

------
Regards,
Alexander Korotkov