Re: Allow pg_archivecleanup to remove backup history files
On Thu, May 25, 2023 at 11:51:18PM +0900, torikoshia wrote:
> Updated patches according to your comment.
- ok(!-f "$tempdir/$walfiles[1]",
- "$test_name: second older WAL file was cleaned up");
- ok(-f "$tempdir/$walfiles[2]",
+ ok(!-f "$tempdir/@$walfiles[1]",
+ "$test_name: second older WAL/backup history file was cleaned up");
+ ok(-f "$tempdir/@$walfiles[2]",
This is still a bit confusing, because as designed the test has a
dependency on the number of elements present in the list, and the
description of the test may not refer to what's actually used (the
second element of each list is either a WAL segment or a backup
history file). I think that I would just rewrite that so as we have a
list of WAL segments in an array with the expected result associated
to each one of them. Basically, something like that:
my @wal_segments = (
{ name => "SEGMENT1", present => 0 },
{ name => "BACKUPFILE1", present => 1 },
{ name => "SEGMENT2", present => 0 });
Then the last part of run_check() would loop through all the elements
listed.
--
Michael
signature.asc
Description: PGP signature
Re: Allow pg_archivecleanup to remove backup history files
On Fri, May 26, 2023 at 10:07:48AM +0900, Kyotaro Horiguchi wrote: > + if (!IsXLogFileName(walfile) && > !IsPartialXLogFileName(walfile) && > + (!cleanBackupHistory || > !IsBackupHistoryFileName(walfile))) > + continue; > > I'm not certain about the others, but I see this a tad tricky to > understand at first glance. Here's how I would phrase it. (A nearby > comment might require a tweak if we go ahead with this change.) > > if (!(IsXLogFileName(walfile) || IsPartialXLogFileName(walfile) > || > (cleanBackupHistory && > IsBackupHistoryFileName(walfile > or > if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile) > && > !(cleanBackupHistory && > IsBackupHistoryFileName(walfile))) FWIW, I am OK with what's written in the patch, but it is true that your second suggestion makes things a bit easier to read, perhaps. -- Michael signature.asc Description: PGP signature
Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?
On Thu, May 25, 2023 at 10:16:27AM +0200, Daniel Gustafsson wrote: > -#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH > +#ifdef USE_OPENSSL > Since this is only calling the pgtls abstraction API and not directly into the > SSL implementation we should use USE_SSL here instead. Same for the > corresponding hunks in the frontend code. Makes sense, done. > + * Note that if we don't support channel binding if we're not using SSL > at > + * all, we would not have advertised the PLUS variant in the first > place. > Seems like a word fell off when merging these sentences. This should probably > be "..support channel binding, or if we're no.." or something similar. Indeed, something has been eaten when merging these lines. > -#if defined(USE_OPENSSL) && (defined(HAVE_X509_GET_SIGNATURE_NID) || > defined(HAVE_X509_GET_SIGNATURE_INFO)) > -#define HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH > +#ifdef USE_OPENSSL > extern char *pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len); > #endif > No need for any guard at all now is there? All supported SSL implementations > have it, and I doubt we'd accept a new one that doesn't (or which can't > implement the function and error out). Yup. It looks that you are right. A build without SSL is not complaining once removed in libpq-int.h or libpq-be.h. > - # Functions introduced in OpenSSL 1.0.2. LibreSSL does not have > - # SSL_CTX_set_cert_cb(). > - AC_CHECK_FUNCS([X509_get_signature_nid SSL_CTX_set_cert_cb]) > + # LibreSSL does not have SSL_CTX_set_cert_cb(). > + AC_CHECK_FUNCS([SSL_CTX_set_cert_cb]) > The comment about when the function was introduced is still interesting and > should remain IMHO. Okay. Kept as well in meson.build. > The changes to the Windows buildsystem worry me a bit, but they don't move the > goalposts in either direction wrt to LibreSSL on Windows so for the purpose of > this patch we don't need to do more. Longer term we should either make > LibreSSL work on Windows builds, or explicitly not support it on Windows. Yes, I was wondering what to do about that in the long term. With the argument that the MSVC scripts may be gone over meson, it is not really appealing to dig into that. Something that was missing in 0001 is the bump of OPENSSL_API_COMPAT in meson.build. This was in 0002. Please find attached an updated patch only for the removal of 1.0.1. Thanks for the review. -- Michael From b59a9a192d88bfd6261a3559ace8caced2e25d9a Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 26 May 2023 10:45:35 +0900 Subject: [PATCH v2] Remove support for OpenSSL 1.0.1 A few notes about that: - This simplifies a lot of code areas related to channel binding, as X509_get_signature_nid() always exists. - This removes the comments related to 1.0.1e introduced by 74242c2. - Do we need to care about the case of building the Postgres code with LibreSSL on Windows for the MSVC scripts, leading to the removal of the check with HAVE_SSL_CTX_SET_CERT_CB? --- src/include/libpq/libpq-be.h | 6 src/include/pg_config.h.in | 3 -- src/backend/libpq/auth-scram.c | 20 ++-- src/backend/libpq/be-secure-openssl.c| 4 --- src/interfaces/libpq/fe-auth-scram.c | 8 ++--- src/interfaces/libpq/fe-auth.c | 2 +- src/interfaces/libpq/fe-secure-openssl.c | 4 --- src/interfaces/libpq/libpq-int.h | 6 src/test/ssl/t/002_scram.pl | 41 doc/src/sgml/installation.sgml | 2 +- configure| 16 - configure.ac | 8 ++--- meson.build | 7 ++-- src/tools/msvc/Solution.pm | 10 +- 14 files changed, 37 insertions(+), 100 deletions(-) diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index 3b2ce9908f..a0b74c8095 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -305,14 +305,8 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len); * * The result is a palloc'd hash of the server certificate with its * size, and NULL if there is no certificate available. - * - * This is not supported with old versions of OpenSSL that don't have - * the X509_get_signature_nid() function. */ -#if defined(USE_OPENSSL) && (defined(HAVE_X509_GET_SIGNATURE_NID) || defined(HAVE_X509_GET_SIGNATURE_INFO)) -#define HAVE_BE_TLS_GET_CERTIFICATE_HASH extern char *be_tls_get_certificate_hash(Port *port, size_t *len); -#endif /* init hook for SSL, the default sets the password callback if appropriate */ #ifdef USE_OPENSSL diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 6d572c3820..ca3a49c552 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -529,9 +529,6 @@ /* Define to 1 if you have the `X509_get_signature_info' function. */ #undef HAVE_X509_GET_SIGNATURE_INFO -/* Define to 1 if you have the `X509_get_signat
Cleaning up nbtree after logical decoding on standby work
Commit 61b313e4, which prepared index access method code for the logical decoding on standbys work, made quite a few mechanical changes. Many routines were revised in order to make sure that heaprel was available in _bt_getbuf()'s P_NEW (new page allocation) path. But this went further than it really had to. Many of the changes to nbtree seem excessive. We only need a heaprel in those code paths that might end up calling _bt_getbuf() with blkno = P_NEW. This includes most callers that pass access = BT_WRITE, and all callers that pass access = BT_READ. This doesn't have to be haphazard -- there just aren't that many places that can allocate new nbtree pages. It's just page splits, and new root page allocations (which are actually a slightly different kind of page split). The rule doesn't need to be complicated (to be fair it looks more complicated than it really is). Attached patch completely removes the changes to _bt_getbuf()'s signature from 61b313e4. This is possible without any loss of functionality. The patch splits _bt_getbuf () in two: the code that handles BT_READ/BT_WRITE stays in _bt_getbuf(), which is now far shorter. Handling of new page allocation is moved to a new routine I've called _bt_alloc(). This is clearer in general, and makes it clear what the rules really are. Any code that might need to call _bt_alloc() must be prepared for that, by having a heaprel to pass to it (the slightly complicated case is interrupted page splits). It's possible that Bertand would have done it this way to begin with were it not for the admittedly pretty bad nbtree convention around P_NEW. It would be nice to get rid of P_NEW in the near future, too -- I gather that there was discussion of that in the context of recent work in this area. -- Peter Geoghegan v1-0001-nbtree-Remove-use-of-P_NEW.patch Description: Binary data
Re: Docs: Encourage strong server verification with SCRAM
On 5/25/23 3:27 PM, Jacob Champion wrote: On Thu, May 25, 2023 at 10:48 AM Jonathan S. Katz wrote: Overall, +1 to tightening the language around the docs in this area. However, to paraphrase Stephen, I think the language, as currently written, makes the problem sound scarier than it actually is, and I agree that we should just inline it above. How does v2 look? I more or less divided the current text into a local section and a network section. (I'm still not clear on where in the current text you're wanting me to inline a sudden aside on SCRAM; it doesn't really seem to fit in any of the existing paragraphs.) I read through the proposal and like this much better. I missed Stephen's point on the "where" to put it in this section; I actually don't know if I agree with that (he says while painting the bikeshed), given the we spend two paragraphs describing how to prevent spoofing in general over the network, vs. just during SCRAM authentication. I rewrote this to just focus on server spoofing that can occur with SCRAM authentication and did some wordsmithing. I was torn on keeping in the part of offline analysis of an intercepted hash, given one can do this with md5 as well, but perhaps it helps elaborate on the consequences. Thanks, Jonathan diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml index dbe23db54f..a3f4b258f7 100644 --- a/doc/src/sgml/runtime.sgml +++ b/doc/src/sgml/runtime.sgml @@ -2014,6 +2014,18 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433 CA. + + To prevent server spoofing from occurring when using + scram-sha-256 password authentication + over a network, you should ensure you are connecting using SSL. Additionally, + the SCRAM implementation in libpq cannot protect + the entire authentication exchange, but using the + channel_binding=require connection parameter provides a + mitigation against server spoofing. An attacker that uses a rogue server to + intercept a SCRAM exchange can use offline analysis to determine the hashed + password from the client. + + To prevent spoofing with GSSAPI, the server must be configured to accept only hostgssenc connections OpenPGP_signature Description: OpenPGP digital signature
Re: Allow pg_archivecleanup to remove backup history files
At Thu, 25 May 2023 23:51:18 +0900, torikoshia
wrote in
> Updated patches according to your comment.
+ printf(_(" -x --strip-extension=EXTclean up files if they have
this extension\n"));
Perhaps a comma is needed after "-x". The apparent inconsistency
between the long name and the description is perplexing. I think it
might be more suitable to reword the descrition to "ignore this
extension while identifying files for cleanup" or something along
those lines..., and then name the option as "--ignore-extension".
The patch leaves the code.
>* Truncation is essentially harmless, because we skip
> names of
>* length other than XLOG_FNAME_LEN. (In principle,
> one could use
>* a 1000-character additional_ext and get trouble.)
>*/
> strlcpy(walfile, xlde->d_name, MAXPGPATH);
> TrimExtension(walfile, additional_ext);
The comment is no longer correct about the file name length.
+ if (!IsXLogFileName(walfile) &&
!IsPartialXLogFileName(walfile) &&
+ (!cleanBackupHistory ||
!IsBackupHistoryFileName(walfile)))
+ continue;
I'm not certain about the others, but I see this a tad tricky to
understand at first glance. Here's how I would phrase it. (A nearby
comment might require a tweak if we go ahead with this change.)
if (!(IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)
||
(cleanBackupHistory &&
IsBackupHistoryFileName(walfile
or
if (!IsXLogFileName(walfile) && !IsPartialXLogFileName(walfile)
&&
!(cleanBackupHistory &&
IsBackupHistoryFileName(walfile)))
By the way, this patch reduces the nesting level. If we go that
direction, the following structure could be reworked similarly, and I
believe it results in a more standard structure.
if ((xldir = opendir(archiveLocation)) != NULL)
{
...
}
else
pg_fatal("could not open archive location..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Order changes in PG16 since ICU introduction
On Mon, 2023-05-22 at 14:34 +0200, Peter Eisentraut wrote: > Please put blank lines between > > > > > etc., matching existing style. > > We usually don't capitalize the collation parameters like > > CREATE COLLATION mycollation1 (PROVIDER = icu, LOCALE = 'ja-JP); > > elsewhere in the documentation. > > Table 24.2. ICU Collation Settings should probably be sorted by key, > or > at least by something. > > All tables should referenced in the text, like "Table x.y shows this > and > that." (Note that a table could float to a different page in some > output formats, so just putting it into a section without some > introductory text isn't sound.) Thank you, done. > Table 24.1. ICU Collation Levels shows punctuation as level 4, which > is > only true in shifted mode, which isn't the default. The whole > business > of treating variable collation elements is getting a bit lost in this > description. The kv option is described as "Classes of characters > ignored during comparison at level 3.", which is effectively true but > not the whole picture. I organized the documentation around practical examples and available options, and less around the conceptual model. I think that's a good start, but you're right that it over-simplifies in a few areas. Discussing the model would work better along with an explanation of ICU rules, where you can make better use of those concepts. I feel like there are some interesting things that can be done with rules, but I haven't had a chance to really dig in yet. Regards, Jeff Davis
Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?
On Thu, May 25, 2023 at 10:27:02AM +0200, Daniel Gustafsson wrote: > There are still RHEL7 animals like chub who use 1.0.2 so I'm not worried. We > might want to consider displaying the OpenSSL version number during configure > (meson already does it) in all branches to make it easier to figure out which > versions we have coverage for? +1. -- Michael signature.asc Description: PGP signature
Re: Avoiding another needless ERROR during nbtree page deletion
On Mon, May 22, 2023 at 10:59 AM Peter Geoghegan wrote: > Attached is v2, which does it that way. It also adjusts the approach > taken to release locks and pins when the left sibling validation check > fails. I pushed this just now, backpatching all the way. > Not including a revised amcheck patch here, since I'm not exactly sure > what to do with your feedback on that one just yet. I'd like to go with a minimal approach in my patch to address the remaining issue in amcheck. Something similar to the patch that was posted as part of v1. While it seems important to address the issue, making sure that we have coverage of the leftmost page really being half-dead (as opposed to something that would constitute corruption) seems much less important. Ideally we'd have exhaustive coverage, but it's not a priority for me right now. -- Peter Geoghegan
Re: Implement generalized sub routine find_in_log for tap test
On Thu, May 25, 2023 at 06:34:20PM +0100, Dagfinn Ilmari Mannsåker wrote: > However, none of the other functions in ::Utils know anything about node > objects, which makes me think it should be a method on the node itself > (i.e. in PostgreSQL::Test::Cluster) instead. Also, I think log_contains > would be a better name, since it just returns a boolean. The name > find_in_log makes me think it would return the log lines matching the > pattern, or the position of the match in the file. > > In that case, the slurp_file() call would have to be fully qualified, > since ::Cluster uses an empty import list to avoid polluting the method > namespace with imported functions. Hmm. connect_ok() and connect_fails() in Cluster.pm have a similar log comparison logic, feeding from the offset of a log file. Couldn't you use the same code across the board for everything? Note that this stuff is parameterized so as it is possible to check if patterns match or do not match, for multiple patterns. It seems to me that we could use the new log finding routine there as well, so how about extending it a bit more? You would need, at least: - One parameter for log entries matching. - One parameter for log entries not matching. -- Michael signature.asc Description: PGP signature
Re: ERROR: no relation entry for relid 6
Richard Guo writes:
> On Wed, May 24, 2023 at 2:48 AM Tom Lane wrote:
>> I wonder if we could do something involving recording the
>> rinfo_serial numbers of all the clauses extracted from a particular
>> syntactic join level (we could keep this in a bitmapset attached
>> to each SpecialJoinInfo, perhaps) and then filtering the joinclauses
>> on the basis of serial numbers instead of required_relids.
> I think this is a better way to fix the issue. I went ahead and drafted
> a patch as attached. But I doubt that the collecting of rinfo_serial
> numbers is thorough in the patch. Should we also collect the
> rinfo_serial of quals generated in reconsider_outer_join_clauses()?
Not sure. However, I thought of a possible fix that doesn't require
so much new mechanism: we could consider potentially-commuted outer
joins as part of the relid set that's okay to discard, as in the
attached. This is still relying on RINFO_IS_PUSHED_DOWN, which I
continue to feel is not quite the right thing here, but on the other
hand that logic survived for years without trouble. What broke it
was addition of outer-join relids to the mix. I briefly considered
proposing that we simply ignore all outer-join relids in the test that
decides whether to keep or discard a joinqual, but this way seems at
least slightly more principled.
A couple of notes:
1. The test case you give upthread only needs to ignore commute_below_l,
that is it still passes without the lines
+join_plus_commute = bms_add_members(join_plus_commute,
+removable_sjinfo->commute_above_r);
Based on what deconstruct_distribute_oj_quals is doing, it seems
likely to me that there are cases that require ignoring
commute_above_r, but I've not tried to devise one. It'd be good to
have one to include in the commit, if we can find one.
2. We could go a little further in refactoring, specifically the
computation of joinrelids could be moved into remove_rel_from_query,
since remove_useless_joins itself doesn't need it. Not sure if
this'd be an improvement or not. Certainly it'd be a loser if
remove_useless_joins grew a reason to need the value; but I can't
foresee a reason for that to happen --- remove_rel_from_query is
where basically all the work is going on.
3. I called the parameter removable_sjinfo because sjinfo is already
used within another loop, leading to a shadowed-parameter warning.
In a green field we'd probably have called the parameter sjinfo
and used another name for the loop's local variable, but that would
make the patch bulkier without adding anything. Haven't decided
whether to rename before commit or leave it as-is.
regards, tom lane
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index a61e35f92d..54ae0379b3 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -35,7 +35,8 @@
/* local functions */
static bool join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo);
-static void remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
+static void remove_rel_from_query(PlannerInfo *root, int relid,
+ SpecialJoinInfo *removable_sjinfo,
Relids joinrelids);
static void remove_rel_from_restrictinfo(RestrictInfo *rinfo,
int relid, int ojrelid);
@@ -93,7 +94,7 @@ restart:
if (sjinfo->ojrelid != 0)
joinrelids = bms_add_member(joinrelids, sjinfo->ojrelid);
- remove_rel_from_query(root, innerrelid, sjinfo->ojrelid, joinrelids);
+ remove_rel_from_query(root, innerrelid, sjinfo, joinrelids);
/* We verify that exactly one reference gets removed from joinlist */
nremoved = 0;
@@ -331,10 +332,13 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo)
* the planner's data structures that will actually be consulted later.
*/
static void
-remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
+remove_rel_from_query(PlannerInfo *root, int relid,
+ SpecialJoinInfo *removable_sjinfo,
Relids joinrelids)
{
RelOptInfo *rel = find_base_rel(root, relid);
+ int ojrelid = removable_sjinfo->ojrelid;
+ Relids join_plus_commute;
List *joininfos;
Index rti;
ListCell *l;
@@ -454,8 +458,19 @@ remove_rel_from_query(PlannerInfo *root, int relid, int ojrelid,
* outerjoin-delayed quals, which can get marked as requiring the rel in
* order to force them to be evaluated at or above the join. We can't
* just discard them, though. Only quals that logically belonged to the
- * outer join being discarded should be removed from the query.
- *
+ * outer join being discarded should be removed from the query. However,
+ * we might encounter a qual that is a clone of a deletable qual with some
+ * outer-join relids added (see deconstruct_distribute_oj_quals). To
+ * ensure we get rid of such clones as well, add the relids of all OJs
+ * commutable with this one to the set we
Re: PG 16 draft release notes ready
On Thu, 2023-05-18 at 16:49 -0400, Bruce Momjian wrote: > I have completed the first draft of the PG 16 release notes. I found two typos. Yours, Laurenz Albe diff --git a/doc/src/sgml/release-16.sgml b/doc/src/sgml/release-16.sgml index faecae7c42..7dad0b8550 100644 --- a/doc/src/sgml/release-16.sgml +++ b/doc/src/sgml/release-16.sgml @@ -1294,7 +1294,7 @@ Determine the ICU default locale from the environment (Jeff Davis) -However, ICU doesn't support the C local so UTF-8 is used in such cases. Previously the default was always UTF-8. +However, ICU doesn't support the C locale so UTF-8 is used in such cases. Previously the default was always UTF-8. @@ -1335,7 +1335,7 @@ Author: Peter Eisentraut -Add Windows process the system collations (Jose Santamaria Flecha) +Add Windows to process the system collations (Jose Santamaria Flecha) ADD THIS?
Re: PG 16 draft release notes ready
Bruce Momjian writes:
> I have completed the first draft of the PG 16 release notes. You can
> see the output here:
>
> https://momjian.us/pgsql_docs/release-16.html
>
> I will adjust it to the feedback I receive; that URL will quickly show
> all updates.
The bit about auto_explain and query parameters says:
> Allow auto_explain to log query parameters used in executing prepared
> statements (Dagfinn Ilmari Mannsåker)
>
> This is controlled by auto_explain.log_parameter_max_length, and by
> default query parameters will be logged with no length
> restriction. SHOULD THIS BE MORE CLEARLY IDENTIFIED AS CONTROLLING THE
> EXECUTION OF PREPARED STATEMENTS?
This is wrong, the logging applies to all query parameters, not just for
prepared statements (and has nothing to do with controlling the
execution thereof). That was just the only way to test it when it was
written, because psql's \bind command exist yet then.
Should we perhaps add some tests for that, like the attached?
- ilmari
>From d3630f299fc2d2d9f9eb3addd426f98e5196100d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Thu, 25 May 2023 21:13:11 +0100
Subject: [PATCH] Test auto_explain parameter logging with protocol-level bind
parameters
When auto_explain.log_parameter_max_length was added, psql didn't have
the \bind command for extended query protocol yet, so the test could
only use prepared statements.
---
contrib/auto_explain/t/001_auto_explain.pl | 31 ++
1 file changed, 31 insertions(+)
diff --git a/contrib/auto_explain/t/001_auto_explain.pl b/contrib/auto_explain/t/001_auto_explain.pl
index abb422f8de..d2a0078546 100644
--- a/contrib/auto_explain/t/001_auto_explain.pl
+++ b/contrib/auto_explain/t/001_auto_explain.pl
@@ -106,6 +106,21 @@ sub query_log
qr/Query Parameters:/,
"query parameters not logged when disabled, text mode");
+# bind parameters
+$log_contents = query_log($node,
+ q{SELECT * FROM pg_proc WHERE proname = $1 AND prokind = $2 \bind ascii f \g}
+);
+
+like(
+ $log_contents,
+ qr/Query Text: SELECT \* FROM pg_proc WHERE proname = \$1 AND prokind = \$2/,
+ "query text with parameters logged, text mode");
+
+like(
+ $log_contents,
+ qr/Query Parameters: \$1 = 'ascii', \$2 = 'f'/,
+ "query parameters logged, text mode");
+
# Query Identifier.
# Logging enabled.
$log_contents = query_log(
@@ -172,6 +187,22 @@ sub query_log
qr/"Node Type": "Index Scan"[^}]*"Index Name": "pg_class_relname_nsp_index"/s,
"index scan logged, json mode");
+# query with bind parameters in JSON format.
+$log_contents = query_log(
+ $node,
+ q{SELECT * FROM pg_class WHERE relname = $1 AND relkind = $2 \bind pg_proc r \x},
+ { "auto_explain.log_format" => "json" });
+
+like(
+ $log_contents,
+ qr/"Query Text": "SELECT \* FROM pg_class WHERE relname = \$1 AND relkind = \$2 "/,
+ "query text with parameters logged, json mode");
+
+like(
+ $log_contents,
+ qr/"Query Parameters": "\$1 = 'pg_proc', \$2 = 'r'"/,
+ "query parameters logged, json mode");
+
# Check that PGC_SUSET parameters can be set by non-superuser if granted,
# otherwise not
--
2.39.2
Re: Docs: Encourage strong server verification with SCRAM
On Thu, May 25, 2023 at 10:48 AM Jonathan S. Katz wrote: > Overall, +1 to tightening the language around the docs in this area. > > However, to paraphrase Stephen, I think the language, as currently > written, makes the problem sound scarier than it actually is, and I > agree that we should just inline it above. How does v2 look? I more or less divided the current text into a local section and a network section. (I'm still not clear on where in the current text you're wanting me to inline a sudden aside on SCRAM; it doesn't really seem to fit in any of the existing paragraphs.) --Jacob From 96dee3dead97d38afd0d8139f5c9954ab76dfcba Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Mon, 22 May 2023 16:46:23 -0700 Subject: [PATCH v2] docs: encourage strong server verification with SCRAM The server verification in libpq's SCRAM implementation can be subverted in various ways. While mitigations for some of the issues exist, it's probably wise for most users to combine it with strong server authentication, to avoid entering a SCRAM exchange with an untrusted server. Recommend that in the docs. --- doc/src/sgml/runtime.sgml | 14 ++ 1 file changed, 14 insertions(+) diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml index dbe23db54f..359932 100644 --- a/doc/src/sgml/runtime.sgml +++ b/doc/src/sgml/runtime.sgml @@ -1994,6 +1994,20 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433 the socket. + + An additional avenue for server spoofing is via the network. Active attackers + who successfully insert themselves between the client and the real server can + not only read data coming from the client, as above, but also read data + coming from the server or tamper with data sent by either peer. The strongest + form of password authentication + (scram-sha-256 in combination with + channel_binding=require) can provide partial mitigation, + but the current SCRAM implementation in libpq cannot protect the entire + authentication exchange, and an active attacker can retrieve a hashed + password from the client for offline analysis, even if they cannot complete + the exchange successfully. + + To prevent spoofing on TCP connections, either use SSL certificates and make sure that clients check the server's certificate, -- 2.25.1
Re: pg_collation.collversion for C.UTF-8
Jeff Davis writes: > What should we do with locales like C.UTF-8 in both libc and ICU? I vote for passing those to the existing C-specific code paths, whereever we have any (not sure that we do for functionality). The semantics are quite well-defined and I can see no good coming of allowing either provider to mess with them. > If we capture it like the C locale, then where do we draw the line? Any > locale that begins with "C."? What if the language part is C but there > is some other part to the locale? What about lower case? Should all of > these apply the same way except with POSIX? What about backwards > compatibility? Probably "C", or "C.anything", or "POSIX", or "POSIX.anything". Case-independent might be good, but we haven't accepted such in the past, so I don't feel strongly about it. (Arguably, passing lower case "c" to the provider would provide an "out" to anybody who dislikes our choices here.) regards, tom lane
Re: pg_collation.collversion for C.UTF-8
On Wed, 2023-04-19 at 14:07 +1200, Thomas Munro wrote: > That strengthens my opinion that C.UTF-8 (the real C.UTF-8 supplied > by > the glibc project) isn't supposed to be versioned, but it's extremely > unfortunate that a bunch of OSes (Debian and maybe more) have been > sorting text in some other order under that name for years. What should we do with locales like C.UTF-8 in both libc and ICU? We either need to capture it and use the memcmp/pg_ascii code paths so it doesn't use the provider at all (like C); or if we send it to the provider, we can't have too many expectations about what will be done with it (even if we know what "should" happen). If we capture it like the C locale, then where do we draw the line? Any locale that begins with "C."? What if the language part is C but there is some other part to the locale? What about lower case? Should all of these apply the same way except with POSIX? What about backwards compatibility? If we pass it to the provider: * ICU: Recent versions of ICU don't recognize C.UTF-8 at all, and if you try to open it, you'll get the root collator (with warning or error, which is not great for such a common locale name). ICU versions 63 and earlier recognize C.UTF-8 as en-US-u-va-posix (a.k.a. en_US_POSIX), which has some adjustments to match expectations of C sorting (e.g. upper case first). * libc: problems as raised in this thread. Regards, Jeff Davis
Re: vector search support
Hi Nathan, A nice side effect of using the float8[] to represent vectors is that it allows for vectors of different sizes to coexist in the same column. We most frequently see (pgvector) vector columns being used for storing ML embeddings. Given that different models produce embeddings with a different number of dimensions, the need to specify a vector’s size in DDL tightly couples the schema to a single model. Support for variable length vectors would be a great way to decouple those concepts. It would also be a differentiating feature from existing vector stores. One drawback is that variable length vectors complicates indexing for similarity search because similarity measures require vectors of consistent length. Partial indexes are a possible solution to that challenge --- Oliver Rice Supabase: https://supabase.com/ > On Apr 21, 2023, at 7:07 PM, Nathan Bossart wrote: > > Attached is a proof-of-concept/work-in-progress patch set that adds > functions for "vectors" repreѕented with one-dimensional float8 arrays. > These functions may be used in a variety of applications, but I am > proposing them with the AI/ML use-cases in mind. I am posting this early > in the v17 cycle in hopes of gathering feedback prior to PGCon. > > With the accessibility of AI/ML tools such as large language models (LLMs), > there has been a demand for storing and manipulating high-dimensional > vectors in PostgreSQL, particularly around nearest-neighbor queries. Many > of these vectors have more than 1500 dimensions. The cube extension [0] > provides some of the distance functionality (e.g., taxicab, Euclidean, and > Chebyshev), but it is missing some popular functions (e.g., cosine > similarity, dot product), and it is limited to 100 dimensions. We could > extend cube to support more dimensions, but this would require reworking > its indexing code and filling in gaps between the cube data type and the > array types. For some previous discussion about using the cube extension > for this kind of data, see [1]. > > float8[] is well-supported and allows for effectively unlimited dimensions > of data. float8 matches the common output format of many AI embeddings, > and it allows us or extensions to implement indexing methods around these > functions. This patch set does not yet contain indexing support, but we > are exploring using GiST or GIN for the use-cases in question. It might > also be desirable to add support for other linear algebra operations (e.g., > operations on matrices). The attached patches likely only scratch the > surface of the "vector search" use-case. > > The patch set is broken up as follows: > > * 0001 does some minor refactoring of dsqrt() in preparation for 0002. > * 0002 adds several vector-related functions, including distance functions > and a kmeans++ implementation. > * 0003 adds support for optionally using the OpenBLAS library, which is an > implementation of the Basic Linear Algebra Subprograms [2] > specification. Basic testing with this library showed a small > performance boost, although perhaps not enough to justify giving this > patch serious consideration. > > Of course, there are many open questions. For example, should PostgreSQL > support this stuff out-of-the-box in the first place? And should we > introduce a vector data type or SQL domains for treating float8[] as > vectors? IMHO these vector search use-cases are an exciting opportunity > for the PostgreSQL project, so I am eager to hear what folks think. > > [0] https://www.postgresql.org/docs/current/cube.html > [1] https://postgr.es/m/2271927.1593097400%40sss.pgh.pa.us > [2] https://en.wikipedia.org/wiki/Basic_Linear_Algebra_Subprograms > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com >
Re: Docs: Encourage strong server verification with SCRAM
On 5/25/23 1:29 PM, Stephen Frost wrote: Greetings, * Jacob Champion (jchamp...@timescale.com) wrote: On 5/24/23 05:04, Daniel Gustafsson wrote: On 23 May 2023, at 23:02, Stephen Frost wrote: Perhaps more succinctly- maybe we should be making adjustments to the current language instead of just adding a new paragraph. +1 I'm 100% on board for doing that as well, but the "instead of" suggestion makes me think I didn't explain my goal very well. For example, Stephen, you said I have to admit that the patch as presented strikes me as a warning without really providing steps for how to address the issues mentioned though; there's a reference to what was just covered at the bottom but nothing really new. but the last sentence of my patch *is* the suggested step: + ... It's recommended to employ strong server + authentication, using one of the above anti-spoofing measures, to prevent + these attacks. I was referring specifically to that ordering as not being ideal or in line with the rest of the flow of that section. We should integrate the concerns higher in the section where we outline the reason these things matter and then follow those with the specific steps for how to address them, not give a somewhat unclear reference from the very bottom back up to something above. Caught up on this exchange. Some of my comments may be out-of-order. Overall, +1 to tightening the language around the docs in this area. However, to paraphrase Stephen, I think the language, as currently written, makes the problem sound scarier than it actually is, and I agree that we should just inline it above. There may also be some follow-up development work we can do to mitigate the issues. I think it's fine for us to recommend using channel binding, but if we're concerned about server spoofing / rogue servers, we should also recommend using sslmode=verify-full to ensure server-identity. That doesn't necessarily help in the case the server itself has gone rogue, but that mitigates the MITM risk. The SCRAM RFC is also very clear that you should be using TLS[1]. I really don't think the "startup packet" is an actual issue, but I think recommending good TLS / channel binding mitigates this. For the iteration count, I'm generally ambivalent here. I think again, if you're using good TLS, this is most likely mitigated. If you're connecting to a rogue server using good TLS, you likely have other issues to deal with. However, there may be a libpq feature here that lets a client specify a minimum iteration count it will accept for purposes of SCRAM. Thanks, Jonathan [1] https://www.rfc-editor.org/rfc/rfc5802#section-9 OpenPGP_signature Description: OpenPGP digital signature
Re: Docs: Encourage strong server verification with SCRAM
On Thu, May 25, 2023 at 10:29 AM Stephen Frost wrote: > I was referring specifically to that ordering as not being ideal or in > line with the rest of the flow of that section. We should integrate the > concerns higher in the section where we outline the reason these things > matter and then follow those with the specific steps for how to address > them, not give a somewhat unclear reference from the very bottom back up > to something above. Ah, I misunderstood. I'll give that a shot. Thanks! --Jacob
Re: Implement generalized sub routine find_in_log for tap test
vignesh C writes:
> Hi,
>
> The recovery tap test has 4 implementations of find_in_log sub routine
> for various uses, I felt we can generalize these and have a single
> function for the same. The attached patch is an attempt to have a
> generalized sub routine find_in_log which can be used by all of them.
> Thoughts?
+1 on factoring out this common code. Just a few comments on the implementation.
> diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm
> b/src/test/perl/PostgreSQL/Test/Utils.pm
> index a27fac83d2..5c9b2f6c03 100644
> --- a/src/test/perl/PostgreSQL/Test/Utils.pm
> +++ b/src/test/perl/PostgreSQL/Test/Utils.pm
> @@ -67,6 +67,7 @@ our @EXPORT = qw(
>slurp_file
>append_to_file
>string_replace_file
> + find_in_log
>check_mode_recursive
>chmod_recursive
>check_pg_config
> @@ -579,6 +580,28 @@ sub string_replace_file
>
> =pod
>
> +
> +=item find_in_log(node, pattern, offset)
> +
> +Find pattern in logfile of node after offset byte.
> +
> +=cut
> +
> +sub find_in_log
> +{
> + my ($node, $pattern, $offset) = @_;
> +
> + $offset = 0 unless defined $offset;
> + my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile);
Since this function is in the same package, there's no need to qualify
it with the full name. I know the callers you copied it from did, but
they wouldn't have had to either, since it's exported by default (in the
@EXPORT array above), unless the use statement has an explicit argument
list that excludes it.
> + return 0 if (length($log) <= 0 || length($log) <= $offset);
> +
> + $log = substr($log, $offset);
Also, the existing callers don't seem to have got the memo that
slurp_file() takes an optinal offset parameter, which will cause it to
seek to that postion before slurping the file, which is more efficient
than reading the whole file in and substr-ing it. There's not much
point in the length checks either, since regex-matching against an empty
string is very cheap (and if the provide pattern can match the empty
string the whole function call is rather pointless).
> + return $log =~ m/$pattern/;
> +}
All in all, it could be simplified to:
sub find_in_log {
my ($node, $pattern, $offset) = @_;
return slurp_file($node->logfile, $offset) =~ $pattern;
}
However, none of the other functions in ::Utils know anything about node
objects, which makes me think it should be a method on the node itself
(i.e. in PostgreSQL::Test::Cluster) instead. Also, I think log_contains
would be a better name, since it just returns a boolean. The name
find_in_log makes me think it would return the log lines matching the
pattern, or the position of the match in the file.
In that case, the slurp_file() call would have to be fully qualified,
since ::Cluster uses an empty import list to avoid polluting the method
namespace with imported functions.
- ilmari
Re: Docs: Encourage strong server verification with SCRAM
Greetings, * Jacob Champion (jchamp...@timescale.com) wrote: > On 5/24/23 05:04, Daniel Gustafsson wrote: > >> On 23 May 2023, at 23:02, Stephen Frost wrote: > >> Perhaps more succinctly- maybe we should be making adjustments to the > >> current language instead of just adding a new paragraph. > > > > +1 > > I'm 100% on board for doing that as well, but the "instead of" > suggestion makes me think I didn't explain my goal very well. For > example, Stephen, you said > > > I have to admit that the patch as presented strikes me as a warning > > without really providing steps for how to address the issues mentioned > > though; there's a reference to what was just covered at the bottom but > > nothing really new. > > but the last sentence of my patch *is* the suggested step: > > > + ... It's recommended to employ strong server > > + authentication, using one of the above anti-spoofing measures, to > > prevent > > + these attacks. I was referring specifically to that ordering as not being ideal or in line with the rest of the flow of that section. We should integrate the concerns higher in the section where we outline the reason these things matter and then follow those with the specific steps for how to address them, not give a somewhat unclear reference from the very bottom back up to something above. Thanks, Stephen signature.asc Description: PGP signature
Re: Large files for relations
Greetings, * Peter Eisentraut (peter.eisentr...@enterprisedb.com) wrote: > On 24.05.23 02:34, Thomas Munro wrote: > > Thanks all for the feedback. It was a nice idea and it *almost* > > works, but it seems like we just can't drop segmented mode. And the > > automatic transition schemes I showed don't make much sense without > > that goal. > > > > What I'm hearing is that something simple like this might be more > > acceptable: > > > > * initdb --rel-segsize (cf --wal-segsize), default unchanged > > makes sense Agreed, this seems alright in general. Having more initdb-time options to help with certain use-cases rather than having things be compile-time is definitely just generally speaking a good direction to be going in, imv. > > * pg_upgrade would convert if source and target don't match > > This would be good, but it could also be an optional or later feature. Agreed. > Maybe that should be a different mode, like --copy-and-adjust-as-necessary, > so that users would have to opt into what would presumably be slower than > plain --copy, rather than being surprised by it, if they unwittingly used > incompatible initdb options. I'm curious as to why it would be slower than a regular copy..? > > I would probably also leave out those Windows file API changes, too. > > --rel-segsize would simply refuse larger sizes until someone does the > > work on that platform, to keep the initial proposal small. > > Those changes from off_t to pgoff_t? Yes, it would be good to do without > those. Apart of the practical problems that have been brought up, this was > a major annoyance with the proposed patch set IMO. > > > I would probably leave the experimental copy_on_write() ideas out too, > > for separate discussion in a separate proposal. > > right You mean copy_file_range() here, right? Shouldn't we just add support for that today into pg_upgrade, independently of this? Seems like a worthwhile improvement even without the benefit it would provide to changing segment sizes. Thanks, Stephen signature.asc Description: PGP signature
Re: Why does pg_bsd_indent need to be installed?
Andres Freund writes: > On 2023-05-25 09:09:45 -0400, Tom Lane wrote: >> Peter Eisentraut writes: >>> Why does pgindent require that pg_bsd_indent be installed in the path? > Isn't the situation actually *easier* in VPATH builds? There's no build > artifacts in the source tree, so you can just invoke the pg_bsd_indent built > in the build directory against the source tree, without any problems? Well, if you know where the build directory is, sure. But any way you slice it there is an extra bit of knowledge required. Since pg_bsd_indent changes so seldom, keeping it in your PATH is at least as easy as any other solution, IMO. Another reason why I like to do it that way is that it supports running pgindent on files that aren't in the source tree at all, which suits some old habits of mine. But, as I said before, I'm open to adding support for other scenarios as long as we don't remove that one. regards, tom lane
Re: Docs: Encourage strong server verification with SCRAM
On 5/23/23 21:37, Michael Paquier wrote: > On Tue, May 23, 2023 at 10:01:03PM -0400, Stephen Frost wrote: >> To the extent that there was an issue when it was implemented ... it's >> now been implemented and so that was presumably overcome (though I don't >> really specifically recall what the issues were there? Seems like it >> wouldn't matter at this point though), so I don't understand why we >> would wish to revisit it. > > Well, there are IMO two sides issues here: > 1) libpq sends an empty username, which can be an issue if attempting > to make this layer more pluggable with other things that are more > compilation than PG with the SCRAM exchange (OpenLDAP is one, there > could be others). > 2) The backend ignoring the username means that it is not mixed in the > hashes. +1 > Relying on channel binding mitigates the range of problems for 2), (Except for username tampering with possession of the server key alone. As spec'd, that shouldn't be possible. But for the vast majority of users, I think that's probably not on the list of plausible concerns.) > now > one thing is that the default sslmode does not enforce an SSL > connection, either, though this default has been discussed a lot. So > I am really wondering whether there wouldn't be some room for a more > compliant, strict HBA option with scram-sha-256 where we'd only allow > UTF-8 compliant strings. Just some food for thoughts. > > And we could do better about the current state that's 1), anyway. I would definitely like to see improvements in this area. I don't think we'd need to break compatibility with clients, either (unless the server operator explicitly wanted to). The hard part is mainly on the server side, when dealing with a mismatch between the startup packet and the SCRAM header. --Jacob
Re: Why does pg_bsd_indent need to be installed?
Hi, On 2023-05-25 09:09:45 -0400, Tom Lane wrote: > Peter Eisentraut writes: > > Why does pgindent require that pg_bsd_indent be installed in the path? > > Couldn't pgindent call the pg_bsd_indent built inside the tree? That > > would make the whole process much smoother. > > Well, the current expectation is that you run it in a distclean'd > tree, in which case it won't be there. VPATH builds would have > a problem finding it as well. > > I'm not sure if there's any problem in applying it in a built-out > tree, but the VPATH scenario seems like a problem in any case, > especially since (IIUC) meson builds have to be done that way. Isn't the situation actually *easier* in VPATH builds? There's no build artifacts in the source tree, so you can just invoke the pg_bsd_indent built in the build directory against the source tree, without any problems? I'd like to add a build system target for reindenting with the in-tree pg_bsd_indent, obviously with the right dependencies to build pg_bsd_indent first. And yes, meson only supports building in a separate directory (which obviously can be inside the source directory, although I don't do that, because the autoconf vpath build copies such directories, which isn't fun). Greetings, Andres Freund
Re: Docs: Encourage strong server verification with SCRAM
On 5/24/23 05:04, Daniel Gustafsson wrote: >> On 23 May 2023, at 23:02, Stephen Frost wrote: >> * Jacob Champion (jchamp...@timescale.com) wrote: > >>> - low iteration counts accepted by the client make it easier than it >>> probably should be for a MITM to brute-force passwords (note that >>> PG16's scram_iterations GUC, being server-side, does not mitigate >>> this) >> >> This would be good to improve on. > > The mechanics of this are quite straighforward, the problem IMHO lies in how > to > inform and educate users what a reasonable iteration count is, not to mention > what an iteration count is in the first place. Yeah, the education around this is tricky. >> Perhaps more succinctly- maybe we should be making adjustments to the >> current language instead of just adding a new paragraph. > > +1 I'm 100% on board for doing that as well, but the "instead of" suggestion makes me think I didn't explain my goal very well. For example, Stephen, you said > I have to admit that the patch as presented strikes me as a warning > without really providing steps for how to address the issues mentioned > though; there's a reference to what was just covered at the bottom but > nothing really new. but the last sentence of my patch *is* the suggested step: > + ... It's recommended to employ strong server > + authentication, using one of the above anti-spoofing measures, to > prevent > + these attacks. In other words: if you're thinking about authenticating the server via SCRAM, over an otherwise anonymous key exchange, you should probably reconsider. The current architecture of libpq doesn't really seem to be set up for this, and my conversations with security@ have been focusing around the argument that people who want strong guarantees should be authenticating the server using a TLS certificate before doing anything else, so we don't need to consider our departures from the spec to be vulnerabilities. I think that's a reasonable stance to take, as long as we're also explicitly warning people away from the mutual-auth use case. To change this, aspects of the code that we don't currently consider security problems would probably need to be reclassified. If we're depending on SCRAM for server authentication, we can't trust a single byte that the server sends to us until after the SCRAM verifier comes back and checks out. Compared to all the other authentication types, that's unusual; I don't think it's really been a focus for the project before. --Jacob
Re: Allow pg_archivecleanup to remove backup history files
On 2023-05-24 10:26, Michael Paquier wrote:
On Mon, May 22, 2023 at 06:24:49PM +0900, torikoshia wrote:
Thanks for your advice, attached patches.
0001 looks OK, thanks!
+Remove files including backup history file.
This could be reworded as "Remove backup history files.", I assume.
+ Note that when oldestkeptwalfile
is a backup history file,
+ specified file is kept and only preceding WAL files and
backup history files are removed.
The same thing is described at the bottom of the documentation, so it
does not seem necessary here.
- printf(_(" -d, --debug generate debug output
(verbose mode)\n"));
- printf(_(" -n, --dry-run dry run, show the names
of the files that would be removed\n"));
- printf(_(" -V, --version output version
information, then exit\n"));
- printf(_(" -x --strip-extension=EXT clean up files if they
have this extension\n"));
- printf(_(" -?, --helpshow this help, then
exit\n"));
+ printf(_(" -d, --debug generate debug output
(verbose mode)\n"));
+ printf(_(" -n, --dry-run dry run, show the
names of the files that would be removed\n"));
+ printf(_(" -b, --clean-backup-history clean up files
including backup history files\n"));
+ printf(_(" -V, --version output version
information, then exit\n"));
+ printf(_(" -x --strip-extension=EXTclean up files if they
have this extension\n"));
+ printf(_(" -?, --help show this help, then
exit\n"));
Perhaps this indentation had better be adjusted in 0001, no big deal
either way.
- ok(!-f "$tempdir/$walfiles[0]",
- "$test_name: first older WAL file was cleaned up");
+ if (grep {$_ eq '-x.gz'} @options) {
+ ok(!-f "$tempdir/$walfiles[0]",
+ "$test_name: first older WAL file with .gz was
cleaned up");
+ } else {
+ ok(-f "$tempdir/$walfiles[0]",
+ "$test_name: first older WAL file with .gz was
not cleaned up");
+ }
[...]
- ok(-f "$tempdir/$walfiles[2]",
- "$test_name: restartfile was not cleaned up");
+ if (grep {$_ eq '-b'} @options) {
+ ok(!-f "$tempdir/$walfiles[2]",
+ "$test_name: Backup history file was cleaned
up");
+ } else {
+ ok(-f "$tempdir/$walfiles[2]",
+ "$test_name: Backup history file was not
cleaned up");
+ }
That's over-complicated, caused by the fact that all the tests rely on
the same list of WAL files to create, aka @walfiles defined at the
beginning of the script. Wouldn't it be cleaner to handle the fake
file creations with more than one global list, before each command
run? The list of files to be created could just be passed down as an
argument of run_check(), for example, before cleaning up the contents
for the next command.
--
Michael
Thanks for reviewing!
Updated patches according to your comment.
--
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATIONFrom 024b54d5d436bb88bb80bf6b316f697eb96b53f1 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Thu, 25 May 2023 23:13:12 +0900
Subject: [PATCH v4 1/2] Introduce pg_archivecleanup into getopt_long
This patch is a preliminary step to add an easy-to-understand option
to delete backup history files, but it also adds long options to
the existing options.
---
doc/src/sgml/ref/pgarchivecleanup.sgml| 5 -
src/bin/pg_archivecleanup/pg_archivecleanup.c | 20 ---
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/pgarchivecleanup.sgml b/doc/src/sgml/ref/pgarchivecleanup.sgml
index 635e7c7685..09991c2fcd 100644
--- a/doc/src/sgml/ref/pgarchivecleanup.sgml
+++ b/doc/src/sgml/ref/pgarchivecleanup.sgml
@@ -95,6 +95,7 @@ pg_archivecleanup: removing file "archive/00010037000E"
-d
+ --debug
Print lots of debug logging output on stderr.
@@ -104,6 +105,7 @@ pg_archivecleanup: removing file "archive/00010037000E"
-n
+ --dry-run
Print the names of the files that would have been removed on stdout (performs a dry run).
@@ -122,7 +124,8 @@ pg_archivecleanup: removing file "archive/00010037000E"
- -x extension
+ -x extension
+ --strip-extension=extension
Provide an extension
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 7726d05149..707d7d54cf 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -17,7 +17,7 @@
#include "access/xlog_internal.h"
#include "common/logging.h"
-#include "pg_getopt.h"
+#include "getopt_long.h"
const char *progname;
@@ -252,11 +252,11 @@ usage(v
Re: testing dist tarballs
Re: Andres Freund > That's due to MAKELEVEL: > > submake-generated-headers: > ifndef NO_GENERATED_HEADERS > ifeq ($(MAKELEVEL),0) > $(MAKE) -C $(top_builddir)/src/backend generated-headers > endif > endif > > So the distcheck target needs to reset MAKELEVEL=0 - unless somebody has a > better idea? Fwiw, I've had that problem as well in the Debian packages where debian/rules is already a Makefile and calling $(MAKE) from there trips up that logic. The workaround I used is: override_dh_auto_build-arch: # set MAKELEVEL to 0 to force building submake-generated-headers in src/Makefile.global(.in) MAKELEVEL=0 $(MAKE) -C build/src all ... override_dh_auto_test-arch: ifeq (, $(findstring nocheck, $(DEB_BUILD_OPTIONS))) # when tests fail, print newest log files # initdb doesn't like LANG and LC_ALL to contradict, unset LANG and LC_CTYPE here # temp-install wants to be invoked from a top-level make, unset MAKELEVEL here # tell pg_upgrade to create its sockets in /tmp to avoid too long paths unset LANG LC_CTYPE MAKELEVEL; ulimit -c unlimited; \ if ! make -C build check-world \ $(TEMP_CONFIG) \ PGSOCKETDIR="/tmp" \ PG_TEST_EXTRA='ssl' \ PROVE_FLAGS="--verbose"; \ ... (Just mentioning this, not asking it to be changed.) Re: Tom Lane > Andres Freund writes: > > First thing I noticed that 'make dist' doesn't work in a vpath, failing in a > > somewhat obscure way (likely because in a vpath build the the copy from the > > source dir doesn't include GNUMakefile). Do we expect it to work? > > Don't see how it could possibly be useful in a vpath, because you'd have > the real source files and the generated files in different trees. I don't think "make dist" is generally expected to work in vpath builds, that's probably one indirection layer too much. (The "make distcheck" rule generated by automake tests vpath builds, though.) Christoph
Re: memory leak in trigger handling (since PG12)
On 5/24/23 22:19, Andres Freund wrote: > > ... > > Hm - stepping back a bit, why are we doing the work in ExecGetAllUpdatedCols() > over and over? Unless I am missing something, the result doesn't change > across rows. And it doesn't look that cheap to compute, leaving aside the > allocation that bms_union() does. > > It's visible in profiles, not as a top entry, but still. > > Perhaps the easiest to backpatch fix is to just avoid recomputing the value? > But perhaps it'd be just as problmeatic, because callers might modify > ExecGetAllUpdatedCols()'s return value in place... > Yes, I think that's a perfectly valid point - I was actually wondering about that too, but I was focused on fixing this in backbranches so I left this as a future optimization (assuming it can be cached). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: memory leak in trigger handling (since PG12)
On 5/24/23 21:49, Tomas Vondra wrote:
>
>
> On 5/24/23 20:55, Andres Freund wrote:
>> Hi,
>>
>> On 2023-05-24 15:38:41 +0200, Tomas Vondra wrote:
>>> I looked at this again, and I think GetPerTupleMemoryContext(estate)
>>> might do the trick, see the 0002 part.
>>
>> Yea, that seems like the right thing here.
>>
>>
>>> Unfortunately it's not much
>>> smaller/simpler than just freeing the chunks, because we end up doing
>>>
>>> oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
>>> updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
>>> MemoryContextSwitchTo(oldcxt);
>>
>> We could add a variant of ExecGetAllUpdatedCols that switches the context.
>>
>
> Yeah, we could do that. I was thinking about backpatching, and modifying
> ExecGetAllUpdatedCols signature would be ABI break, but adding a
> variant should be fine.
>
>>
>>> and then have to pass updatedCols elsewhere. It's tricky to just switch
>>> to the context (e.g. in ExecASUpdateTriggers/ExecARUpdateTriggers), as
>>> AfterTriggerSaveEvent allocates other bits of memory too (in a longer
>>> lived context).
>>
>> Hm - on a quick look the allocations in trigger.c itself are done with
>> MemoryContextAlloc().
>>
>> I did find a problematic path, namely that ExecGetChildToRootMap() ends up
>> building resultRelInfo->ri_ChildToRootMap in CurrentMemoryContext.
>>
>> That seems like a flat out bug to me - we can't just store data in a
>> ResultRelInfo without ensuring the memory is lives long enough. Nearby places
>> like ExecGetRootToChildMap() do make sure to switch to es_query_cxt.
>>
>>
>> Did you see other bits of memory getting allocated in CurrentMemoryContext?
>>
>
> No, I simply tried to do the context switch and then gave up when it
> crashed on the ExecGetRootToChildMap info. I haven't looked much
> further, but you may be right it's the only bit.
>
> It didn't occur to me it might be a bug - I think the code simply
> assumes it gets called with suitable memory context, just like we do in
> various other places. Maybe it should document the assumption.
>
>>
>>> So we'd have to do another switch again. Not sure how
>>> backpatch-friendly would that be.
>>
>> Yea, that's a valid concern. I think it might be reasonable to use something
>> like ExecGetAllUpdatedColsCtx() in the backbranches, and switch to a
>> short-lived context for the trigger invocations in >= 16.
>>
>
The attached patch does this - I realized we actually have estate in
ExecGetAllUpdatedCols(), so we don't even need a variant with a
different signature. That makes the patch much simpler.
The question is whether we need the signature anyway. There might be a
caller expecting the result to be in CurrentMemoryContext (be it
ExecutorState or something else), and this change might break it. But
I'm not aware of any callers, so maybe that's fine.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 9a6eccebaea969eb5d2e15de99aaabb40b02499c Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Tue, 23 May 2023 17:45:47 +0200
Subject: [PATCH] Use per-tuple context in ExecGetAllUpdatedCols
---
src/backend/executor/execUtils.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 25aba9a243a..37f2fa173f6 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -1349,8 +1349,16 @@ ExecGetExtraUpdatedCols(ResultRelInfo *relinfo, EState *estate)
Bitmapset *
ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate)
{
- return bms_union(ExecGetUpdatedCols(relinfo, estate),
- ExecGetExtraUpdatedCols(relinfo, estate));
+ Bitmapset *ret;
+ MemoryContext oldcxt
+ = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+
+ ret = bms_union(ExecGetUpdatedCols(relinfo, estate),
+ ExecGetExtraUpdatedCols(relinfo, estate));
+
+ MemoryContextSwitchTo(oldcxt);
+
+ return ret;
}
/*
--
2.40.1
Re: PostgreSQL 16 Beta 1 release announcement draft
On 5/25/23 12:16 AM, Andres Freund wrote: Hi, On 2023-05-24 23:30:58 -0400, Jonathan S. Katz wrote: Ah, OK, that's why I didn't grok it. I read through the first message in[1] and definitely agree it should be in the announcement. How about: "PostgreSQL 16 also shows up to a 300% improvement when concurrently loading data with `COPY`" I currently have it as the below in the release announcement. If it you send any suggested updates, I can try to put them in before release: PostgreSQL 16 can also improve the performance of concurrent bulk loading of data using [`COPY`](https://www.postgresql.org/docs/16/sql-copy.html) up to a 300%. It also speeds up concurrent loading when not using COPY, just to a lesser degree. But I can't come up with a concise phrasing for that right now... I left as is (in part because of a hurried morning), but we can improve upon it for the GA. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: Wrong results due to missing quals
Richard Guo writes: > The "incompatible_relids" idea is a stroke of genius. I reviewed the > patch and did not find any problem. So big +1 to the patch. Pushed, thanks for the report and the review. regards, tom lane
Re: memory leak in trigger handling (since PG12)
On 5/24/23 22:22, Andres Freund wrote: > Hi, > > On 2023-05-24 21:56:22 +0200, Tomas Vondra wrote: The really hard thing was determining what causes the memory leak - the simple instrumentation doesn't help with that at all. It tells you there might be a leak, but you don't know where did the allocations came from. What I ended up doing is a simple gdb script that sets breakpoints on all palloc/pfree variants, and prints info (including the backtrace) for each call on ExecutorState. And then a script that aggregate those to identify which backtraces allocated most chunks that were not freed. >>> >>> FWIW, for things like this I found "heaptrack" to be extremely helpful. >>> >>> E.g. for a reproducer of the problem here, it gave me the attach "flame >>> graph" >>> of the peak memory usage - after attaching to a running backend and running >>> an >>> UPDATE triggering the leak.. >>> >>> Because the view I screenshotted shows the stacks contributing to peak >>> memory >>> usage, it works nicely to find "temporary leaks", even if memory is actually >>> freed after all etc. >>> >> >> That's a nice visualization, but isn't that useful only once you >> determine there's a memory leak? Which I think is the hard problem. > > So is your gdb approach, unless I am misunderstanding? The view I > screenshotted shows the "peak" allocated memory, if you have a potential leak, > you can see where most of the allocated memory was allocated. Which at least > provides you with a good idea of where to look for a problem in more detail. > Right, it wasn't my ambition to detect memory leaks but to source of the leak if there's one. I got a bit distracted by the discussion detecting leaks etc. > > Hm. Somehow this doesn't seem quite right. Shouldn't we try to use a > shorter > lived memory context instead? Otherwise we'll just end up with the same > problem in a few years. > I agree using a shorter lived memory context would be more elegant, and more in line with how we do things. But it's not clear to me why we'd end up with the same problem in a few years with what the patch does. >>> >>> Because it sets up the pattern of manual memory management and continues to >>> run the relevant code within a query-lifetime context. >>> >> >> Oh, you mean someone might add new allocations to this code (or into one >> of the functions executed from it), and that'd leak again? Yeah, true. > > Yes. It's certainly not obvious this far down that we are called in a > semi-long-lived memory context. > That's true, but I don't see how adding a ExecGetAllUpdatedCols() variant that allocates stuff in a short-lived context improves this. That'll only cover memory allocated in ExecGetAllUpdatedCols() and nothing else. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Implement generalized sub routine find_in_log for tap test
Hi,
The recovery tap test has 4 implementations of find_in_log sub routine
for various uses, I felt we can generalize these and have a single
function for the same. The attached patch is an attempt to have a
generalized sub routine find_in_log which can be used by all of them.
Thoughts?
Regards,
VIgnesh
From 7c4b6731a27c031ad27f6f88150c1dfaae1f46fe Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Thu, 25 May 2023 10:47:16 +0530
Subject: [PATCH] Remove duplicate find_in_log sub routines from tap tests.
Remove duplicate find_in_log sub routines from tap tests.
---
src/test/authentication/t/003_peer.pl | 15 ++--
src/test/perl/PostgreSQL/Test/Utils.pm| 23 +++
src/test/recovery/t/019_replslot_limit.pl | 14 ---
src/test/recovery/t/033_replay_tsp_drops.pl | 12 +-
.../t/035_standby_logical_decoding.pl | 14 ---
5 files changed, 26 insertions(+), 52 deletions(-)
diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl
index 3272e52cae..a8ff6e8642 100644
--- a/src/test/authentication/t/003_peer.pl
+++ b/src/test/authentication/t/003_peer.pl
@@ -69,17 +69,6 @@ sub test_role
}
}
-# Find $pattern in log file of $node.
-sub find_in_log
-{
- my ($node, $offset, $pattern) = @_;
-
- my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile, $offset);
- return 0 if (length($log) <= 0);
-
- return $log =~ m/$pattern/;
-}
-
my $node = PostgreSQL::Test::Cluster->new('node');
$node->init;
$node->append_conf('postgresql.conf', "log_connections = on\n");
@@ -92,8 +81,8 @@ reset_pg_hba($node, 'peer');
my $log_offset = -s $node->logfile;
$node->psql('postgres');
if (find_in_log(
- $node, $log_offset,
- qr/peer authentication is not supported on this platform/))
+ $node, qr/peer authentication is not supported on this platform/),
+ $log_offset,)
{
plan skip_all => 'peer authentication is not supported on this platform';
}
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index a27fac83d2..5c9b2f6c03 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -67,6 +67,7 @@ our @EXPORT = qw(
slurp_file
append_to_file
string_replace_file
+ find_in_log
check_mode_recursive
chmod_recursive
check_pg_config
@@ -579,6 +580,28 @@ sub string_replace_file
=pod
+
+=item find_in_log(node, pattern, offset)
+
+Find pattern in logfile of node after offset byte.
+
+=cut
+
+sub find_in_log
+{
+ my ($node, $pattern, $offset) = @_;
+
+ $offset = 0 unless defined $offset;
+ my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile);
+
+ return 0 if (length($log) <= 0 || length($log) <= $offset);
+
+ $log = substr($log, $offset);
+ return $log =~ m/$pattern/;
+}
+
+=pod
+
=item check_mode_recursive(dir, expected_dir_mode, expected_file_mode, ignore_list)
Check that all file/dir modes in a directory match the expected values,
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index a1aba16e14..ebd15a0ec4 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -446,18 +446,4 @@ sub get_log_size
return (stat $node->logfile)[7];
}
-# find $pat in logfile of $node after $off-th byte
-sub find_in_log
-{
- my ($node, $pat, $off) = @_;
-
- $off = 0 unless defined $off;
- my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile);
- return 0 if (length($log) <= $off);
-
- $log = substr($log, $off);
-
- return $log =~ m/$pat/;
-}
-
done_testing();
diff --git a/src/test/recovery/t/033_replay_tsp_drops.pl b/src/test/recovery/t/033_replay_tsp_drops.pl
index 0a35a7bda6..5f1287cd90 100644
--- a/src/test/recovery/t/033_replay_tsp_drops.pl
+++ b/src/test/recovery/t/033_replay_tsp_drops.pl
@@ -143,14 +143,4 @@ while ($max_attempts-- >= 0)
}
ok($max_attempts > 0, "invalid directory creation is detected");
-done_testing();
-
-# find $pat in logfile of $node after $off-th byte
-sub find_in_log
-{
- my ($node, $pat, $off) = @_;
-
- my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile, $off);
-
- return $log =~ m/$pat/;
-}
+done_testing();
\ No newline at end of file
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl
index 64beec4bd3..31e347ce90 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -28,20 +28,6 @@ my $res;
my $primary_slotname = 'primary_physical';
my $standby_physical_slotname = 'standby_physical';
-# find $pat in logfile of $node after $off-th byte
-sub find_in_log
-{
- my ($node, $pat, $off) = @_;
-
- $off = 0 unless defined $off;
- my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile);
- return 0 if (length($log) <= $off);
-
- $log = substr($log, $off);
-
- return $log =~ m/$pat/;
-}
-
# Fetch xmin columns from slot's pg_replication_slot
Re: Adding SHOW CREATE TABLE
On Mon, 22 May 2023 at 13:52, Andrew Dunstan wrote: > A performant server side set of functions would be written in C and follow > the patterns in ruleutils.c. We have lots of DDL ruleutils in our Citus codebase: https://github.com/citusdata/citus/blob/main/src/backend/distributed/deparser/citus_ruleutils.c I'm pretty sure we'd be happy to upstream those if that meant, we wouldn't have to update them for every postgres release. We also have the master_get_table_ddl_events UDF, which does what SHOW CREATE TABLE would do.
Re: pgindent vs. pgperltidy command-line arguments
Peter Eisentraut writes: > Until PG15, calling pgindent without arguments would process the whole > tree. Now you get > No files to process at ./src/tools/pgindent/pgindent line 372. > Is that intentional? It was intentional, cf b16259b3c and the linked discussion. > Also, pgperltidy accepts no arguments and always processes the whole > tree. It would be nice if there were a way to process individual files > or directories, like pgindent can. +1, although I wonder if we shouldn't follow pgindent's new lead and require some argument(s). > Attached is a patch for this. > (It seems that it works ok to pass regular files (not directories) to > "find", but I'm not sure if it's portable.) The POSIX spec for find(1) gives an example of applying find to what they evidently intend to be a plain file: if [ -n "$(find file1 -prune -newer file2)" ]; then printf %s\\n "file1 is newer than file2" fi So while I don't see it written in so many words, I think you can assume it's portable. regards, tom lane
Re: Why does pg_bsd_indent need to be installed?
Peter Eisentraut writes: > Why does pgindent require that pg_bsd_indent be installed in the path? > Couldn't pgindent call the pg_bsd_indent built inside the tree? That > would make the whole process much smoother. Well, the current expectation is that you run it in a distclean'd tree, in which case it won't be there. VPATH builds would have a problem finding it as well. I'm not sure if there's any problem in applying it in a built-out tree, but the VPATH scenario seems like a problem in any case, especially since (IIUC) meson builds have to be done that way. I wouldn't object to adding more logic to the calling script to support multiple usage scenarios, of course. regards, tom lane
Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error
On 2023-May-22, Alvaro Herrera wrote: > Hah, yeah, that's because an empty pipeline never calls the code to > allocate the flag array. Here's the trivial fix. Pushed to both branches, thanks for the report. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: Removing unneeded self joins
On 3/6/23 10:30, Michał Kłeczek wrote:
> Hi All,
>
> I just wanted to ask about the status and plans for this patch.
> I can see it being stuck at “Waiting for Author” status in several
> commit tests.
>
> I think this patch would be really beneficial for us as we heavily use
> views to structure out code.
> Each view is responsible for providing some calculated values and
they > are joined in a query to retrieve the full set of information.
>
> Not sure how the process works and how I could help (I am absolutely
> not capable of helping with coding I am afraid - but could sponsor a
> (small :) ) bounty to speed things up).
Yes, I am still working on this feature. Because of significant changes
in the optimizer code which Tom & Richard had been doing last months, I
didn't touch it for a while. But now this work can be continued.
Current patch is rebased on current master. Because of the nullable_rels
logic, introduced recently, ojrelids were highly spreaded across planner
bitmapsets. So, JE logic was changed.
But now, I'm less happy with the code. It seems we need to refactor it:
1. According to reports of some performance engineers, the feature can
cause overhead ~0.5% on trivial queries without joins at all. We should
discover the patch and find the way for quick and cheap return, if the
statement contains no one join or, maybe stronger, no one self join.
2. During join elimination we replace clauses like 'x=x' with 'x IS NOT
NULL'. It is a weak point because we change clause semantic
(mergejoinable to non-mergejoinable, in this example) and could forget
consistently change some RestrictInfo fields.
3. In the previous versions we changed the remove_rel_from_query routine
trying to use it in both 'Useless outer join' and 'Self join'
elimination optimizations. Now, because of the 'ojrelid' field it looks
too complicated. Do we need to split this routine again?
--
Regards
Andrey Lepikhov
Postgres Professional
From cb4340577dab0e8cf5531e9934f5734fda178490 Mon Sep 17 00:00:00 2001
From: Andrey Lepikhov
Date: Mon, 15 May 2023 09:04:51 +0500
Subject: [PATCH] Remove self-joins.
A Self Join Elimination (SJE) feature removes inner join of plain table to itself
in a query tree if can be proved that the join can be replaced with a scan.
The proof based on innerrel_is_unique machinery.
We can remove a self-join when for each outer row:
1. At most one inner row matches the join clause.
2. If the join target list contains any inner vars, an inner row
must be (physically) the same row as the outer one.
In this patch we use the next approach to identify a self-join:
1. Collect all mergejoinable join quals which look like a.x = b.x
2. Check innerrel_is_unique() for the qual list from (1). If it
returns true, then outer row matches only the same row from the inner
relation.
3. If uniqueness of outer relation is deduced from baserestrictinfo clause like
'x=const' on unique field(s), check what the inner has the same clause with the
same constant(s).
4. Compare RowMarks of inner and outer relations. They must have the same
strength.
Some regression tests changed due to self-join removal logic.
---
doc/src/sgml/config.sgml | 16 +
src/backend/optimizer/path/indxpath.c | 38 +
src/backend/optimizer/plan/analyzejoins.c | 1093 -
src/backend/optimizer/plan/planmain.c |5 +
src/backend/utils/misc/guc_tables.c | 22 +
src/include/optimizer/paths.h |3 +
src/include/optimizer/planmain.h |7 +
src/test/regress/expected/equivclass.out | 32 +
src/test/regress/expected/join.out| 774 +++
src/test/regress/expected/sysviews.out|3 +-
src/test/regress/sql/equivclass.sql | 16 +
src/test/regress/sql/join.sql | 340 +++
src/tools/pgindent/typedefs.list |2 +
13 files changed, 2313 insertions(+), 38 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 5da74b3c40..b47d11759e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5437,6 +5437,22 @@ ANY num_sync (
+ enable_self_join_removal (boolean)
+
+ enable_self_join_removal configuration parameter
+
+
+
+
+ Enables or disables the query planner's optimization which analyses
+query tree and replaces self joins with semantically equivalent single
+scans. Take into consideration only plain tables.
+The default is on.
+
+
+
+
enable_seqscan (boolean)
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 1436dbc2f2..1b1aa9083c 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -3491,6 +3491,21 @@ bool
relation_has_unique_index_for(PlannerInfo *root, RelOptInfo *rel,
List *restrictlist,
List *exprlist, List *oprlist)
+{
+ return relation_ha
Re: pgindent vs. pgperltidy command-line arguments
> On 25 May 2023, at 11:10, Peter Eisentraut wrote: > Also, pgperltidy accepts no arguments and always processes the whole tree. > It would be nice if there were a way to process individual files or > directories, like pgindent can. +1, thanks! I've wanted that several times but never gotten around to doing anything about it. -- Daniel Gustafsson
Re: pg_walinspect last query typo
> On 25 May 2023, at 11:13, jian he wrote:
>
> hi.
> https://www.postgresql.org/docs/current/pgwalinspect.html
>
> last query should be:
> SELECT * FROM pg_get_wal_stats('0/1E847D00', '0/1E84F500')
> WHERE count > 0 AND
> "resource_manager/record_type" = 'Transaction'
> LIMIT 1;
Nice catch, the LIMIT 1 has indeed landed in the wrong place. Will fix.
--
Daniel Gustafsson
pg_walinspect last query typo
hi.
https://www.postgresql.org/docs/current/pgwalinspect.html
last query should be:
SELECT * FROM pg_get_wal_stats('0/1E847D00', '0/1E84F500')
WHERE count > 0 AND
"resource_manager/record_type" = 'Transaction'
LIMIT 1;
pgindent vs. pgperltidy command-line arguments
Until PG15, calling pgindent without arguments would process the whole
tree. Now you get
No files to process at ./src/tools/pgindent/pgindent line 372.
Is that intentional?
Also, pgperltidy accepts no arguments and always processes the whole
tree. It would be nice if there were a way to process individual files
or directories, like pgindent can.
Attached is a patch for this.
(It seems that it works ok to pass regular files (not directories) to
"find", but I'm not sure if it's portable.)From ef9d9cc052d77e1509ce18dc004ac0ab96903a13 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 25 May 2023 11:02:25 +0200
Subject: [PATCH] Allow passing files on command line of pgperltidy
---
src/tools/perlcheck/find_perl_files | 7 +--
src/tools/perlcheck/pgperlcritic| 2 +-
src/tools/perlcheck/pgperlsyncheck | 2 +-
src/tools/pgindent/pgperltidy | 2 +-
4 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/tools/perlcheck/find_perl_files
b/src/tools/perlcheck/find_perl_files
index fd99dab83b..ad15f6b47a 100644
--- a/src/tools/perlcheck/find_perl_files
+++ b/src/tools/perlcheck/find_perl_files
@@ -3,11 +3,14 @@
# shell function to find all perl files in the source tree
find_perl_files () {
+ if [ $# -eq 0 ]; then
+ set -- .
+ fi
{
# take all .pl and .pm files
- find . -type f -name '*.p[lm]' -print
+ find "$@" -type f -name '*.p[lm]' -print
# take executable files that file(1) thinks are perl files
- find . -type f -perm -100 -exec file {} \; -print |
+ find "$@" -type f -perm -100 -exec file {} \; -print |
egrep -i ':.*perl[0-9]*\>' |
cut -d: -f1
} | sort -u | grep -v '^\./\.git/'
diff --git a/src/tools/perlcheck/pgperlcritic b/src/tools/perlcheck/pgperlcritic
index 1c2f787580..2ec6f20de3 100755
--- a/src/tools/perlcheck/pgperlcritic
+++ b/src/tools/perlcheck/pgperlcritic
@@ -14,7 +14,7 @@ PERLCRITIC=${PERLCRITIC:-perlcritic}
. src/tools/perlcheck/find_perl_files
-find_perl_files | xargs $PERLCRITIC \
+find_perl_files "$@" | xargs $PERLCRITIC \
--quiet \
--program-extensions .pl \
--profile=src/tools/perlcheck/perlcriticrc
diff --git a/src/tools/perlcheck/pgperlsyncheck
b/src/tools/perlcheck/pgperlsyncheck
index 730f5927cd..da59c9727c 100755
--- a/src/tools/perlcheck/pgperlsyncheck
+++ b/src/tools/perlcheck/pgperlsyncheck
@@ -13,4 +13,4 @@ set -e
# for zsh
setopt shwordsplit 2>/dev/null || true
-find_perl_files | xargs -L 1 perl $INCLUDES -cw 2>&1 | grep -v OK
+find_perl_files "$@" | xargs -L 1 perl $INCLUDES -cw 2>&1 | grep -v OK
diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy
index 5e704119eb..6af27d21d5 100755
--- a/src/tools/pgindent/pgperltidy
+++ b/src/tools/pgindent/pgperltidy
@@ -9,4 +9,4 @@ PERLTIDY=${PERLTIDY:-perltidy}
. src/tools/perlcheck/find_perl_files
-find_perl_files | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc
+find_perl_files "$@" | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc
--
2.40.1
Logical Replication Conflict Resolution
Working with PostgreSQL Logical Replication is just great! It helps a lot doing real time replication for analytical purposes without using any other 3d party service. Although all these years working as a product architect of reporting I have noted a few requirements which are always a challenge and may help enhance logical replication even better. To the point: PostgreSQL15 Logical Replication is going to stop while there is a conflict on the destination database. On the other hand, PGLogical can handle the conflicts with more options like error, apply_remote, keep_local, last_update_wins, first_update_wins while streaming. I was thinking that it would be great to add those capabilities into Logical Replication during streaming and even better on snapshot if it is possible. This enhancement method is going to solve a lot of issues while there are hybrid solutions which are updating databases with SQL Scripts and Logical Replication. At the same time will make Logical Replication "more reliable" as it will not stop replicating lines in live environments when the decision of conflict is already decided and configured. In my case I am consolidating data from multiple erp system databases to a destination database for reporting purposes. All erps, have the same table schema as the destination database, source database has the tenant_id identifier in non primary keys but has replica identity index. Now there are scenarios where we may need to update manually the destination database using scripts which are having the ONCONFLICT statement, but during that time if a new record is inserted into the database and the batch statement finishes earlier than replication, the replication will find a conflict.
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
Hi, Peter Smith , 24 May 2023 Çar, 05:59 tarihinde şunu yazdı: > Hi, and thanks for the patch! It is an interesting idea. > > I have not yet fully read this thread, so below are only my first > impressions after looking at patch 0001. Sorry if some of these were > already discussed earlier. > > TBH the patch "reuse-workers" logic seemed more complicated than I had > imagined it might be. > If you mean patch 0001 by the patch "reuse-workers", most of the complexity comes with some refactoring to split apply worker and tablesync worker paths. [1] If you mean the whole patch set, then I believe it's because reusing replication slots also requires having a proper snapshot each time the worker moves to a new table. [2] > > 1. > IIUC with patch 0001, each/every tablesync worker (a.k.a. TSW) when it > finishes dealing with one table then goes looking to find if there is > some relation that it can process next. So now every TSW has a loop > where it will fight with every other available TSW over who will get > to process the next relation. > > Somehow this seems all backwards to me. Isn't it strange for the TSW > to be the one deciding what relation it would deal with next? > > IMO it seems more natural to simply return the finished TSW to some > kind of "pool" of available workers and the main Apply process can > just grab a TSW from that pool instead of launching a brand new one in > the existing function process_syncing_tables_for_apply(). Or, maybe > those "available" workers can be returned to a pool maintained within > the launcher.c code, which logicalrep_worker_launch() can draw from > instead of launching a whole new process? > > (I need to read the earlier posts to see if these options were already > discussed and rejected) > I think ([3]) relying on a single apply worker for the assignment of several tablesync workers might bring some overhead, it's possible that some tablesync workers wait in idle until the apply worker assigns them something. OTOH yes, the current approach makes tablesync workers race for a new table to sync. TBF both ways might be worth discussing/investigating more, before deciding which way to go. > 2. > AFAIK the thing that identifies a tablesync worker is the fact that > only TSW will have a 'relid'. > > But it feels very awkward to me to have a TSW marked as "available" > and yet that LogicalRepWorker must still have some OLD relid field > value lurking (otherwise it will forget that it is a "tablesync" > worker!). > > IMO perhaps it is time now to introduce some enum 'type' to the > LogicalRepWorker. Then an "in_use" type=TSW would have a valid 'relid' > whereas an "available" type=TSW would have relid == InvalidOid. > Hmm, relid will be immediately updated when the worker moves to a new table. And the time between finishing sync of a table and finding a new table to sync should be minimal. I'm not sure how having an old relid for such a small amount of time can do any harm. > 3. > Maybe I am mistaken, but it seems the benchmark results posted are > only using quite a small/default values for > "max_sync_workers_per_subscription", so I wondered how those results > are affected by increasing that GUC. I think having only very few > workers would cause more sequential processing, so conveniently the > effect of the patch avoiding re-launch might be seen in the best > possible light. OTOH, using more TSW in the first place might reduce > the overall tablesync time because the subscriber can do more work in > parallel. So I'm not quite sure what the goal is here. E.g. if the user doesn't care much about how long tablesync phase takes then there is maybe no > need for this patch at all. OTOH, I thought if a user does care about > the subscription startup time, won't those users be opting for a much > larger "max_sync_workers_per_subscription" in the first place? > Therefore shouldn't the benchmarking be using a larger number too? Regardless of how many tablesync workers there are, reusing workers will speed things up if a worker has a chance to sync more than one table. Increasing the number of tablesync workers, of course, improves the tablesync performance. But if it doesn't make 100% parallel ( meaning that # of sync workers != # of tables to sync), then reusing workers can bring an additional improvement. Here are some benchmarks similar to earlier, but with 100 tables and different number of workers: ++-+-+-++ || 2 workers | 4 workers | 6 workers | 8 workers | ++-+-+-++ | master | 2579.154 ms | 1383.153 ms | 1001.559 ms | 911.758 ms | ++-+-+-++ | patch | 1724.230 ms | 853.894 ms | 601.176 ms | 496.395 ms | ++-+-+-++ So yes, increasing the number of workers makes it faster. But reusing workers can still improve more.
AW: Proposal: Removing 32 bit support starting from PG17++
Tom Lane writes: >Hans Buschmann writes: >> This inspired me to propose dropping 32bit support for PostgreSQL starting >> with PG17. >I don't think this is a great idea. Even if Intel isn't interested, >there'll be plenty of 32-bit left in the lower end of the market >(think ARM, IoT, and so on). Hello Tom, Certainly there are many 32bit implementations out there in many markets. Therefore I spoke from inspiration by Intel's move, what does not implicitely indicate a preference of x86 architecture. Considering ARM (worlds most used ISA) I think we should focus on the use cases. All implementations in many various scenarios (keyboard controllers,SetTop-Boxes,TV sets, SSD/Hard-disk-controllers,BMC controllers and many more) are no real use cases for Postgres on ARM. The only relevant usage scenario at this time is in the datacenter when AARCH64 based CPU designs are used in servers. The most spreaded usage of ARM (nowadays almost 64bit), which is used dayly by biliion of people, are the Smartphones and tablets, which are completelely unsupported by Postgres! An officially supported access module for remote access to server-based database would bring Postgres to a much broader knowledge and use for many people. (why not provide an official libpq on IOS or Android to enable specialized client applications?) On the IoT side I have not much knowledge, perhaps you have a relevant example of a native 32bit server implementation in this area. But to my knowledge there is no special support in our code base yet (OS, File systems, storage, administration). For me the central question is: Are there many use cases for new PG server installations on 32bit starting with late 2024/2025? and not Do we provide support for NEW VERSIONS for aging architectures which themselves aren't used for new installations any more and mostly never are updated even to currently supported versions? Hans Buschmann
Re: running logical replication as the subscription owner
On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada wrote: > > On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote: > > > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada > > wrote: > > > > > > Thank you for updating the patch! Here are review comments: > > > > > > + /* > > > +* Make sure that the copy command runs as the table owner, unless > > > +* the user has opted out of that behaviour. > > > +*/ > > > + run_as_owner = MySubscription->runasowner; > > > + if (!run_as_owner) > > > + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); > > > + > > > /* Now do the initial data copy */ > > > PushActiveSnapshot(GetTransactionSnapshot()); > > > > > > I think we should switch users before the acl check in > > > LogicalRepSyncTableStart(). > > > > > > > Agreed, we should check acl with the user that is going to perform > > operations on the target table. BTW, is it okay to perform an > > operation on the system table with the changed user as that would be > > possible with your suggestion (see replorigin_create())? > > Do you see any problem in particular? > > As per the documentation, pg_replication_origin_create() is only > allowed to the superuser by default, but in CreateSubscription() a > non-superuser (who has pg_create_subscription privilege) can call > replorigin_create(). Nothing in particular but it seems a bit odd to perform operations on catalog tables with some other user table owners when that was not the actual intent of this option. > OTOH, we don't necessarily need to switch to the > table owner user for checking ACL and RLS. We can just pass either > table owner OID or subscription owner OID to pg_class_aclcheck() and > check_enable_rls() without actually switching the user. > I think that would be better. -- With Regards, Amit Kapila.
Why does pg_bsd_indent need to be installed?
Why does pgindent require that pg_bsd_indent be installed in the path? Couldn't pgindent call the pg_bsd_indent built inside the tree? That would make the whole process much smoother.
Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?
> On 24 May 2023, at 16:15, Tom Lane wrote: > We had definitely better have some animals still using 1.0.2, but > I don't see much reason to think that the last public release > wouldn't be good enough. There are still RHEL7 animals like chub who use 1.0.2 so I'm not worried. We might want to consider displaying the OpenSSL version number during configure (meson already does it) in all branches to make it easier to figure out which versions we have coverage for? -- Daniel Gustafsson
Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?
> On 25 May 2023, at 00:18, Michael Paquier wrote: > > On Wed, May 24, 2023 at 01:03:04PM +0200, Daniel Gustafsson wrote: >> When we moved the goalposts to 1.0.1 (commit 7b283d0e1d1) we referred to >> RHEL6 >> using 1.0.1, and RHEL6 goes out of ELS in late June 2024 seems possible to >> drop >> 1.0.1 support during v17. I haven't studied the patch yet but I'll have a >> look >> at it. Patch looks to be in pretty good shape, just a few minor comments: -#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH +#ifdef USE_OPENSSL Since this is only calling the pgtls abstraction API and not directly into the SSL implementation we should use USE_SSL here instead. Same for the corresponding hunks in the frontend code. +* Note that if we don't support channel binding if we're not using SSL at +* all, we would not have advertised the PLUS variant in the first place. Seems like a word fell off when merging these sentences. This should probably be "..support channel binding, or if we're no.." or something similar. -#if defined(USE_OPENSSL) && (defined(HAVE_X509_GET_SIGNATURE_NID) || defined(HAVE_X509_GET_SIGNATURE_INFO)) -#define HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH +#ifdef USE_OPENSSL extern char *pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len); #endif No need for any guard at all now is there? All supported SSL implementations have it, and I doubt we'd accept a new one that doesn't (or which can't implement the function and error out). - # Functions introduced in OpenSSL 1.0.2. LibreSSL does not have - # SSL_CTX_set_cert_cb(). - AC_CHECK_FUNCS([X509_get_signature_nid SSL_CTX_set_cert_cb]) + # LibreSSL does not have SSL_CTX_set_cert_cb(). + AC_CHECK_FUNCS([SSL_CTX_set_cert_cb]) The comment about when the function was introduced is still interesting and should remain IMHO. The changes to the Windows buildsystem worry me a bit, but they don't move the goalposts in either direction wrt to LibreSSL on Windows so for the purpose of this patch we don't need to do more. Longer term we should either make LibreSSL work on Windows builds, or explicitly not support it on Windows. -- Daniel Gustafsson
Re: running logical replication as the subscription owner
On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote: > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada wrote: > > > > Thank you for updating the patch! Here are review comments: > > > > + /* > > +* Make sure that the copy command runs as the table owner, unless > > +* the user has opted out of that behaviour. > > +*/ > > + run_as_owner = MySubscription->runasowner; > > + if (!run_as_owner) > > + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); > > + > > /* Now do the initial data copy */ > > PushActiveSnapshot(GetTransactionSnapshot()); > > > > I think we should switch users before the acl check in > > LogicalRepSyncTableStart(). > > > > Agreed, we should check acl with the user that is going to perform > operations on the target table. BTW, is it okay to perform an > operation on the system table with the changed user as that would be > possible with your suggestion (see replorigin_create())? Do you see any problem in particular? As per the documentation, pg_replication_origin_create() is only allowed to the superuser by default, but in CreateSubscription() a non-superuser (who has pg_create_subscription privilege) can call replorigin_create(). OTOH, we don't necessarily need to switch to the table owner user for checking ACL and RLS. We can just pass either table owner OID or subscription owner OID to pg_class_aclcheck() and check_enable_rls() without actually switching the user. > > > > > While this approach works, I'm not sure we really need a trigger for > > this test. I've attached a patch for discussion that doesn't use > > triggers for the regression tests. We create a new subscription owned > > by a user who doesn't have the permission to the target table. The > > test passes only if run_as_owner = true works. > > > > Why in the test do you need to give additional permissions to > regress_admin2 when the actual operation has to be performed by the > table owner? Good point. We need to give the ability to SET ROLE to regress_admin2 but other permissions are unnecessary. > > +# Because the initial data sync is working as the table owner, all > +# dat should be copied. > > Typo. /dat/data Will fix. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
