Wrong buffer limits check

[Mikhail Gribkov]

Hi hackers,

I have tried to analyse Postgres code with Svace static analyzer [1] and
found something I think is a real bug.

In pgp-decrypt.c, in prefix_init function the following check:
if (len > sizeof(tmpbuf))

seem to be erroneous and should really look this way:
if (len > PGP_MAX_BLOCK)

Otherwise the below checks in this line could lead to buffer overflows:
if (buf[len - 2] != buf[len] || buf[len - 1] != buf[len + 1])

This is because buf will point to tmpbuf, while tmpbuf have a size of
PGP_MAX_BLOCK + 2.

What do you think? The proposed patch towarts the current master branch is
attached.

[1] - https://svace.pages.ispras.ru/svace-website/en/

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick


v001-Fix_buffer_len_check.patch
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Alexander,

>> Thank you for catching it.  Please, post this.

Just for a more complete picture of the final state here.
I have posted the described fix (for avoiding race condition in the tests)
separately:
https://commitfest.postgresql.org/45/4616/

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com



>

Avoid race condition for event_triggers regress test

[Mikhail Gribkov]

Hi hackers,

After committing the on-login trigger
(e83d1b0c40ccda8955f1245087f0697652c4df86) the event_trigger regress test
became sensible to any other parallel tests, not only DDL. Thus it should
be placed in a separate parallel schedule group.

The current problem is that a race condition may occur on some systems,
when oidjoins test starts a moment later than normally and affects logins
count for on-login trigger test. The problem is quite a rare one and I only
faced it once. But rare or not - the problem is a problem and it should be
addressed.

Such race condition can be simulated by adding "select pg_sleep(2);" and
"\c" at the very beginning of oidjoins.sql and adding "select pg_sleep(5);"
after creation of the login trigger in event_trigger.sql.
The resulting symptoms are quite recognizable: regression.diffs file will
contain unexpected welcome message for oidjoins test and unexpectedly
increased result of "SELECT COUNT(*) FROM user_logins;" for event_triggers
test. (These are accompanied with the expected responses to the newly added
commands of course)

To get rid of the unexpected results the oidjoins and event_triggers tests
should be splitted into separate paralell schedule groups. This is exactly
what the proposed (attached) patch is doing.

What do you think?
--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com


v001_avoid_race_condition_for_event_trigger_test.patch
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Alexander,

Sorry for my long offline and thanks for the activity. So should we close
the patch on the commitfest page now?

By the way I had one more issue with the login trigger tests (quite a rare
one though). A race condition may occur on some systems, when oidjoins test
starts a moment later than normally and affects logins count for on-login
trigger test. Thus I had to split event_trigger and oidjoins tests into
separate parallel groups. I'll post this as an independent patch then.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Mon, Oct 16, 2023 at 4:05 AM Alexander Korotkov 
wrote:

> On Mon, Oct 16, 2023 at 4:00 AM Michael Paquier 
> wrote:
> > On Mon, Oct 16, 2023 at 02:47:03AM +0300, Alexander Korotkov wrote:
> > > The attached revision fixes test failures spotted by
> > > commitfest.cputube.org.  Also, perl scripts passed perltidy.
> >
> > Still you've missed a few things.  At quick glance:
> > - The code indentation was off a bit in event_trigger.c.
> > - 005_login_trigger.pl fails if the code is compiled with
> > ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS because a WARNING is
> > reported in test "create tmp objects: err equals".
> > - 005_sspi.pl is older than the new test 005_login_trigger.pl, could
> > you rename it with a different number?
>
> You are very fast and sharp eye!
> Thank you for fixing the indentation.  I just pushed fixes for the rest.
>
> --
> Regards,
> Alexander Korotkov
>

Re: Permute underscore separated components of columns before fuzzy matching

[Mikhail Gribkov]

Hello Arne,

yep, now the warnings have gone. And I must thank you for quite a fun time
I had here testing your patch :) I tried even some weird combinations like
this:
postgres=# create table t("_ __ ___" int);
CREATE TABLE
postgres=# select "__ _ ___" from t;
ERROR:  column "__ _ ___" does not exist
LINE 1: select "__ _ ___" from t;
   ^
HINT:  Perhaps you meant to reference the column "t._ __ ___".
postgres=# select "___ __ _" from t;
ERROR:  column "___ __ _" does not exist
LINE 1: select "___ __ _" from t;
   ^
HINT:  Perhaps you meant to reference the column "t._ __ ___".
postgres=#

... and it still worked fine.
Honestly I'm not entirely sure fixing only two switched words is worth the
effort, but the declared goal is clearly achieved.

I think the patch is good to go, although you need to fix code formatting.
At least the char*-definition and opening "{" brackets are conspicuous.
Maybe there are more: it is worth running pgindend tool.

And it would be much more convenient to work with your patch if every next
version file will have a unique name (maybe something like "_v2", "_v3"
etc. suffixes)

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
<http://www.flickr.com/photos/youzhick/albums>*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Mon, Jul 17, 2023 at 1:42 AM Arne Roland  wrote:

> Hello Mikhail,
>
> I'm sorry. Please try attached patch instead.
>
> Thank you for having a look!
>
> Regards
> Arne
>
> --
> *From:* Mikhail Gribkov 
> *Sent:* Thursday, July 6, 2023 13:31
> *To:* Arne Roland 
> *Cc:* Pg Hackers 
> *Subject:* Re: Permute underscore separated components of columns before
> fuzzy matching
>
> Hello Arne,
>
> The goal of supporting words-switching hints sounds interesting and I've
> tried to apply your patch.
> The patch was applied smoothly to the latest master and check-world
> reported no problems. Although I had problems after trying to test the new
> functionality.
>
> I tried to simply mix words in pg_stat_activity.wait_event_type:
>
> postgres=# select wait_type_event from pg_stat_activity ;
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
> in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
> 2023-07-06 

Re: Permute underscore separated components of columns before fuzzy matching

[Mikhail Gribkov]

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

Hello Arne,

The goal of supporting words-switching hints sounds interesting and I've tried 
to apply your patch.
The patch was applied smoothly to the latest master and check-world reported no 
problems. Although I had problems after trying to test the new functionality.

I tried to simply mix words in pg_stat_activity.wait_event_type:

postgres=# select wait_type_event from pg_stat_activity ;
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end in 
MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] ERROR:  column "wait_type_event" does not 
exist at character 8
2023-07-06 14:12:35.968 MSK [1480] HINT:  Perhaps you meant to reference the 
column "pg_stat_activity.wait_event_type".
2023-07-06 14:12:35.968 MSK [1480] STATEMENT:  select wait_type_event from 
pg_stat_activity ;
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
ERROR:  column "wait_type_event" does not 

Re: Permute underscore separated components of columns before fuzzy matching

[Mikhail Gribkov]

Hello Arne,

The goal of supporting words-switching hints sounds interesting and I've
tried to apply your patch.
The patch was applied smoothly to the latest master and check-world
reported no problems. Although I had problems after trying to test the new
functionality.

I tried to simply mix words in pg_stat_activity.wait_event_type:

postgres=# select wait_type_event from pg_stat_activity ;
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] WARNING:  detected write past chunk end
in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
2023-07-06 14:12:35.968 MSK [1480] ERROR:  column "wait_type_event" does
not exist at character 8
2023-07-06 14:12:35.968 MSK [1480] HINT:  Perhaps you meant to reference
the column "pg_stat_activity.wait_event_type".
2023-07-06 14:12:35.968 MSK [1480] STATEMENT:  select wait_type_event from
pg_stat_activity ;
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
WARNING:  detected write past chunk end in MessageContext 0x559d668aaf30
ERROR:  column "wait_type_event" does not exist
LINE 1: select wait_type_event from pg_stat_activity ;
   ^
HINT:  Perhaps you meant to reference the column
"pg_stat_activity.wait_event_type".
postgres=#

So the desired hint is really there, but thgether with looots of warnings.
For sure these 

Re: Fixing tab-complete for dollar-names

[Mikhail Gribkov]

Hi hackers,

As not much preliminary interest seem to be here, I'm sending the patch to
the upcoming commitfest

--
 best regards,
Mikhail A. Gribkov


On Sat, Jun 17, 2023 at 12:51 AM Mikhail Gribkov  wrote:

> Hi hackers,
>
> In modern versions of Postgres the dollar sign is a totally legal
> character for identifiers (except for the first character), but
> tab-complete do not treat such identifiers well.
> For example if one try to create an Oracle-style view like this:
>
> create view v$activity as select * from pg_stat_activity;
>
> , he will get a normally functioning view, but psql tab-complete will not
> help him. Type "v", "v$" or "v$act" and press  - nothing will be
> suggested.
>
> Attached is a small patch fixing this problem.
> Honestly I'm a little surprised that this was not done before. Maybe,
> there are some special considerations I am not aware of, and the patch will
> break something?
> What would you say?
> --
>  best regards,
> Mikhail A. Gribkov
>

Fixing tab-complete for dollar-names

[Mikhail Gribkov]

Hi hackers,

In modern versions of Postgres the dollar sign is a totally legal character
for identifiers (except for the first character), but tab-complete do not
treat such identifiers well.
For example if one try to create an Oracle-style view like this:

create view v$activity as select * from pg_stat_activity;

, he will get a normally functioning view, but psql tab-complete will not
help him. Type "v", "v$" or "v$act" and press  - nothing will be
suggested.

Attached is a small patch fixing this problem.
Honestly I'm a little surprised that this was not done before. Maybe, there
are some special considerations I am not aware of, and the patch will break
something?
What would you say?
--
 best regards,
Mikhail A. Gribkov


v001_fix_dollar_names_tab_complete.patch
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi hackers,

The attached v40 patch is a fresh rebase on master branch to actualize the
state before the upcoming commitfest.
Nothing has changed beyond rebasing.

And just for convenience, here is a link to the exact message of the thread
describing the current approach:
https://www.postgresql.org/message-id/CAMEv5_vg4aJOoUC74XJm%2B5B7%2BTF1nT-Yhtg%2BawtBOESXG5Grfg%40mail.gmail.com

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
<http://www.flickr.com/photos/youzhick/albums>*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Wed, Mar 22, 2023 at 10:38 PM Daniel Gustafsson  wrote:

> > On 22 Mar 2023, at 18:54, Robert Haas  wrote:
>
> > Basically, I think 0001 is a good idea -- I'm much more nervous about
> > 0002. I think we should get 0001 polished up and committed.
>
> Correct me if I'm wrong, but I believe you commented on v27-0001 of the
> login
> event trigger patch series?  Sorry about the confusion if so, this is a
> very
> old and lengthy thread with many twists and turns.  This series was closed
> downthread when the original authors of login EVT left, and the 0001 GUC
> patch
> extracted into its own thread.  That patch now lives at:
>
> https://commitfest.postgresql.org/42/4013/
>
> This thread was then later revived by Mikhail Gribkov but without 0001
> instead
> referring to the above patch for that part.
>
> The new patch for 0001 is quite different, and I welcome your eyes on that
> since I agree with you that it would be a good idea.
>
> --
> Daniel Gustafsson
>
>
>
>


v40-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Gregory,

Thanks for the note. The problem was that the patch was not aware of
yesterday Tom Lane's changes in the test.
It's fixed now: the attached v39 patch contains the updated version along
with the freshest rebase on master branch.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Wed, Mar 15, 2023 at 8:45 PM Gregory Stark (as CFM) 
wrote:

> It looks like the patch is failing a test by not dumping
> login_event_trigger_func? I'm guessing there's a race condition in the
> test but I don't know. I also don't see the tmp_test_jI6t output file
> being preserved in the artifacts anywhere :(
>
> https://cirrus-ci.com/task/6391161871400960?logs=test_world#L2671
>
>
> [16:16:48.594] # Looks like you failed 1 test of 10350.
>
>
> # Running: pg_dump --no-sync
>
> --file=/tmp/cirrus-ci-build/src/bin/pg_dump/tmp_check/tmp_test_jI6t/only_dump_measurement.sql
> --table-and-children=dump_test.measurement --lock-wait-timeout=18
> postgres
> [16:16:27.027](0.154s) ok 6765 - only_dump_measurement: pg_dump runs
> .
> [16:16:27.035](0.000s) not ok 6870 - only_dump_measurement: should
> dump CREATE FUNCTION dump_test.login_event_trigger_func
> [16:16:27.035](0.000s)
> [16:16:27.035](0.000s) #   Failed test 'only_dump_measurement: should
> dump CREATE FUNCTION dump_test.login_event_trigger_func'
> #   at t/002_pg_dump.pl line 4761.
> .
> [16:16:48.594] +++ tap check in src/bin/pg_dump +++
> [16:16:48.594] [16:16:05] t/001_basic.pl  ok 612 ms (
> 0.01 usr 0.00 sys + 0.24 cusr 0.26 csys = 0.51 CPU)
> [16:16:48.594]
> [16:16:48.594] # Failed test 'only_dump_measurement: should dump
> CREATE FUNCTION dump_test.login_event_trigger_func'
> [16:16:48.594] # at t/002_pg_dump.pl line 4761.
> [16:16:48.594] # Review only_dump_measurement results in
> /tmp/cirrus-ci-build/src/bin/pg_dump/tmp_check/tmp_test_jI6t
>


v39-On_client_login_event_trigger.patch
Description: Binary data

Re: GUC for temporarily disabling event triggers

[Mikhail Gribkov]

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

I like it now.

* The patch does what it intends to do;
* The implementation way is clear;
* All test are passed;
* No additional problems catched - at least by my eyes;

I think it can be marked as Ready for Committer

N.B. In fact I've encountered couple of failed tests during installcheck-world, 
although the same fails are there even for master branch. Thus doesn't seem to 
be this patch issue.

The new status of this patch is: Ready for Committer

Re: On login trigger: take three

[Mikhail Gribkov]

Hi hackers,

The attached v38 patch is a fresh rebase on master branch.
Nothing has changed beyond rebasing.

And just for convenience, here is a link to the exact message of the thread
describing the current approach:
https://www.postgresql.org/message-id/CAMEv5_vg4aJOoUC74XJm%2B5B7%2BTF1nT-Yhtg%2BawtBOESXG5Grfg%40mail.gmail.com

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick


> Thank you
>
> marked as ready for committer
>
> Regards
>
> Pavel
>
>
>>


v38-On_client_login_event_trigger.patch
Description: Binary data

Re: [PATCH] psql: Add tab-complete for optional view parameters

[Mikhail Gribkov]

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

Hi Christoph,

The patch have a potential, although I have to agree with Jim Jones, it 
obviously have a problem with "alter view  set" handling.

First of all user can notice, that SET and RESET alternatives are proposed in 
an absolutely equivalent way:
postgres=# alter view VVV 
ALTER COLUMN  OWNER TO  RENAMERESET (   SET ( SET SCHEMA

But completion of a parentheses differs fore these alternatives:

postgres=# alter view VVV reset -> completes with "(" -> 
CHECK_OPTION  SECURITY_BARRIER  SECURITY_INVOKER

postgres=# alter view VVV set -> completes with a single spase -> 
Display all 188 possibilities? (y or n)
(and these 188 possibilities do not contain "(")

The probmen is obviously in the newly added second line of the following clause:
COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
  "SET SCHEMA", "SET (", "RESET (");

"set schema" and "set (" alternatives are competing, while completion of the 
common part "set" leads to a string composition which does not have the 
check branch (Matches("ALTER", "VIEW", MatchAny, "SET")).

I think it may worth looking at "alter materialized view"  completion tree and 
making "alter view" the same way.

The new status of this patch is: Waiting on Author

Re: GUC for temporarily disabling event triggers

[Mikhail Gribkov]

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

Hi Daniel,

I have reviewed the patch and I liked it (well I did liked it already since it 
was a part of login trigger patch previously). 
All tests are passed and the manual experiments with all types of event 
triggers are also passed.

Everything is fine and I think It can be marked as Ready for Committer, 
although I have one final question.
There is a complete framework for disabling various types of the event triggers 
separately, but, the list of valid GUC values only include 'all' and 'none'. 
Why not adding support for all the event trigger types separately? Everything 
is already there in the patch; the only thing needed is expanding couple of 
enums. It's cheap in terms of code size and even cheaper in terms of 
performance. And moreover - it would be a good example for anyone adding new 
trigger types.

The new status of this patch is: Waiting on Author

Re: On login trigger: take three

[Mikhail Gribkov]

  Hi Pavel,

On Mon, Jan 16, 2023 at 9:10 AM Pavel Stehule 
wrote:

>
>
> ne 15. 1. 2023 v 7:32 odesílatel Pavel Stehule 
> napsal:
>
>> Hi
>>
>>
>>> On Thu, Jan 12, 2023 at 9:51 AM Pavel Stehule 
>>> wrote:
>>>
 Hi

 I checked this patch and it looks well. All tests passed. Together with
 https://commitfest.postgresql.org/41/4013/ it can be a good feature.

 I re-tested impact on performance and for the worst case looks like
 less than 1% (0.8%). I think it is acceptable. Tested pgbench scenario
 "SELECT 1"

 pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres

 733 tps (master), 727 tps (patched).

 I think raising an exception inside should be better tested - not it is
 only in 001_stream_rep.pl - generally more tests are welcome - there
 are no tested handling exceptions.

>>>
>> Thank you
>>
>> check-world passed without problems
>> build doc passed without problems
>> I think so tests are now enough
>>
>> I'll mark this patch as ready for committer
>>
>
> Unfortunately, I  forgot one important point. There are not any tests
> related to backup.
>
> I miss pg_dump related tests.
>
> I mark this patch as waiting on the author.
>
>

Thanks for noticing this.
I have added sections to pg_dump tests. Attached v37 patch contains these
additions along with the fresh rebase on master.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick






v37-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Pavel,

Thanks for pointing out the tests. I completely agree that using an
exception inside on-login trigger should be tested. It cannot be done via
regular *.sql/*.out regress tests, thus I have added another perl test to
authentication group doing this.
Attached v36 patch contains this test along with the fresh rebase on master.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Thu, Jan 12, 2023 at 9:51 AM Pavel Stehule 
wrote:

> Hi
>
> I checked this patch and it looks well. All tests passed. Together with
> https://commitfest.postgresql.org/41/4013/ it can be a good feature.
>
> I re-tested impact on performance and for the worst case looks like less
> than 1% (0.8%). I think it is acceptable. Tested pgbench scenario "SELECT
> 1"
>
> pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres
>
> 733 tps (master), 727 tps (patched).
>
> I think raising an exception inside should be better tested - not it is
> only in 001_stream_rep.pl - generally more tests are welcome - there are
> no tested handling exceptions.
>
> Regards
>
> Pavel
>
>


v36-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Ted,

> bq. in to the system
> 'in to' -> into
> bq. Any bugs in a trigger procedure
> Any bugs -> Any bug

Thanks!  Fixed typos in the attached v35.

>   +   if (event == EVT_Login)
>   +   dbgtag = CMDTAG_LOGIN;
>   +   else
>   +   dbgtag = CreateCommandTag(parsetree);
> The same snippet appears more than once. It seems CMDTAG_LOGIN handling
can be moved into `CreateCommandTag`.

It appears twice in fact, both cases are nearby: in the main code and under
the assert-checking ifdef.
Moving CMDTAG_LOGIN to CreateCommandTag would change the CreateCommandTag
function signature and ideology. CreateCommandTag finds a tag based on the
command parse tree, while login event is a specific case when we do not
have any command and the parse tree is NULL. Changing CreateCommandTag
signature for these two calls looks a little bit overkill as it would lead
to changing lots of other places the function is called from.
We could move this snippet to a separate function, but here are the similar
concerns I think: it would make sense for a more common or a more complex
snippet, but not for two cases of if-else one-liners.

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
<http://www.flickr.com/photos/youzhick/albums>*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Sat, Dec 17, 2022 at 3:29 PM Ted Yu  wrote:

>
>
> On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov 
> wrote:
>
>> Hi Nikita,
>>
>> > Mikhail, I've checked the patch and previous discussion,
>> > the condition mentioned earlier is still actual:
>>
>> Thanks for pointing this out! My bad, I forgot to fix the documentation
>> example.
>> Attached v34 has this issue fixed, as well as a couple other problems
>> with the example code.
>>
>> --
>>  best regards,
>> Mikhail A. Gribkov
>>
> Hi,
>
> bq. in to the system
>
> 'in to' -> into
>
> bq. Any bugs in a trigger procedure
>
> Any bugs -> Any bug
>
> +   if (event == EVT_Login)
> +   dbgtag = CMDTAG_LOGIN;
> +   else
> +   dbgtag = CreateCommandTag(parsetree);
>
> The same snippet appears more than once. It seems CMDTAG_LOGIN handling
> can be moved into `CreateCommandTag`.
>
> Cheers
>


v35-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi Nikita,

> Mikhail, I've checked the patch and previous discussion,
> the condition mentioned earlier is still actual:

Thanks for pointing this out! My bad, I forgot to fix the documentation
example.
Attached v34 has this issue fixed, as well as a couple other problems with
the example code.

--
 best regards,
Mikhail A. Gribkov


v34-On_client_login_event_trigger.patch
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi hackers,
Attached v33 is a new rebase of the flagless version of the patch.  As
there were no objections at first glance, I’ll try to post it to the
upcoming commitfest, thus the brief recap of all the patch details is below.

v33-On_client_login_event_trigger
The patch introduces a 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 the
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.

The patch is prepared to be applied to the master branch and tested when
applied after e52f8b301ed54aac5162b185b43f5f1e44b6b17e commit by Thomas
Munro (Date:   Fri Dec 16 17:36:22 2022 +1300).
Regression tests and documentation included.

A couple of words about what and why I changed compared to the previous
author's version.
First, the (en/dis)abling GUC was removed from the patch because
ideologically it is a separate feature and nowadays it’s  discussed and
 supported in a separate thread by Daniel Gustaffson:
https://www.postgresql.org/message-id/flat/9140106E-F9BF-4D85-8FC8-F2D3C094A6D9%40yesql.se
Second, I have removed the dathasloginevt flag. The flag was initially
added to the patch for performance reasons and it did the job, although it
was quite a clumsy construct causing tons of bugs and could potentially
lead to tons more during further PostgreSQL evolution. I have removed the
flag and found that the performance drop is not that significant.

And this is an aspect I should describe in more detail.
The possible performance drop is expected as an increased time used to
login a user NOT using a login trigger.
First of all, the method of performance check:
echo 'select 1;' > ./tst.sql
pgbench -n -C -T3 -f tst.sql -U postgres postgres
The output value "average connection time" is the one I use to compare
performance.
Now, what are the results.
* master branch: 0.641 ms
* patched version: 0.644 ms
No significant difference here and it is just what was expected. Based on
the patch design the performance drop can be expected when there are lots
of event triggers created, but not the login one. Thus I have created 1000
drop triggers (the script for creating triggers is attached too) in the
database and repeated the test:
* master branch: 0.646 ms
* patched version: 0.754 ms
For 2000 triggers the patched version connection time is further increased
to 0.862. Thus we have a login time rise in about 16.5% per 1000 event
triggers in the database. It is a statistically noticeable value, still I
don’t think it’s a critical one we should be afraid of.
N.B. The exact values of the login times  slightly differ from the ones I
posted in the previous email. Well, that’s the repeatability level we have.
This convinces me even more that the observed level of performance drop is
acceptable.
--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick


v33-On_client_login_event_trigger.patch
Description: Binary data


create_1000_triggers.sql
Description: Binary data

Re: On login trigger: take three

[Mikhail Gribkov]

Hi hackers,



Since the original authors, as Daniel said,  seems to have retired from the
patch, I have allowed myself to continue the patch polishing.

Attached v32 includes fresh rebase and the following fixes:

- Copying dathasloginevt flag during DB creation from template;

- Restoring dathasloginevt flag after re-enabling a disabled trigger;

- Preserving dathasloginevt flag during not-running a trigger because of
some filters (running with 'session_replication_role=replica' for example);

- Checking tags during the trigger creation;

The (en/dis)abling GUC was removed from the patch by default because it is
now discussed and  supported in a separate thread by Daniel Gustaffson:


*https://www.postgresql.org/message-id/9140106e-f9bf-4d85-8fc8-f2d3c094a...@yesql.se*


Still the back-ported version of the Daniel’s patch is attached here too –
just for convenience.



While the above changes represent the straight work continue, there is
another version to consider. Most of the patch problems seem to originate
from the dathasloginevt flag support. There are lots of known problems
(partly fixed) and probably a lot more unfound. At the same time the flag
itself is not a critical element, but just a performance optimizer for
logging-in of users who are NOT using the on-login triggers.

I have made a simpler version without the flag and tried to compare the
performance. The results show that life without dathasloginevt is still
possible. When there is a small number of non-login event triggers, the
difference is barely noticeable indeed. To show the difference, I have
added 1000 sql_drop event triggers and used pgbench to continuously query
the database for “select 1”  with reconnects for 3 seconds. Here are the
results. For each version I recorded average connection time with 0 and
with 1000 event triggers:

With dathasloginevt flag: 0.609 ms VS 0.611 ms

No-flag version: 0.617 ms VS 0.727 ms

You can see that the difference is noticeable, but not that critical as we
can expect for 1000 triggers (while in a vast majority of real cases there
would be a much lesser number of triggers). I.e. the flagless version of
the patch introduces a huge reliability raise at the cost of a small
performance drop.

What do you think? Maybe it’s better to use the flagless version? Or even a
small performance drop is considered as unacceptable?



The attached files include the two versions of the patch: “v32” for the
updated version with flag and “v31_nf” – for the flagless version. Both
versions contain “0001“ file for the patch itself and “0002” for
back-ported controlling GUC.
--
 best regards,
Mikhail A. Gribkov



On Fri, Nov 4, 2022 at 3:58 AM Ian Lawrence Barwick 
wrote:

> 2022年11月4日(金) 5:23 Daniel Gustafsson :
> >
> > > On 20 Sep 2022, at 16:10, Daniel Gustafsson  wrote:
> >
> > > Since the original authors seems to have retired from the patch
> > > (I've only rebased it to try and help) I am inclined to mark it as
> returned
> > > with feedback.
> >
> > Doing so now since no updates have been posted for quite some time and
> holes
> > have been poked in the patch.
>
> I see it was still "Waiting on Author" so I went ahead and changed the
> status.
>
> > The GUC for temporarily disabling event triggers, to avoid the need for
> single-
> > user mode during troubleshooting, might however be of interest so I will
> break
> > that out and post to a new thread.
>
> For reference this is the new thread:
>
>
> https://www.postgresql.org/message-id/9140106E-F9BF-4D85-8FC8-F2D3C094A6D9%40yesql.se
>
> Regards
>
> Ian Barwick
>
>
>


v32-0002-Add-GUC-for-temporarily-disabling-event-triggers.patch
Description: Binary data


v31_nf-0001-Add-support-event-triggers-on-authenticated-login-noflag.patch
Description: Binary data


v31_nf-0002-Add-GUC-for-temporarily-disabling-event-triggers.patch
Description: Binary data


v32-0001-Add-support-event-triggers-on-authenticated-login.patch
Description: Binary data

Re: Nicely exiting PG_TRY and PG_CATCH

[Mikhail Gribkov]

> Yeah, you can't return or goto out of the PG_TRY part.

So this is a problem if the check would ever work.
(Sorry for such a delayed answer.)

Then we need to fix it. Attached is a minimal patch, which changes nothing
except for correct PG_TRY exiting.
Isn't it better this way?

CCing to Peter Eisentraut, whose patch originally introduced these returns.
Peter, will such a patch work somehow against your initial idea?

--
 best regards,
Mikhail A. Gribkov

e-mail: youzh...@gmail.com
*http://www.flickr.com/photos/youzhick/albums
<http://www.flickr.com/photos/youzhick/albums>*
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



On Sat, Oct 8, 2022 at 12:19 AM Tom Lane  wrote:

> Mikhail Gribkov  writes:
> > Usually it's not a good idea to exit PG_TRY() block via return statement.
> > Otherwise it would leave PG_exception_stack global variable in a wrong
> > state and next ereport() will jump to some junk address.
>
> Yeah, you can't return or goto out of the PG_TRY part.
>
> > Another suspicious case is PG_CATCH block in jsonb_plpython.c:
>
> This should be OK.  The PG_CATCH and PG_FINALLY macros are set up so that
> we've fully restored that state *before* we execute any of the
> error-handling code.  It would be basically impossible to have a guarantee
> that CATCH blocks never throw errors; they'd be so restricted as to be
> near useless, like signal handlers.
>
> regards, tom lane
>
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 923703535a..0c8550e53c 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -418,7 +418,7 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc)
 	{
 		args = PyList_New(proc->nargs);
 		if (!args)
-			return NULL;
+			goto end_of_try;
 
 		for (i = 0; i < proc->nargs; i++)
 		{
@@ -458,6 +458,7 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc)
 			/* cache the output conversion functions */
 			PLy_output_setup_record(>result, desc, proc);
 		}
+end_of_try:
 	}
 	PG_CATCH();
 	{
@@ -694,7 +695,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 	{
 		pltdata = PyDict_New();
 		if (!pltdata)
-			return NULL;
+			goto end_of_try;
 
 		pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
 		PyDict_SetItemString(pltdata, "name", pltname);
@@ -839,7 +840,8 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 			if (!pltargs)
 			{
 Py_DECREF(pltdata);
-return NULL;
+pltdata = NULL;
+goto end_of_try;
 			}
 			for (i = 0; i < tdata->tg_trigger->tgnargs; i++)
 			{
@@ -858,6 +860,7 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *r
 		}
 		PyDict_SetItemString(pltdata, "args", pltargs);
 		Py_DECREF(pltargs);
+end_of_try:
 	}
 	PG_CATCH();
 	{

Nicely exiting PG_TRY and PG_CATCH

[Mikhail Gribkov]

Hi hackers,

I've found some odd lines in plpython-related code. These look to me like a
potential source of problems, but maybe I'm not fully aware of some nuances.

Usually it's not a good idea to exit PG_TRY() block via return statement.
Otherwise it would leave PG_exception_stack global variable in a wrong
state and next ereport() will jump to some junk address. But here it is a
straight return in plpy_exec.c:
PG_TRY();
{
args = PyList_New(proc->nargs);
if (!args)
return NULL;
...
(
https://github.com/postgres/postgres/blob/0fe954c28584169938e5c0738cfaa9930ce77577/src/pl/plpython/plpy_exec.c#L421
)

Two more cases could be found further in the same file:
https://github.com/postgres/postgres/blob/0fe954c28584169938e5c0738cfaa9930ce77577/src/pl/plpython/plpy_exec.c#L697
https://github.com/postgres/postgres/blob/0fe954c28584169938e5c0738cfaa9930ce77577/src/pl/plpython/plpy_exec.c#L842

Isn't it a problem?


Another suspicious case is PG_CATCH block in jsonb_plpython.c:
PG_CATCH();
{
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
 errmsg("could not convert value \"%s\" to jsonb", str)));
}
(
https://github.com/postgres/postgres/blob/0fe954c28584169938e5c0738cfaa9930ce77577/contrib/jsonb_plpython/jsonb_plpython.c#L372
)

The problem is that  leaving PG_CATCH() without PG_RE_THROW(),
ReThrowError() or FlushErrorState() will consume an errordata stack slot,
while the stack size is only 5. Do this five times and we'll have a PANIC
on the next ereport().
As it's stated in elog.c (comment about the error stack depth at the
beginning of FlushErrorState()), "The only case where it would be more than
one deep is if we serviced an error that interrupted construction of
another message."
Looks to me that  FlushErrorState() should be inserted before this ereport.
Shouldn't it?

--
 best regards,
Mikhail A. Gribkov