public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1 1/4] Add a parser_hook hook.
5+ messages / 4 participants
[nested] [flat]
* [PATCH v1 1/4] Add a parser_hook hook.
@ 2021-04-21 14:47 Julien Rouhaud <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Julien Rouhaud @ 2021-04-21 14:47 UTC (permalink / raw)
This does nothing but allow third-party plugins to implement a different
syntax, and fallback on the core parser if they don't implement a superset of
the supported core syntax.
---
src/backend/tcop/postgres.c | 16 ++++++++++++++--
src/include/tcop/tcopprot.h | 5 +++++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2d6d145ecc..e91db69830 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -99,6 +99,9 @@ int log_statement = LOGSTMT_NONE;
/* GUC variable for maximum stack depth (measured in kilobytes) */
int max_stack_depth = 100;
+/* Hook for plugins to get control in pg_parse_query() */
+parser_hook_type parser_hook = NULL;
+
/* wait N seconds to allow attach from a debugger */
int PostAuthDelay = 0;
@@ -589,18 +592,27 @@ ProcessClientWriteInterrupt(bool blocked)
* database tables. So, we rely on the raw parser to determine whether
* we've seen a COMMIT or ABORT command; when we are in abort state, other
* commands are not processed any further than the raw parse stage.
+ *
+ * To support loadable plugins that monitor the parsing or implements SQL
+ * syntactic sugar we provide a hook variable that lets a plugin get control
+ * before and after the standard parsing process. If the plugin only implement
+ * a subset of postgres supported syntax, it's its duty to call raw_parser (or
+ * the previous hook if any) for the statements it doesn't understand.
*/
List *
pg_parse_query(const char *query_string)
{
- List *raw_parsetree_list;
+ List *raw_parsetree_list = NIL;
TRACE_POSTGRESQL_QUERY_PARSE_START(query_string);
if (log_parser_stats)
ResetUsage();
- raw_parsetree_list = raw_parser(query_string, RAW_PARSE_DEFAULT);
+ if (parser_hook)
+ raw_parsetree_list = (*parser_hook) (query_string, RAW_PARSE_DEFAULT);
+ else
+ raw_parsetree_list = raw_parser(query_string, RAW_PARSE_DEFAULT);
if (log_parser_stats)
ShowUsage("PARSER STATISTICS");
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index 968345404e..131dc2b22e 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -17,6 +17,7 @@
#include "nodes/params.h"
#include "nodes/parsenodes.h"
#include "nodes/plannodes.h"
+#include "parser/parser.h"
#include "storage/procsignal.h"
#include "utils/guc.h"
#include "utils/queryenvironment.h"
@@ -43,6 +44,10 @@ typedef enum
extern PGDLLIMPORT int log_statement;
+/* Hook for plugins to get control in pg_parse_query() */
+typedef List *(*parser_hook_type) (const char *str, RawParseMode mode);
+extern PGDLLIMPORT parser_hook_type parser_hook;
+
extern List *pg_parse_query(const char *query_string);
extern List *pg_rewrite_query(Query *query);
extern List *pg_analyze_and_rewrite(RawStmt *parsetree,
--
2.30.1
--ymfuxy2befenx45f
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="v1-0002-Add-a-sqlol-parser.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: disabled SSL log_like tests
@ 2025-04-17 14:56 Tom Lane <[email protected]>
2025-04-17 18:37 ` Re: disabled SSL log_like tests Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2025-04-17 14:56 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: pgsql-hackers
Andrew Dunstan <[email protected]> writes:
> Back in 2022 in commit 55828a6b6084 we disabled a bunch of tests due to
> a timing issue. Nothing seems to have been done since ... do we still
> want to keep these commented out lines around? This "temporary" fix
> seems to have stretched the definition of that term more than somewhat.
Per my coffee cup, "Nothing is as permanent as a temporary fix".
However, I wonder whether Andres' work at 8b886a4e3 could be used
to improve this, either directly or as inspiration?
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: disabled SSL log_like tests
2025-04-17 14:56 Re: disabled SSL log_like tests Tom Lane <[email protected]>
@ 2025-04-17 18:37 ` Andrew Dunstan <[email protected]>
2025-04-18 19:45 ` Re: disabled SSL log_like tests Tom Lane <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Dunstan @ 2025-04-17 18:37 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: pgsql-hackers
On 2025-04-17 Th 10:56 AM, Tom Lane wrote:
> Andrew Dunstan <[email protected]> writes:
>> Back in 2022 in commit 55828a6b6084 we disabled a bunch of tests due to
>> a timing issue. Nothing seems to have been done since ... do we still
>> want to keep these commented out lines around? This "temporary" fix
>> seems to have stretched the definition of that term more than somewhat.
> Per my coffee cup, "Nothing is as permanent as a temporary fix".
>
> However, I wonder whether Andres' work at 8b886a4e3 could be used
> to improve this, either directly or as inspiration?
>
>
I don't think so - these tests are about checking log file contents, not
a psql problem.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: disabled SSL log_like tests
2025-04-17 14:56 Re: disabled SSL log_like tests Tom Lane <[email protected]>
2025-04-17 18:37 ` Re: disabled SSL log_like tests Andrew Dunstan <[email protected]>
@ 2025-04-18 19:45 ` Tom Lane <[email protected]>
2025-04-18 20:08 ` Re: disabled SSL log_like tests Jacob Champion <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2025-04-18 19:45 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: pgsql-hackers
Andrew Dunstan <[email protected]> writes:
> On 2025-04-17 Th 10:56 AM, Tom Lane wrote:
>> However, I wonder whether Andres' work at 8b886a4e3 could be used
>> to improve this, either directly or as inspiration?
> I don't think so - these tests are about checking log file contents, not
> a psql problem.
Well, I was mainly leaning on the "inspiration" part: the idea
is to wait for something that must come out after the text we
want to look for. After digging around a bit I noticed that
002_connection_limits.pl's connect_fails_wait is doing something
that's almost what we want: that test cranks log_min_messages
up to DEBUG2 and then waits for the postmaster's report of
backend exit before believing that it's done.
Awhile later I had the attached patch. Some notes:
* The commented-out tests in 001_ssltests.pl contained hard-wired
expectations as to certificate serial numbers, which are obsolete now.
I just replaced them with "\d+", but if anyone feels like that's not
good enough, we could continue to check for exact serial numbers and
eat the ensuing maintenance effort.
* I was more than slightly surprised to find that there are a bunch of
other connect_fails callers that are testing log_like or log_unlike
and thereby risking the same type of race condition. Some of those
tests are relatively new and perhaps just haven't failed *yet*,
but I wonder if we changed something since 2022 that solves this
problem in a different way? Anyway, after this change any such
caller must set log_min_messages = debug2 or fail. I think I got
all the relevant test scripts in the attached.
regards, tom lane
Attachments:
[text/x-diff] v1-fix-connect_fails-races.patch (10.7K, ../../[email protected]/2-v1-fix-connect_fails-races.patch)
download | inline diff:
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 756b4146050..37d96d95a1a 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -66,6 +66,8 @@ sub test_conn
my $node = PostgreSQL::Test::Cluster->new('primary');
$node->init;
$node->append_conf('postgresql.conf', "log_connections = on\n");
+# Needed to allow connect_fails to inspect postmaster log:
+$node->append_conf('postgresql.conf', "log_min_messages = debug2");
$node->start;
# Test behavior of log_connections GUC
diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl
index 69ba73bd2b9..2879800eacf 100644
--- a/src/test/authentication/t/003_peer.pl
+++ b/src/test/authentication/t/003_peer.pl
@@ -72,6 +72,8 @@ sub test_role
my $node = PostgreSQL::Test::Cluster->new('node');
$node->init;
$node->append_conf('postgresql.conf', "log_connections = on\n");
+# Needed to allow connect_fails to inspect postmaster log:
+$node->append_conf('postgresql.conf', "log_min_messages = debug2");
$node->start;
# Set pg_hba.conf with the peer authentication.
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index 6748b109dec..2dc6bec9b89 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -66,6 +66,7 @@ $node->append_conf(
listen_addresses = '$hostaddr'
krb_server_keyfile = '$krb->{keytab}'
log_connections = on
+log_min_messages = debug2
lc_messages = 'C'
});
$node->start;
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 352b0fc1fa7..d1315ed5351 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -48,6 +48,8 @@ note "setting up PostgreSQL instance";
my $node = PostgreSQL::Test::Cluster->new('node');
$node->init;
$node->append_conf('postgresql.conf', "log_connections = on\n");
+# Needed to allow connect_fails to inspect postmaster log:
+$node->append_conf('postgresql.conf', "log_min_messages = debug2");
$node->start;
$node->safe_psql('postgres', 'CREATE USER test0;');
diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl
index d88994abc24..4f035417a40 100644
--- a/src/test/modules/oauth_validator/t/001_server.pl
+++ b/src/test/modules/oauth_validator/t/001_server.pl
@@ -48,6 +48,8 @@ $node->init;
$node->append_conf('postgresql.conf', "log_connections = on\n");
$node->append_conf('postgresql.conf',
"oauth_validator_libraries = 'validator'\n");
+# Needed to allow connect_fails to inspect postmaster log:
+$node->append_conf('postgresql.conf', "log_min_messages = debug2");
$node->start;
$node->safe_psql('postgres', 'CREATE USER test;');
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 8759ed2cbba..7877cc8d994 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2624,7 +2624,12 @@ If this regular expression is set, matches it with the output generated.
=item log_unlike => [ qr/prohibited message/ ]
-See C<log_check(...)>.
+See C<log_check(...)>. CAUTION: use of either option requires that
+the server's log_min_messages be at least DEBUG2, and that no other
+client backend is launched concurrently. These requirements allow
+C<connect_fails> to wait to see the postmaster-log report of backend
+exit, without which there is a race condition as to whether we will
+see the expected backend log output.
=back
@@ -2652,7 +2657,14 @@ sub connect_fails
like($stderr, $params{expected_stderr}, "$test_name: matches");
}
- $self->log_check($test_name, $log_location, %params);
+ if (defined($params{log_like}) or defined($params{log_unlike}))
+ {
+ $self->wait_for_log(
+ qr/DEBUG: (00000: )?client backend.*exited with exit code \d/,
+ $log_location);
+
+ $self->log_check($test_name, $log_location, %params);
+ }
}
=pod
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 086abf3b8b3..8b0de2d8e7e 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -60,6 +60,8 @@ my $common_connstr;
note "setting up data directory";
my $node = PostgreSQL::Test::Cluster->new('primary');
$node->init;
+# Needed to allow connect_fails to inspect postmaster log:
+$node->append_conf('postgresql.conf', "log_min_messages = debug2");
# PGHOST is enforced here to set up the node, subsequent connections
# will use a dedicated connection string.
@@ -807,10 +809,8 @@ $node->connect_fails(
expected_stderr =>
qr/certificate authentication failed for user "anotheruser"/,
# certificate authentication should be logged even on failure
- # temporarily(?) skip this check due to timing issue
- # log_like =>
- # [qr/connection authenticated: identity="CN=ssltestuser" method=cert/],
-);
+ log_like =>
+ [qr/connection authenticated: identity="CN=ssltestuser" method=cert/],);
# revoked client cert
$node->connect_fails(
@@ -818,11 +818,10 @@ $node->connect_fails(
. sslkey('client-revoked.key'),
"certificate authorization fails with revoked client cert",
expected_stderr => qr|SSL error: ssl[a-z0-9/]* alert certificate revoked|,
- # temporarily(?) skip this check due to timing issue
- # log_like => [
- # qr{Client certificate verification failed at depth 0: certificate revoked},
- # qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number 2315134995201656577, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
- # ],
+ log_like => [
+ qr{Client certificate verification failed at depth 0: certificate revoked},
+ qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number \d+, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
+ ],
# revoked certificates should not authenticate the user
log_unlike => [qr/connection authenticated:/],);
@@ -872,24 +871,20 @@ $node->connect_fails(
$common_connstr . " " . "sslmode=require sslcert=ssl/client.crt",
"intermediate client certificate is missing",
expected_stderr => qr/SSL error: tlsv1 alert unknown ca/,
- # temporarily(?) skip this check due to timing issue
- # log_like => [
- # qr{Client certificate verification failed at depth 0: unable to get local issuer certificate},
- # qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number 2315134995201656576, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
- # ]
-);
+ log_like => [
+ qr{Client certificate verification failed at depth 0: unable to get local issuer certificate},
+ qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number \d+, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
+ ]);
$node->connect_fails(
"$common_connstr sslmode=require sslcert=ssl/client-long.crt "
. sslkey('client-long.key'),
"logged client certificate Subjects are truncated if they're too long",
expected_stderr => qr/SSL error: tlsv1 alert unknown ca/,
- # temporarily(?) skip this check due to timing issue
- # log_like => [
- # qr{Client certificate verification failed at depth 0: unable to get local issuer certificate},
- # qr{Failed certificate data \(unverified\): subject "\.\.\./CN=ssl-123456789012345678901234567890123456789012345678901234567890", serial number 2315418733629425152, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
- # ]
-);
+ log_like => [
+ qr{Client certificate verification failed at depth 0: unable to get local issuer certificate},
+ qr{Failed certificate data \(unverified\): subject "\.\.\./CN=ssl-123456789012345678901234567890123456789012345678901234567890", serial number \d+, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
+ ]);
# Use an invalid cafile here so that the next test won't be able to verify the
# client CA.
@@ -904,12 +899,10 @@ $node->connect_fails(
"$common_connstr sslmode=require sslcert=ssl/client+client_ca.crt",
"intermediate client certificate is untrusted",
expected_stderr => qr/SSL error: tlsv1 alert unknown ca/,
- # temporarily(?) skip this check due to timing issue
- # log_like => [
- # qr{Client certificate verification failed at depth 1: unable to get local issuer certificate},
- # qr{Failed certificate data \(unverified\): subject "/CN=Test CA for PostgreSQL SSL regression test client certs", serial number 2315134995201656577, issuer "/CN=Test root CA for PostgreSQL SSL regression test suite"},
- # ]
-);
+ log_like => [
+ qr{Client certificate verification failed at depth 1: unable to get local issuer certificate},
+ qr{Failed certificate data \(unverified\): subject "/CN=Test CA for PostgreSQL SSL regression test client certs", serial number \d+, issuer "/CN=Test root CA for PostgreSQL SSL regression test suite"},
+ ]);
# test server-side CRL directory
switch_server_cert(
@@ -923,12 +916,10 @@ $node->connect_fails(
. sslkey('client-revoked.key'),
"certificate authorization fails with revoked client cert with server-side CRL directory",
expected_stderr => qr|SSL error: ssl[a-z0-9/]* alert certificate revoked|,
- # temporarily(?) skip this check due to timing issue
- # log_like => [
- # qr{Client certificate verification failed at depth 0: certificate revoked},
- # qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number 2315134995201656577, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
- # ]
-);
+ log_like => [
+ qr{Client certificate verification failed at depth 0: certificate revoked},
+ qr{Failed certificate data \(unverified\): subject "/CN=ssltestuser", serial number \d+, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
+ ]);
# revoked client cert, non-ASCII subject
$node->connect_fails(
@@ -936,11 +927,9 @@ $node->connect_fails(
. sslkey('client-revoked-utf8.key'),
"certificate authorization fails with revoked UTF-8 client cert with server-side CRL directory",
expected_stderr => qr|SSL error: ssl[a-z0-9/]* alert certificate revoked|,
- # temporarily(?) skip this check due to timing issue
- # log_like => [
- # qr{Client certificate verification failed at depth 0: certificate revoked},
- # qr{Failed certificate data \(unverified\): subject "/CN=\\xce\\x9f\\xce\\xb4\\xcf\\x85\\xcf\\x83\\xcf\\x83\\xce\\xad\\xce\\xb1\\xcf\\x82", serial number 2315420958437414144, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
- # ]
-);
+ log_like => [
+ qr{Client certificate verification failed at depth 0: certificate revoked},
+ qr{Failed certificate data \(unverified\): subject "/CN=\\xce\\x9f\\xce\\xb4\\xcf\\x85\\xcf\\x83\\xcf\\x83\\xce\\xad\\xce\\xb1\\xcf\\x82", serial number \d+, issuer "/CN=Test CA for PostgreSQL SSL regression test client certs"},
+ ]);
done_testing();
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: disabled SSL log_like tests
2025-04-17 14:56 Re: disabled SSL log_like tests Tom Lane <[email protected]>
2025-04-17 18:37 ` Re: disabled SSL log_like tests Andrew Dunstan <[email protected]>
2025-04-18 19:45 ` Re: disabled SSL log_like tests Tom Lane <[email protected]>
@ 2025-04-18 20:08 ` Jacob Champion <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Jacob Champion @ 2025-04-18 20:08 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; pgsql-hackers
On Fri, Apr 18, 2025 at 12:46 PM Tom Lane <[email protected]> wrote:
> * The commented-out tests in 001_ssltests.pl contained hard-wired
> expectations as to certificate serial numbers, which are obsolete now.
> I just replaced them with "\d+", but if anyone feels like that's not
> good enough, we could continue to check for exact serial numbers and
> eat the ensuing maintenance effort.
No, I think \d+ should be fine.
(I haven't stared closely at the racing code yet, but I like the
concept of the patch.)
Thanks!
--Jacob
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-04-18 20:08 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21 14:47 [PATCH v1 1/4] Add a parser_hook hook. Julien Rouhaud <[email protected]>
2025-04-17 14:56 Re: disabled SSL log_like tests Tom Lane <[email protected]>
2025-04-17 18:37 ` Re: disabled SSL log_like tests Andrew Dunstan <[email protected]>
2025-04-18 19:45 ` Re: disabled SSL log_like tests Tom Lane <[email protected]>
2025-04-18 20:08 ` Re: disabled SSL log_like tests Jacob Champion <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox