Re: Statistics Import and Export
BTW, while nosing around looking for an explanation for our cross-version-upgrade woes, I chanced to notice that relation_statistics_update rejects negative values for relpages and relallvisible. This is nonsense. Internally those values are BlockNumbers and can have any value from 0 to UINT32_MAX. We represent them as signed int32 at the SQL level, which means they can read out as any int32 value. So the range checks that are being applied to them are flat wrong and should be removed. Admittedly, you'd need a table exceeding 16TB (if I did the math right) to see a problem, but that doesn't make it not wrong. It might be a good idea to change the code so that it declares these values internally as BlockNumber and uses PG_GETARG_UINT32, but I think that would only be a cosmetic change not a correctness issue. regards, tom lane
Re: Psql meta-command conninfo+
On 2025-Feb-21, Sami Imseih wrote: > > If we want to include 'role' in this output, what I'd propose is to > > have \conninfo issue "SHOW role", which is accepted by every server > > version. If it fails (say because we're in an aborted transaction), > > just omit that row from the output. > > v37- would have handled this as the list of PQ parameters was > dynamically generated and only those parameters > reported by the specific version of the server showed up in > \conninfo+. Okay, I have pushed this with some trivial tweaks -- removed the question marks and capitalized a couple of words. I also changed "Port" to "Server Port" because I wasn't sure it was obvious it was that, and maybe we want to list the client port as well (like pg_stat_activity does). We can continue to discuss adding 'role', 'server authorization' and so on, if people think they are going to be useful. We can consider such a decision an open item for 18. I tried it with 9.2, which doesn't have in_hot_standby. It shows like this 55441 18devel 356833=# \conninfo Connection Information Parameter │ Value ──┼ Database │ alvherre Client User │ alvherre Host │ localhost Host Address │ ::1 Server Port │ 55441 Options │ Protocol Version │ 3 Password Used│ false GSSAPI Authenticated │ false Backend PID │ 356833 TLS Connection │ true TLS Library │ OpenSSL TLS Protocol │ TLSv1.3 TLS Key Bits │ 256 TLS Cipher │ TLS_AES_256_GCM_SHA384 TLS Compression │ false ALPN │ none Superuser│ on Hot Standby │ unknown (19 rows) I think "unknown" here is okay, though we could probably say "unsupported by server" or just set it to null. Note that the boolean for superuser says 'on' instead of 'true'. Maybe we should make all the booleans use on/off instead of true/false? Not sure. Now we need someone to implement the PQsslAttribute() equivalent for GSS! If only to prove that the effort of tweaking the TLS layer to support multiple libraries ... Also, there's a bunch of "(char *)" casts that are 100% due to printTableAddCell() taking a char * instead of const char * for the cell value. That seems a bit silly, we should change that. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: PATCH: warn about, and deprecate, clear text passwords
On 21/02/2025 23:33, Greg Sabino Mullane wrote: There have been a few complaints lately about the fact that we cavalierly allow clear text passwords to be sent when doing CREATE USER or ALTER USER. These, of course, can end up in many places, such as pg_stat_activity, pg_stat_statements, .psql_history, and the server logs. It is a genuinely valid complaint, and for security purposes, there is little recourse other than telling users "don't do that". The canonical recommendation is to use psql's awesome \password feature. Second best is to use your application/driver of choice, which hopefully has support for not sending passwords in the clear. Please find attached a patch to implement a new GUC called cleartext_passwords_action as an attempt to solve these problems. It is an enum and accepts one of three values: 1. "warn" (the new default) This issues a warning if a clear text password is used, but allows the change to proceed. The hint can change to recommend \password if the current application_name is 'psql'. By keeping this as a warning, we let people know this is a bad idea, and give people time to modify their applications. Examples: ALTER USER alice PASSWORD 'mynewpass'; WARNING: using a clear text password DETAIL: Sending a password using plain text is deprecated and may be removed in a future release of PostgreSQL. HINT: Use a client that can change the password without sending it in clear text ALTER USER eve PASSWORD 'anothernewpass'; WARNING: using a clear text password DETAIL: Sending a password using plain text is deprecated and may be removed in a future release of PostgreSQL. HINT: If using psql, you can set the password with \password 2. "allow" This does nothing, and thus emulates the historical behavior. 3. "disallow" This prevents the use of plain old text completely, by throwing an error if a password set or change is attempted. So people who want to prevent clear text can do so right away, and at some point we can make this the default (and people can always change to hint or allow if desired) Bike shedding welcome. I realize the irony that 'disallow' means valid attempts will now show up in the database logs that otherwise would not, but I'm not sure how to work around that (or if we should). I'm obviously +1 on this patch since I sent kinda the same patch two weeks ago (https://www.postgresql.org/message-id/8f17493f-0886-406d-8573-0fadcb998b1d%40dalibo.co). The only major difference is that your patch can completely disable plain text passwords. More options, that sounds better to me. -- Guillaume Lelarge Consultant https://dalibo.com
Re: Restrict copying of invalidated replication slots
Some review comments for patch v2-0001.
==
Commit message
1.
Currently we can copy an invalidated logical and physical replication slot
using functions 'pg_copy_logical_replication_slot' and
'pg_copy_physical_replication_slot' respectively.
With this patch we will throw an error in such cases.
/we can copy an invalidated logical and physical replication slot/we
can copy invalidated logical and physical replication slots/
==
doc/src/sgml/func.sgml
pg_copy_physical_replication_slot:
2.
-is omitted, the same value as the
source slot is used.
+is omitted, the same value as the source slot is used. Copy of an
+invalidated physical replication slot in not allowed.
Typo /in/is/
Also, IMO you don't need to say "physical replication slot" because it
is clear from the function's name.
SUGGESTION
Copy of an invalidated slot is not allowed.
~~~
pg_copy_logical_replication_slot:
3.
+Copy of an invalidated logical replication slot in not allowed.
Typo /in/is/
Also, IMO you don't need to say "logical replication slot" because it
is clear from the function's name.
SUGGESTION
Copy of an invalidated slot is not allowed.
==
src/backend/replication/slotfuncs.c
copy_replication_slot:
4.
+ /* Check if source slot was invalidated while copying of slot */
+ SpinLockAcquire(&src->mutex);
+ first_slot_contents = *src;
+ SpinLockRelease(&src->mutex);
+
+ src_isinvalidated = (first_slot_contents.data.invalidated != RS_INVAL_NONE);
+
+ if (src_isinvalidated)
+ ereport(ERROR,
+ (errmsg("could not copy replication slot \"%s\"",
+ NameStr(*src_name)),
+ errdetail("The source replication slot was invalidated during the
copy operation.")));
4a.
We already know that it was not invalid the FIRST time we looked at
it, so IMO we only need to confirm that the SECOND look gives the same
answer. IOW, I thought the code should be like below. (AFAICT
Sawada-san's review says the same as this).
Also, I think it is better to say "became invalidated" instead of "was
invalidated", to imply the state was known to be ok before the copy.
SUGGESTION
+ /* Check if source slot became invalidated during the copy operation */
+ if (second_slot_contents.data.invalidated != RS_INVAL_NONE)
+ ereport(ERROR,
~
4b.
Unnecessary parentheses in the ereport.
~
4c.
There seems some weird mix of tense "cannot copy" versus "could not
copy" already in this file. But, maybe at least you can be consistent
within the patch and always say "cannot".
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: MAX_BACKENDS size (comment accuracy)
On Sun, Feb 23, 2025 at 4:16 AM Andres Freund wrote: > We do count the number of lwlock share lockers and the number of buffer > refcounts within those bits. And obviously 0 lockers/refcounts has to be > valid. So I think the limit is correct? Ah, right. That makes perfect sense. The 18 bits need to be able to hold a count, not just an index, and I confused myself about that from the moment I thought about the name PROC_NUMBER_BITS, which I retract. > I didn't yet have enough coffe, but isn't the inval.c limit 2^24-1 rather than > 2^23-1? Yeah, it has 24 bits of space, but curiously backend_hi is signed, so (msg->sm.backend_hi << 16) would be sign-extended, so it wouldn't actually work if you used all 24 bits... which is obviously not a real problem...
Re: Elimination of the repetitive code at the SLRU bootstrap functions.
On 2025-Feb-18, Evgeny Voropaev wrote:
> Created functions BootStrapSlruPage,SimpleLruZeroAndLogPage,
> WriteSlruZeroPageXlogRec. Using of these functions allows to delete
> ZeroXYZPage functions, WriteXYZZeroPageXlogRec functions and eliminate code
> repetitions.
I think the overall idea here is good, but I didn't like the new
WriteSlruZeroPageXlogRec helper function; it looks too much like a
modularity violation (same for the fact that you have to pass the rmid
and info from each caller into slru.c). I would not do that in slru.c
but instead in every individual caller, and have this helper function in
xloginsert.c where it can be caller XLogSimpleInsert or something like
that.
Also, please do not document a bunch of functions together in a single
comment. Instead, have one comment for each function. I mean this
here:
> diff --git a/src/backend/access/transam/slru.c
> b/src/backend/access/transam/slru.c
> index 9ce628e62a5..cc069da19c6 100644
> --- a/src/backend/access/transam/slru.c
> +++ b/src/backend/access/transam/slru.c
> @@ -363,6 +364,65 @@ check_slru_buffers(const char *name, int *newval)
> return false;
> }
>
> +/*
> + * BootStrapSlruPage,
> + * SimpleLruZeroAndLogPage,
> + * SimpleLruZeroPage
> + * - functions nullifying SLRU pages.
> + *
> + * BootStrapSlruPage is the most holistic. It performs:
> + * 1. locking,
> + * 2. nullifying,
> + * 3. logging (when writeXlog is true),
> + * 4. writing out,
> + * 5. releasing the lock.
> + *
> + * SimpleLruZeroAndLogPage performs:
> + * 2. nullifying,
> + * 3. logging (when writeXlog is true),
> + * 4. writing out.
> + *
> + * If the writeXlog is true, BootStrapSlruPage and SimpleLruZeroAndLogPage
> + * emit an XLOG record saying we did this.
> + * If the writeXlog is false, the rmid and info parameters are unused.
> + *
> + * SimpleLruZeroPage performs:
> + * 2. nullifying.
> + */
> +void
> +BootStrapSlruPage(SlruCtl ctl, int64 pageno,
> + bool writeXlog, RmgrId rmid, uint8 info)
> +{
This is not our style, and I don't think it's very ergonomical. Most
people don't read source files from top to bottom normally (heck,
sometimes I read source from bottom to top), so somebody looking at
SimpleLruZeroAndLogPage (the second function) might just not realize
that it's documented a few pagefuls above itself.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)
Re: Simplify the logic a bit (src/bin/scripts/reindexdb.c)
On 2025-Feb-21, Ranier Vilela wrote: > Any chance to push this forward? > Is it worth creating a committfest entry? Yeah, I'll have a look. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Entristecido, Wutra (canción de Las Barreras) echa a Freyr a rodar y a nosotros al mar"
Re: MAX_BACKENDS size (comment accuracy)
Hi, On 2025-02-22 18:54:08 +1300, Thomas Munro wrote: > On Sat, Feb 22, 2025 at 7:27 AM Andres Freund wrote: > +#define MAX_BACKENDS_BITS18 > +#define MAX_BACKENDS((1 << MAX_BACKENDS_BITS)-1) > > +1 for working forwards from the bits. But why not call it > PROC_NUMBER_BITS? Partially just because it was named MAX_BACKENDS historically. But also because it seems like it could be misleading - ProcNumber has more bits than PROC_NUMBER_BITS would indicate. > Hmm. Why shouldn't the highest usable ProcNumber (that you might call > PROC_NUMBER_MAX, like INT_MAX et all) FWIW, I don't think we should name it _MAX, precisely because of INT_MAX etc. INT_MAX indicate the actual range of the type, which isn't what we're dealing with here. > be (1 << PROC_NUMBER_BITS) - 1, and wouldn't that imply that MAX_BACKENDS > should actually be 1 << PROC_NUMBER_BITS and that any valid ProcNumber (a > 0-based index) should be *less than* MAX_BACKENDS (a count)? I don't *think* so, but I'm good at off-by-one-ing: > In other words, doesn't the current coding arbitrarily prevent the use of > one more backend, the one that has the highest ProcNumber representable in > 18 bits? If I'm right about that I think it is perhaps related to the use > of 0 as an invalid/unset BackendId before the ProcNumber-only redesign. We do count the number of lwlock share lockers and the number of buffer refcounts within those bits. And obviously 0 lockers/refcounts has to be valid. So I think the limit is correct? > + * currently realistic configurations. Even if that limitation were removed, > + * we still could not a) exceed 2^23-1 because inval.c stores the ProcNumber > + * as a 3-byte signed integer, b) INT_MAX/4 because some places compute > + * 4*MaxBackends without any overflow check. This is rechecked in the > > Should those two constraints have their own assertions? Probably wouldn't hurt, even though I think it's unlikely to matter anytime soon. I didn't yet have enough coffe, but isn't the inval.c limit 2^24-1 rather than 2^23-1? Greetings, Andres Freund
Re: Non-text mode for pg_dumpall
hi.
v20-0001
in src/bin/pg_dump/pg_dumpall.c, we have:
static const char *connstr = "";
case 'd':
connstr = pg_strdup(optarg);
break;
i am not sure you can declare it as "const" for connstr.
since connstr value can be changed.
``#include "pg_backup.h"`` can be removed from src/bin/pg_dump/pg_dumpall.c
Other than that, v20_0001 looks good to me.
v20_0002
const char *formatName = "p";
formatName should not be declared as "const", since its value can be changed.
+ /* Create a subdirectory with 'databases' name under main directory. */
+ if (mkdir(db_subdir, 0755) != 0)
+ pg_fatal("could not create subdirectory \"%s\": %m", db_subdir);
can change to
if (mkdir(db_subdir, pg_dir_create_mode) != 0)
pg_fatal("could not create subdirectory \"%s\": %m", db_subdir);
then in src/bin/pg_dump/pg_dumpall.c need add ``#include "common/file_perm.h"``
similarly
+ else if (mkdir(dirname, 0700) < 0)
+ pg_fatal("could not create directory \"%s\": %m", dirname);
can change to
``
else if (mkdir(dirname, pg_dir_create_mode) != 0)
pg_fatal("could not create directory \"%s\": %m", dirname);
``
+
+ if (!conn)
+ pg_log_info("considering PATTERN as NAME for --exclude-database
option as no db connection while doing pg_restore.");
"db connection" maybe "database connection" or "connection"
+ /*
+ * We need to reset on_exit_nicely_index with each database so that
we can restore
+ * multiple databases by archive. See EXIT_NICELY macro for more details.
+ */
+ if (dboid_cell != NULL)
+ reset_exit_nicely_list(n_errors ? 1 : 0);
i don't fully understand this part, anyway, i think EXIT_NICELY, you mean
MAX_ON_EXIT_NICELY?
just found out, parseArchiveFormat looks familiar with parseDumpFormat.
for all the options in pg_restore.
--list option is not applicable to multiple databases, therefore
option --use-list=list-file also not applicable,
in the doc we should mention it.
global.dat comments should not mention "cluster", "global objects"
would be more appropriate.
global.dat comments should not mention "--\n-- Database \"%s\" dump\n--\n\n"
the attached minor patch fixes this issue.
v20_refactor_restore_comments.nocfbot
Description: Binary data
Re: Make query cancellation keys longer
On Mon, 9 Sept 2024 at 17:58, Robert Haas wrote:
>
> On Fri, Aug 16, 2024 at 11:29 AM Robert Haas wrote:
> > > I'll split this patch like that, to make it easier to compare and merge
> > > with Jelte's corresponding patches.
> >
> > That sounds great. IMHO, comparing and merging the patches is the next
> > step here and would be great to see.
>
> Heikki, do you have any update on this work?
My patchset in the other protocol thread needed a rebase. So I took
that as an opportunity to rebase this patchset on top of it, because
this seems to be the protocol change that we can most easily push over
the finish line.
1. I changed the last patch from Heikki to only contain the changes
for the cancel lengthening. The general protocol change related things
I merged with mine (I only kept some error handling and docs).
2. I also removed the length field in the new BackendKey definition,
eventhough I asked for that addition previously. I agree with Heikki
that it's actually easier to parse and create without, because you can
use the same code for both versions.
3. I made our timingsafe_bcmp implementation call into OpenSSL's CRYPTO_memcmp.
One open question on the last patch is: Document what the maximum size
of the cancel key is that the client can expect? I think Jacob might
have some ideas on that.
From 0168dd6d463eb989d2e944c8acccf7cc620f5db1 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Mon, 23 Dec 2024 16:49:10 +0100
Subject: [PATCH v4 1/7] libpq: Add PQfullProtocolVersion to exports.txt
This is necessary to be able to actually use the function on Windows.
---
src/interfaces/libpq/exports.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 9b789cbec0b..d5143766858 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -209,3 +209,4 @@ PQservice 206
PQsetAuthDataHook 207
PQgetAuthDataHook 208
PQdefaultAuthDataHook 209
+PQfullProtocolVersion 210
base-commit: bba2fbc6238b2a0a7f348fbbb5c31ffa7623bc39
--
2.43.0
From 804da9044f08c62db598b42296b973284ca5b32b Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Wed, 5 Jun 2024 11:40:04 +0200
Subject: [PATCH v4 2/7] libpq: Trace NegotiateProtocolVersion correctly
This changes the libpq tracing code to correctly trace the
NegotiateProtocolVersion message. Previously it wasn't important that
tracing of the NegotiateProtocolVersion message worked correctly,
because in practice libpq never received it. Now that we are planning to
introduce protocol changes in future commits it starts to become more
useful for testing/debugging.
---
src/interfaces/libpq/fe-trace.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/interfaces/libpq/fe-trace.c b/src/interfaces/libpq/fe-trace.c
index 641e70f321c..a45f0d85587 100644
--- a/src/interfaces/libpq/fe-trace.c
+++ b/src/interfaces/libpq/fe-trace.c
@@ -578,9 +578,15 @@ pqTraceOutput_RowDescription(FILE *f, const char *message, int *cursor, bool reg
static void
pqTraceOutput_NegotiateProtocolVersion(FILE *f, const char *message, int *cursor)
{
+ int nparams;
+
fprintf(f, "NegotiateProtocolVersion\t");
pqTraceOutputInt32(f, message, cursor, false);
- pqTraceOutputInt32(f, message, cursor, false);
+ nparams = pqTraceOutputInt32(f, message, cursor, false);
+ for (int i = 0; i < nparams; i++)
+ {
+ pqTraceOutputString(f, message, cursor, false);
+ }
}
static void
--
2.43.0
From d4264c2487cb7c84efeaee09cbe52685487c0b88 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Wed, 14 Aug 2024 18:25:16 +0200
Subject: [PATCH v4 3/7] libpq: Handle NegotiateProtocolVersion message
differently
Previously libpq would always error when the server returns a
NegotiateProtocolVersion message. This was fine because libpq only
supported a single protocol version and did not support any protocol
parameters. But we now that we're discussing protocol changes we need to
change this behaviour, and introduce a fallback mechanism when
connecting to an older server.
This patch modifies the client side checks to allow a range of supported
protocol versions, instead of only allowing the exact version that was
requested. Currently this "range" only contains the 3.0 version, but in
a future commit we'll change this. In addition it changes the errors for
protocol to say that they error because we did not request any
parameters, not because the server did not support some of them. In a
future commit more infrastructure for protocol parameters will be added,
so that these checks can also succeed when receiving unsupported
parameters back.
Note that at the moment this change does not have any behavioural
effect, because libpq will only request version 3.0 and will never send
protocol parameters. Which means that the client never receives a
NegotiateProtocolVersion message from the server.
---
src/interfaces/libpq/fe-connect.c | 3 +-
src/interfa
Re: Fix logging for invalid recovery timeline
On 2/21/25 09:09, Benoit Lobréau wrote:
On 2/20/25 4:40 PM, David Steele wrote:
Benoit -- this was your idea. Did you want to submit a patch yourself?
Here is an attempt at that. I kept the wording I used above. Is it fine
to repeat the whole ereport block twice?
I think for translation purposes this is probably how it needs to be but
I wonder if we could do something like:
errdetail("Latest checkpoint in %s is at %X/%X <...>",
haveBackupLabel ? "pg_control" ? "backup_label",
I'll defer to Michael on that.
In general this patch and the new messages look good to me, though.
Regards,
-David
Re: PATCH: warn about, and deprecate, clear text passwords
On 22/02/2025 09:07, Guillaume Lelarge wrote: On 21/02/2025 23:33, Greg Sabino Mullane wrote: There have been a few complaints lately about the fact that we cavalierly allow clear text passwords to be sent when doing CREATE USER or ALTER USER. These, of course, can end up in many places, such as pg_stat_activity, pg_stat_statements, .psql_history, and the server logs. It is a genuinely valid complaint, and for security purposes, there is little recourse other than telling users "don't do that". The canonical recommendation is to use psql's awesome \password feature. Second best is to use your application/driver of choice, which hopefully has support for not sending passwords in the clear. Please find attached a patch to implement a new GUC called cleartext_passwords_action as an attempt to solve these problems. It is an enum and accepts one of three values: 1. "warn" (the new default) This issues a warning if a clear text password is used, but allows the change to proceed. The hint can change to recommend \password if the current application_name is 'psql'. By keeping this as a warning, we let people know this is a bad idea, and give people time to modify their applications. Examples: ALTER USER alice PASSWORD 'mynewpass'; WARNING: using a clear text password DETAIL: Sending a password using plain text is deprecated and may be removed in a future release of PostgreSQL. HINT: Use a client that can change the password without sending it in clear text ALTER USER eve PASSWORD 'anothernewpass'; WARNING: using a clear text password DETAIL: Sending a password using plain text is deprecated and may be removed in a future release of PostgreSQL. HINT: If using psql, you can set the password with \password 2. "allow" This does nothing, and thus emulates the historical behavior. 3. "disallow" This prevents the use of plain old text completely, by throwing an error if a password set or change is attempted. So people who want to prevent clear text can do so right away, and at some point we can make this the default (and people can always change to hint or allow if desired) Bike shedding welcome. I realize the irony that 'disallow' means valid attempts will now show up in the database logs that otherwise would not, but I'm not sure how to work around that (or if we should). I'm obviously +1 on this patch since I sent kinda the same patch two weeks ago (https://www.postgresql.org/message- id/8f17493f-0886-406d-8573-0fadcb998b1d%40dalibo.co). The only major difference is that your patch can completely disable plain text passwords. More options, that sounds better to me. It applies cleanly, compiles without errors or even warnings. I did some tests, and I only found one small issue: set password_encryption to 'md5'; create user u4 password 'md5u1'; WARNING: using a clear text password DETAIL: Sending a password using plain text is deprecated and may be removed in a future release of PostgreSQL. HINT: If using psql, you can set the password with \password WARNING: setting an MD5-encrypted password DETAIL: MD5 password support is deprecated and will be removed in a future release of PostgreSQL. HINT: Refer to the PostgreSQL documentation for details about migrating to another password type. CREATE ROLE It complains that I'm using a plain text password and a MD5-encrypted password. Can't be both. (Probably not an issue with this patch, but rather an issue with the commit that implemented MD5-password warnings.) If I use a real md5 password, it only complains about MD5 encrypted password: create user u5 password 'md58026a39c502750413402a90d9d8bae3c'; WARNING: setting an MD5-encrypted password DETAIL: MD5 password support is deprecated and will be removed in a future release of PostgreSQL. HINT: Refer to the PostgreSQL documentation for details about migrating to another password type. CREATE ROLE Other tests were successful. Thanks Greg! -- Guillaume Lelarge Consultant https://dalibo.com
Re: Virtual generated columns
On Sat, Feb 22, 2025 at 11:55 PM Richard Guo wrote: > Attached are the updated patches to fix all the mentioned issues. I > plan to push them early next week after staring at the code for a bit > longer, barring any objections. Sign... I neglected to make the change in 0001 that a Var newnode compares its varlevelsup with 0 when deciding to wrap it in a ReturningExpr. I made this change in 0002 though, so maybe we're good here. Still, I'll fix this later. Thanks Richard
Re: generic plans and "initial" pruning
Alexander Lakhin 于2025年2月22日周六 23:00写道： > Hello Amit, > > 21.02.2025 05:40, Amit Langote wrote: > > I pushed the final piece yesterday. > > > Please look at new error, produced by the following script, > starting from 525392d57: > CREATE TABLE t(id int) PARTITION BY RANGE (id); > CREATE INDEX idx on t(id); > CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (10) TO (20); > CREATE TABLE tp_2 PARTITION OF t FOR VALUES FROM (20) TO (30) PARTITION BY > RANGE(id); > CREATE TABLE tp_2_1 PARTITION OF tp_2 FOR VALUES FROM (21) to (22); > CREATE TABLE tp_2_2 PARTITION OF tp_2 FOR VALUES FROM (22) to (23); > CREATE FUNCTION stable_one() RETURNS INT AS $$ BEGIN RETURN 1; END; $$ > LANGUAGE plpgsql STABLE; > > SELECT min(id) OVER (PARTITION BY id ORDER BY id) FROM t WHERE id >= > stable_one(); > > ERROR: XX000: trying to open a pruned relation > LOCATION: ExecGetRangeTableRelation, execUtils.c:830 > > This issue was discovered with SQLsmith. > The error message was added in commit 525392d57. In this case, the estate->es_unpruned_relids only includes 1, which is the offset of table t. In register_partpruneinfo(), we collect glob->prunableRelids; in this case, it contains 2,3,4,5. Then we will do: result->unprunableRelids = bms_difference(glob->allRelids, glob->prunableRelids); so the result->unprunableRelids only contains 1. But tp_2 is also partition table, and its partpruneinfo created by create_append_plan() is put into the head of global list. So we first process it in ExecDoInitialPruning(). Then error reports because we only contain 1 in estate->es_unpruned_relids. -- Thanks, Tender Wang
Re: Virtual generated columns
On Sat, Feb 22, 2025 at 2:35 AM Dean Rasheed wrote: > On Fri, 21 Feb 2025 at 06:16, Richard Guo wrote: > > * The expansion of virtual generated columns occurs after subquery > > pullup, which can lead to issues. This was an oversight on my part. > > Initially, I believed it wasn't possible for an RTE_RELATION RTE to > > have 'lateral' set to true, so I assumed it would be safe to expand > > virtual generated columns after subquery pullup. However, upon closer > > look, this doesn't seem to be the case: if a subquery had a LATERAL > > marker, that would be propagated to any of its child RTEs, even for > > RTE_RELATION child RTE if this child rel has sampling info (see > > pull_up_simple_subquery). > > Ah yes. That matches my initial instinct, which was to expand virtual > generated columns early in the planning process, but I didn't properly > understand why that was necessary. After chewing on this point for a bit longer, I think the virtual generated columns should be expanded after we have pulled up any SubLinks within the query's quals; otherwise any virtual generated column references within the SubLinks that should be transformed into joins wouldn't get expanded. As an example, please consider: create table t (a int, b int); create table vt (a int, b int generated always as (a * 2)); insert into t values (1, 1); insert into vt values (1); # select 1 from t t1 where exists (select 1 from vt where exists (select t1.a from t t2 where vt.b = 2)); ERROR: unexpected virtual generated column reference > LGTM aside from a comment in fireRIRrules() that needed updating and a > minor issue in the callback function: when deciding whether to wrap > newnode in a ReturningExpr, if newnode is a Var, it should now compare > its varlevelsup with 0, not var->varlevelsup, since newnode hasn't had > its varlevelsup adjusted at that point. Nice catch. Attached are the updated patches to fix all the mentioned issues. I plan to push them early next week after staring at the code for a bit longer, barring any objections. Thanks Richard v7-0001-Expand-virtual-generated-columns-in-the-planner.patch Description: Binary data v7-0002-Eliminate-code-duplication-in-replace_rte_variables-callbacks.patch Description: Binary data
Re: generic plans and "initial" pruning
Hello Amit, 21.02.2025 05:40, Amit Langote wrote: I pushed the final piece yesterday. Please look at new error, produced by the following script, starting from 525392d57: CREATE TABLE t(id int) PARTITION BY RANGE (id); CREATE INDEX idx on t(id); CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (10) TO (20); CREATE TABLE tp_2 PARTITION OF t FOR VALUES FROM (20) TO (30) PARTITION BY RANGE(id); CREATE TABLE tp_2_1 PARTITION OF tp_2 FOR VALUES FROM (21) to (22); CREATE TABLE tp_2_2 PARTITION OF tp_2 FOR VALUES FROM (22) to (23); CREATE FUNCTION stable_one() RETURNS INT AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE; SELECT min(id) OVER (PARTITION BY id ORDER BY id) FROM t WHERE id >= stable_one(); ERROR: XX000: trying to open a pruned relation LOCATION: ExecGetRangeTableRelation, execUtils.c:830 This issue was discovered with SQLsmith. Best regards, Alexander Lakhin Neon (https://neon.tech)
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
On Tue, 18 Feb 2025 at 09:51, Jelte Fennema-Nio wrote: > Rebased it, and moved some of the new header definitions around to > hopefully not have to rebase again. This required another rebase, but I've decided that I think it'll be most fruitful to continue the discussion on protocol changes in the thread about longer cancel keys[1]. Over there I've removed patch 5, 7 and 8 and rebased Heikki's changes on top of the rest. I hope this'll let us make some progress sooner. [1]: https://www.postgresql.org/message-id/flat/CAGECzQTfc_O%2BHXqAo5_-xG4r3EFVsTefUeQzSvhEyyLDba-O9w%40mail.gmail.com#b88eb8989587b1f3acef5c00736b097f
Re: TAP test started using meson, can get a tcp port already used by another test.
On 2025-02-22 04:11, Andrew Dunstan wrote: On 2025-02-21 Fr 11:49 AM, Andres Freund wrote: Pushed. Thanks Thank you very much! Best regards, Roman Zharkov
Re: Statistics Import and Export
On Fri, 2025-02-21 at 07:24 +, Hayato Kuroda (Fujitsu) wrote: > I hope I'm in the correct thread. I found the commit 1fd1bd8 - it is > so cool. Yes, documentation corrections are appreciated, thank you. > But pgupgrade.sgml [2] and source code [3] said that statistics must > be updated. Changed. Regards, Jeff Davis
Re: Proposal: Filter irrelevant change before reassemble transactions during logical decoding
Hi Ajin, Some review comments for v14-0002. == src/backend/replication/logical/decode.c 1. There is lots of nearly duplicate code checking to see if a change is filterable DecodeInsert: + /* + * When filtering changes, determine if the relation associated with the change + * can be skipped. This could be because the relation is unlogged or because + * the plugin has opted to exclude this relation from decoding. + */ + if (FilterChangeIsEnabled(ctx) && ReorderBufferFilterByRelFileLocator(ctx->reorder, XLogRecGetXid(r), - buf->origptr, &target_locator, true)) + buf->origptr, &target_locator, + REORDER_BUFFER_CHANGE_INSERT, + true)) DecodeUpdate: + /* + * When filtering changes, determine if the relation associated with the change + * can be skipped. This could be because the relation is unlogged or because + * the plugin has opted to exclude this relation from decoding. + */ + if (FilterChangeIsEnabled(ctx) && + ReorderBufferFilterByRelFileLocator(ctx->reorder, XLogRecGetXid(r), + buf->origptr, &target_locator, + REORDER_BUFFER_CHANGE_UPDATE, + true)) + return; + DecodeDelete: + /* + * When filtering changes, determine if the relation associated with the change + * can be skipped. This could be because the relation is unlogged or because + * the plugin has opted to exclude this relation from decoding. + */ + if (FilterChangeIsEnabled(ctx) && + ReorderBufferFilterByRelFileLocator(ctx->reorder, XLogRecGetXid(r), + buf->origptr, &target_locator, + REORDER_BUFFER_CHANGE_DELETE, + true)) + return; + DecodeMultiInsert: /* + * When filtering changes, determine if the relation associated with the change + * can be skipped. This could be because the relation is unlogged or because + * the plugin has opted to exclude this relation from decoding. + */ + if (FilterChangeIsEnabled(ctx) && + ReorderBufferFilterByRelFileLocator(ctx->reorder, XLogRecGetXid(r), + buf->origptr, &rlocator, + REORDER_BUFFER_CHANGE_INSERT, + true)) + return; + DecodeSpecConfirm: + /* + * When filtering changes, determine if the relation associated with the change + * can be skipped. This could be because the relation is unlogged or because + * the plugin has opted to exclude this relation from decoding. + */ + if (FilterChangeIsEnabled(ctx) && + ReorderBufferFilterByRelFileLocator(ctx->reorder, XLogRecGetXid(r), + buf->origptr, &target_locator, + REORDER_BUFFER_CHANGE_INSERT, + true)) + return; + Can't all those code fragments (DecodeInsert, DecodeUpdate, DecodeDelete, DecodeMultiInsert, DecodeSpecConfirm) delegate to a new/common 'SkipThisChange(...)' function? == src/backend/replication/logical/reorderbuffer.c 2. /* * After encountering a change that cannot be filtered out, filtering is - * temporarily suspended. Filtering resumes after processing every 100 changes. + * temporarily suspended. Filtering resumes after processing CHANGES_THRESHOLD_FOR_FILTER + * changes. * This strategy helps to minimize the overhead of performing a hash table * search for each record, especially when most changes are not filterable. */ -#define CHANGES_THRESHOLD_FOR_FILTER 100 +#define CHANGES_THRESHOLD_FOR_FILTER 0 Why is this defined as 0? Some accidental residue from performance testing different values? == src/test/subscription/t/001_rep_changes.pl 3. +# Check that an unpublished change is filtered out. +$logfile = slurp_file($node_publisher->logfile, $log_location); +ok($logfile =~ qr/Filtering INSERT/, + 'unpublished INSERT is filtered'); + +ok($logfile =~ qr/Filtering UPDATE/, + 'unpublished UPDATE is filtered'); + +ok($logfile =~ qr/Filtering DELETE/, + 'unpublished DELETE is filtered'); + AFAICT these are probably getting filtered out because the entire table is not published at all. Should you also add different tests where you do operations on a table that IS partially replicated but only some of the *operations* are not published. e.g. test the different 'pubactions' of the PUBLICATION 'publish' parameter. Maybe you need different log checks to distinguish the different causes for the filtering. == Kind Regards, Peter Smith. Fujitsu Australia
Re: [PATCH] Fix Potential Memory Leak in pg_amcheck Code
Hi! On Fri, 7 Feb 2025 at 13:28, Daniel Gustafsson wrote: > > That being said, there is > value in doing the right thing and setting good examples in our own code as > many do read it and reference it. > We call PQclear on such a > case elsewhere in the file so it's not entirely consistent, but it's not all > that important as the memory will be reclaimed at exit. So, are we +1 or -1 on moving this forward? -- Best regards, Kirill Reshke
