Re: [BUG]Update Toast data failure in logical replication
On Mon, Feb 7, 2022 at 12:25 PM Amit Kapila wrote: > > On Sat, Feb 5, 2022 at 6:10 AM Amit Kapila wrote: > > > > On Fri, Feb 4, 2022 at 9:06 PM Alvaro Herrera > > wrote: > > > > > > > > > I have some suggestions > > > on the comments and docs though. > > > > > > > Thanks, your suggestions look good to me. I'll take care of these in > > the next version. > > > > Attached please find the modified patches. I have looked into the latest modification and back branch patches and they look fine to me. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: row filtering for logical replication
Hi - I did a review of the v77 patches merged with Amit's v77 diff patch [1].
(Maybe this is equivalent to reviewing v78)
Below are my review comments:
==
1. doc/src/sgml/ref/create_publication.sgml - CREATE PUBLICATION
+ The WHERE clause allows simple expressions that
don't have
+ user-defined functions, operators, collations, non-immutable built-in
+ functions, or references to system columns.
+
That seems slightly ambiguous for operators and collations. It's only
the USER-DEFINED ones we don't support.
Perhaps it should be worded like:
"allows simple expressions that don't have user-defined
functions/operators/collations, non-immutable built-in functions..."
or like
"allows simple expressions that don't have user-defined functions,
user-defined operators, user-defined collations, non-immutable
built-in functions..."
~~~
2. src/backend/catalog/pg_publication.c - GetTopMostAncestorInPublication
+Oid
+GetTopMostAncestorInPublication(Oid puboid, List *ancestors)
+{
+ ListCell *lc;
+ Oid topmost_relid = InvalidOid;
+
+ /*
+ * Find the "topmost" ancestor that is in this publication.
+ */
+ foreach(lc, ancestors)
+ {
+ Oid ancestor = lfirst_oid(lc);
+ List*apubids = GetRelationPublications(ancestor);
+ List*aschemaPubids = NIL;
+
+ if (list_member_oid(apubids, puboid))
+ topmost_relid = ancestor;
+ else
+ {
+ aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
+ if (list_member_oid(aschemaPubids, puboid))
+ topmost_relid = ancestor;
+ }
+
+ list_free(apubids);
+ list_free(aschemaPubids);
+ }
+
+ return topmost_relid;
+}
Wouldn't it be better for the aschemaPubids to be declared and freed
inside the else block?
e.g.
else
{
List *aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
if (list_member_oid(aschemaPubids, puboid))
topmost_relid = ancestor;
list_free(aschemaPubids);
}
~~~
3. src/backend/commands/publicationcmds.c - contain_invalid_rfcolumn
+ if (pubviaroot && relation->rd_rel->relispartition)
+ {
+ publish_as_relid = GetTopMostAncestorInPublication(pubid, ancestors);
+
+ if (publish_as_relid == InvalidOid)
+ publish_as_relid = relid;
+ }
Consider using the macro code for the InvalidOid check. e.g.
if (!OidIsValid(publish_as_relid)
publish_as_relid = relid;
~~~
4. src/backend/commands/publicationcmds.c - IsRowFilterSimpleExpr (Tests)
+ switch (nodeTag(node))
+ {
+ case T_ArrayExpr:
+ case T_BooleanTest:
+ case T_BoolExpr:
+ case T_CaseExpr:
+ case T_CaseTestExpr:
+ case T_CoalesceExpr:
+ case T_CollateExpr:
+ case T_Const:
+ case T_FuncExpr:
+ case T_MinMaxExpr:
+ case T_NullTest:
+ case T_RelabelType:
+ case T_XmlExpr:
+ return true;
+ default:
+ return false;
+ }
I think there are several missing regression tests.
4a. There is a new message that says "User-defined collations are not
allowed." but I never saw any test case for it.
4b. There is also the RelabelType which seems to have no test case.
Amit previously provided [2] some SQL which would give an unexpected
error, so I guess that should be a new regression test case. e.g.
create table t1(c1 int, c2 varchar(100));
create publication pub1 for table t1 where (c2 < 'john');
~~~
5. src/backend/commands/publicationcmds.c - IsRowFilterSimpleExpr (Simple?)
+/*
+ * Is this a simple Node permitted within a row filter expression?
+ */
+static bool
+IsRowFilterSimpleExpr(Node *node)
+{
A lot has changed in this area recently and I feel that there is
something not quite 100% right with the naming and/or logic in this
expression validation. IMO there are several functions that seem to
depend too much on each other in special ways...
IIUC the "walker" logic now seems to be something like this:
a) Check for special cases of the supported nodes
b) Then check for supported (simple?) nodes (i.e.
IsRowFilterSimpleExpr is now acting as a "catch-all" after the special
case checks)
c) Then check for some unsupported node embedded within a supported
node (i.e. call expr_allowed_in_node)
d) If any of a,b,c was bad then give an error.
To achieve that logic the T_FuncExpr was added to the
"IsRowFilterSimpleExpr". Meanwhile, other nodes like
T_ScalarArrayOpExpr and T_NullIfExpr now are removed from
IsRowFilterSimpleExpr - I don't quite know why these got removed but
perhaps there is implicit knowledge that those node kinds were already
checked by the "walker" before the IsRowFilterSimpleExpr function ever
gets called.
So, although I trust that everything is working OK, I don't think
IsRowFilterSimpleExpr is really just about simple nodes anymore. It is
harder to see why some supported nodes are in there, and some
supported nodes are not. It seems tightly entwined with the logic of
check_simple_rowfilter_expr_walker; i.e. there seem to be assumptions
about exactly when it will be called and what was checked before and
what will be checked after calling it.
IMO probably all the nodes we are supporting should be in the
IsRowFilterSimpleExpr just for completeness (e.g. put T_NullIfExpr
Remove IS_AF_UNIX macro?
I noticed that in pgstatfuncs.c, the AF_UNIX macro is being used
unprotected by HAVE_UNIX_SOCKETS, apparently since 2008. So I think the
redirection through IS_AF_UNIX() is no longer necessary. (More
generally, all supported platforms are now HAVE_UNIX_SOCKETS, but even
if there were a new platform in the future, it seems plausible that it
would define the AF_UNIX symbol even without kernel support.) So maybe
we should remove the IS_AF_UNIX() macro and make the code a bit more
consistent?From 388be5ebd137fc409878ac74061526e6dfcecf57 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 7 Feb 2022 08:37:00 +0100
Subject: [PATCH] Remove IS_AF_UNIX macro
---
src/backend/libpq/hba.c | 4 ++--
src/backend/libpq/pqcomm.c | 24
src/backend/postmaster/postmaster.c | 4 ++--
src/include/common/ip.h | 6 --
src/interfaces/libpq/fe-connect.c | 14 +++---
5 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index a7f3def184..d84a40b726 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -2138,12 +2138,12 @@ check_hba(hbaPort *port)
/* Check connection type */
if (hba->conntype == ctLocal)
{
- if (!IS_AF_UNIX(port->raddr.addr.ss_family))
+ if (port->raddr.addr.ss_family != AF_UNIX)
continue;
}
else
{
- if (IS_AF_UNIX(port->raddr.addr.ss_family))
+ if (port->raddr.addr.ss_family == AF_UNIX)
continue;
/* Check SSL state */
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index f05723dc92..b96c2811a4 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -408,7 +408,7 @@ StreamServerPort(int family, const char *hostName, unsigned
short portNumber,
for (addr = addrs; addr; addr = addr->ai_next)
{
- if (!IS_AF_UNIX(family) && IS_AF_UNIX(addr->ai_family))
+ if (family != AF_UNIX && addr->ai_family == AF_UNIX)
{
/*
* Only set up a unix domain socket when they really
asked for it.
@@ -493,7 +493,7 @@ StreamServerPort(int family, const char *hostName, unsigned
short portNumber,
* unpredictable behavior. With no flags at all, win32 behaves
as Unix
* with SO_REUSEADDR.
*/
- if (!IS_AF_UNIX(addr->ai_family))
+ if (addr->ai_family != AF_UNIX)
{
if ((setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
(char *) ,
sizeof(one))) == -1)
@@ -545,7 +545,7 @@ StreamServerPort(int family, const char *hostName, unsigned
short portNumber,
errmsg("could not bind %s address
\"%s\": %m",
familyDesc, addrDesc),
saved_errno == EADDRINUSE ?
-(IS_AF_UNIX(addr->ai_family) ?
+(addr->ai_family == AF_UNIX ?
errhint("Is another postmaster
already running on port %d?",
(int) portNumber) :
errhint("Is another postmaster
already running on port %d?"
@@ -763,7 +763,7 @@ StreamConnection(pgsocket server_fd, Port *port)
}
/* select NODELAY and KEEPALIVE options if it's a TCP connection */
- if (!IS_AF_UNIX(port->laddr.addr.ss_family))
+ if (port->laddr.addr.ss_family != AF_UNIX)
{
int on;
#ifdef WIN32
@@ -1638,7 +1638,7 @@ int
pq_getkeepalivesidle(Port *port)
{
#if defined(PG_TCP_KEEPALIVE_IDLE) || defined(SIO_KEEPALIVE_VALS)
- if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
+ if (port == NULL || port->laddr.addr.ss_family == AF_UNIX)
return 0;
if (port->keepalives_idle != 0)
@@ -1672,7 +1672,7 @@ pq_getkeepalivesidle(Port *port)
int
pq_setkeepalivesidle(int idle, Port *port)
{
- if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
+ if (port == NULL || port->laddr.addr.ss_family == AF_UNIX)
return STATUS_OK;
/* check SIO_KEEPALIVE_VALS here, not just WIN32, as some toolchains lack it */
@@ -1723,7 +1723,7 @@ int
pq_getkeepalivesinterval(Port *port)
{
#if defined(TCP_KEEPINTVL) || defined(SIO_KEEPALIVE_VALS)
- if (port == NULL || IS_AF_UNIX(port->laddr.addr.ss_family))
+ if (port == NULL || port->laddr.addr.ss_family == AF_UNIX)
return 0;
if
Re: decoupling table and index vacuum
On Fri, Feb 4, 2022 at 11:45 PM Robert Haas wrote: > > On Wed, Jan 26, 2022 at 8:58 AM Dilip Kumar wrote: > > TODO: > > - This is just a POC patch to discuss the design idea and needs a lot > > of improvement and testing. > > - We are using a slightly different format for storing the dead tids > > into the conveyor belt which is explained in the patch but the > > traditional multi-pass vacuum is still using the same format (array of > > ItemPointeData), so we need to unify that format. > > - Performance testing. > > - Cleaner interfaces so that we can easily be integrated with auto > > vacuum, currently, this is not provided for the manual vacuum. > > - Add test cases. > > I think this is a pretty interesting piece of work. I appreciate the > effort you've obviously put into the comments, although I do think > some of them are going to need some additional clarification. But I > think the bigger questions here at the moment are things like (1) Is > this the right idea? and if not (2) How could we change it to make it > better? and (3) Is there any way that we can make it simpler? It was > the last of these questions that prompted me to post > http://postgr.es/m/ca+tgmoy18rzqqdm2je2wdkia8ngtedhp7ulthb3a-abs+wb...@mail.gmail.com > because, if that thought were to work out, then we could have more > things in common between the conveyor-belt and non-conveyor-belt > cases, and we might be able to start with some preliminary work to > jigger more things in to the second phase, and then look to integrate > the conveyor belt stuff separately. I agree that if we can do something like that then integrating the conveyor belt will be much cleaner. > My second thought was that perhaps we can create a test scenario > where, in one index, the deduplication and bottom-up index deletion > and kill_prior_tuple mechanisms are very effective, and in another > index, it's not effective at all. For example, maybe index A is an > index on the primary key, and index B is a non-unique index on some > column that we're updating with ever-increasing values (so that we > never put new tuples into a page that could be productively cleaned > up). I think what should happen in this case is that A should not grow > in size even if it's never vacuumed, while B will require vacuuming to > keep the size down. If this example isn't exactly right, maybe we can > construct one where that does happen. Then we could try to demonstrate > that with this patch we can do less vacuuming work and still keep up > than what would be possible without the patch. We'll either be able to > show that this is true, or we will see that it's false, or we won't be > able to really see much difference. Any of those would be interesting > findings. +1 > One thing we could try doing in order to make that easier would be: > tweak things so that when autovacuum vacuums the table, it only > vacuums the indexes if they meet some threshold for bloat. I'm not > sure exactly what happens with the heap vacuuming then - do we do > phases 1 and 2 always, or a combined heap pass, or what? But if we > pick some criteria that vacuums indexes sometimes and not other times, > we can probably start doing some meaningful measurement of whether > this patch is making bloat better or worse, and whether it's using > fewer or more resources to do it. I think we can always trigger phase 1 and 2 and phase 2 will only vacuum conditionally based on if all the indexes are vacuumed for some conveyor belt pages so we don't have risk of scanning without marking anything unused. And we can try to measure with other approaches as well where we completely avoid phase 2 and it will be done only along with phase 1 whenever applicable. > Do you have a git branch for this work? Yeah, my repository: https://github.com/dilipbalaut11/conveyor_test branch: DecouplingIndexAndHeapVacuumUsingCB -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: [BUG]Update Toast data failure in logical replication
On Sat, Feb 5, 2022 at 6:10 AM Amit Kapila wrote: > > On Fri, Feb 4, 2022 at 9:06 PM Alvaro Herrera wrote: > > > > > > I have some suggestions > > on the comments and docs though. > > > > Thanks, your suggestions look good to me. I'll take care of these in > the next version. > Attached please find the modified patches. -- With Regards, Amit Kapila. HEAD-v8-0001-WAL-log-unchanged-toasted-replica-identity-key-.patch Description: Binary data 14-v8-0001-WAL-log-unchanged-toasted-replica-identity-key-.patch Description: Binary data 13-v8-0001-WAL-log-unchanged-toasted-replica-identity-key-.patch Description: Binary data 12-v8-0001-WAL-log-unchanged-toasted-replica-identity-key-.patch Description: Binary data 11-v8-0001-WAL-log-unchanged-toasted-replica-identity-key-.patch Description: Binary data 10-v8-0001-WAL-log-unchanged-toasted-replica-identity-key-.patch Description: Binary data
Re: pg_receivewal - couple of improvements
Hi, On Mon, Feb 07, 2022 at 12:03:03PM +0530, Bharath Rupireddy wrote: > > What if someone doesn't use pg_receivewal as an archive location? The > pg_receivewal can also be used for synchronous replication quorum > right? In this situation, I don't mind if some of the WAL files are > missing in pg_receivewal's target directory Those two seem entirely incompatible, why would you have a synchronous pg_receivewal that would ask for records removed by the primary, even if part of synchronous quorum, apart from inadequate (and dangerous) configuration? Also, in which scenario exactly would you be willing to pay a huge overhead to make sure that all the WAL records are safely transferred to one or multiple alternative location but at the same time don't mind if you're missing some WAL segments?
Re: pg_receivewal - couple of improvements
On Mon, Feb 7, 2022 at 8:23 AM Michael Paquier wrote: > > On Sun, Feb 06, 2022 at 01:01:41PM +0530, Bharath Rupireddy wrote: > > With the start position as an option, users can just provide the LSN > > from which they want to stream the WAL, in the above case, it can be > > from primary's latest checkpoint LSN. > > This still strikes me as a dangerous thing to have, prone to errors > with a bunch of base backups wasted if one does a mistake as it would > be very easy to cause holes in the WAL stored, until one has to > redeploy from a base backup in urgency. This kind of control is > provided by replication slots for new locations, and the current > archive location if anything is stored, so I would leave it at that. What if someone doesn't use pg_receivewal as an archive location? The pg_receivewal can also be used for synchronous replication quorum right? In this situation, I don't mind if some of the WAL files are missing in pg_receivewal's target directory, but I don't want to do the manual work of getting the WAL files to the pg_receivewal's target directory from my archive location to make the pg_receivewal up and connect with primary again? The primary can still remove the WAL files needed by pg_receivewal (after the max_slot_wal_keep_size limit). If I can tell pg_receivewal where to get the start position, then that will be a good idea. > On top of that, this kind of control is prone to more race conditions > with the backend, as a concurrent checkpoint could make the LSN you > are looking for irrelevant. I understand that having a start position as an option is more error-prone and creates holes in the WAL file. Why can't we allow users to choose the current start position calculation of the pg_receivewal? Users can choose to tell pg_receivewal either to get start position from its target directory or from its slot's restart_lsn or from the server's identify_system command. Thoughts? Regards, Bharath Rupireddy.
Re: Query choosing Bad Index Path
Hi, On Mon, Feb 07, 2022 at 11:36:17AM +0530, Valli Annamalai wrote: > *Postgres version:* 11.4 > > *Problem:* > Query choosing Bad Index Path. Details are provided below: > > > *Table :* > > > > > > > *Doubt* >1. Why is this Query choosing *Index Scan Backward using table1_pkey > Index *though it's cost is high. It can rather choose > BITMAP OR > (Index on RECORDID) i.e; table1_idx6 > (Index on RELATEDID) i.e; table1_idx7 > > Below is the selectivity details from *pg_stats* table > - Recordid has 51969 distinct values. And selectivity > (most_common_freqs) for *recordid = 15842006928391817* is 0.00376667 > - Relatedid has 82128 distinct values. And selectivity > (most_common_freqs) for *recordid = 15842006928391817* is 0.0050666 > > Since, selectivity is less, this should logically choose this Index, which > would have improve my query performance here. > I cross-checked the same by removing PrimaryKey to this table and query now > chooses these indexes and response is in 100ms. Please refer the plan below > (after removing primary key): You already sent the same email less than an hour ago on pgsql-performance ([1]), which is the correct mailing list, please don't post on this mailing list also. Note also that sending information as images can be problematic as some people here (including me) won't be able to see them. I tried on a web-based MUA and that's not really better though, as the images are hardly readable and definitely not grep-able. You will likely have more answers sending information in plain text. [1] https://www.postgresql.org/message-id/cadkhgij+gt_fdkzwgp8ozsy6irbymykmrjsphqhct1a2kbg...@mail.gmail.com
Re: 2022-01 Commitfest
On Sun, Feb 06, 2022 at 02:57:45PM +0800, Julien Rouhaud wrote: > > On Sun, Feb 06, 2022 at 03:49:50PM +0900, Michael Paquier wrote: > > > > FWIW, I just apply a two-week rule here, as of half the commit fest > > period to let people the time to react: > > - If a patch has been waiting on author since the 15th of January, > > mark it as RwF. > > - If it has been left as waiting on author after the 15th of January, > > move it to the next CF. > > Thanks. Note that I was planning to do that on Monday, as it didn't seemed > rushed enough to spend time on it during the weekend. And that's now done. I also sent an email to warn the authors of those patches and closed the 2022-01 commit fest.
Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit
Hi Julien, On Sun, Jan 16, 2022 at 1:07 PM Julien Rouhaud wrote: > On Thu, Jan 06, 2022 at 05:29:23PM +0900, Etsuro Fujita wrote: > > Done. Attached is a new version. > I also see that Fuji-san raised some questions, so for now I will simply > change > the patch status to Waiting on Author. Thanks for letting me know! I posted a new version of the patchset which addresses Fujii-san’s comments. I changed the status and moved the patchset to the next commitfest. Best regards, Etsuro Fujita
Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit
On Thu, Jan 13, 2022 at 11:54 AM Fujii Masao wrote: > At first I'm reading the 0001 patch. Here are the comments for the patch. Thanks for reviewing! > 0001 patch failed to be applied. Could you rebase the patch? Done. Attached is an updated version of the patch set. > + entry->changing_xact_state = true; > + do_sql_command_begin(entry->conn, "DEALLOCATE ALL"); > + pending_deallocs = lappend(pending_deallocs, entry); > > Originally entry->changing_xact_state is not set to true when executing > DEALLOCATE ALL. But the patch do that. Why do we need this change? > > The source comment explains that we intentionally ignore errors in the > DEALLOCATE. But the patch changes DEALLOCATE ALL so that it's executed via > do_sql_command_begin() that can cause an error. Is this OK? > > + if (ignore_errors) > + pgfdw_report_error(WARNING, res, conn, true, sql); > > When DEALLOCATE fails, originally even warning message is not logged. But the > patch changes DEALLOCATE so that its result is received via > do_sql_command_end() that can log warning message even when ignore_errors > argument is enabled. Why do we need to change the behavior? Yeah, we don’t need to change the behavior as discussed before, so I fixed these. I worked on the patch after a while, so I forgot about that. :-( > + > + This option controls whether postgres_fdw commits > + multiple remote (sub)transactions opened in a local (sub)transaction > + in parallel when the local (sub)transaction commits. > > Since parallel_commit is an option for foreign server, how the server with > this option enabled is handled by postgres_fdw should be documented, instead? Agreed. I rewrote this slightly like the attached. Does that make sense? > + > +Note that if many remote (sub)transactions are opened on a remote server > +in a local (sub)transaction, this option might increase the remote > +server’s load significantly when those remote (sub)transactions are > +committed. So be careful when using this option. > + > > This paragraph should be inside the listitem for parallel_commit, shouldn't > it? I put this note outside, because it’s rewritten to a note about both the parallel_commit and parallel_abort options in the following patch. But it would be good to make the parallel-commit patch independent, so I moved it into the list. > async_capable=true also may cause the similar issue? If so, this kind of note > should be documented also in async_capable? That’s right. I think it would be good to add a similar note about that, but I’d like to leave that for another patch. > This explains that the remote server's load will be increased > *significantly*. But "significantly" part is really true? I think that that would depend on how many transactions are committed on the remote side at the same time. But the word “significantly” might be too strong, so I dropped the word. > I'd like to know how much parallel_commit=true actually can increase the load > in a remote server. Ok, I’ll do a load test. About the #0002 patch: This is in preparation for the parallel-abort patch (#0003), but I’d like to propose a minor cleanup for commit 85c696112: 1) build an abort command to be sent to the remote in pgfdw_abort_cleanup(), using a macro, only when/if necessary, as before, and 2) add/modify comments a little bit. Sorry for the delay again. Best regards, Etsuro Fujita v3-0001-postgres-fdw-Add-support-for-parallel-commit.patch Description: Binary data v3-0002-postgres_fdw-Minor-cleanup-for-pgfdw_abort_cleanup.patch Description: Binary data v3-0003-postgres-fdw-Add-support-for-parallel-abort.patch Description: Binary data
Move replication slot structures/enums/macros to a new header file for better usability by external tools/modules
Hi,
While working on pg_replslotdata tool [1], it was observed that some
of the replication slot structures/enums/macros such as
ReplicationSlotPersistentData, ReplicationSlotPersistency,
ReplicationSlotOnDisk, ReplicationSlotOnDisk etc. are currently in
replication/slot.h and replication/slot.c. This makes the replication
slot on disk data structures unusable by the external tools/modules.
How about moving these structures to a new header file called
slot_common.h as attached in the patch here?
Thoughts?
PS: I'm planning to have the tool separately, as it was rejected to be in core.
[1]
https://www.postgresql.org/message-id/CALj2ACW0rV5gWK8A3m6_X62qH%2BVfaq5hznC%3Di0R5Wojt5%2Byhyw%40mail.gmail.com
Regards,
Bharath Rupireddy.
From 8cdae73e6126118d2826b68a5e0037effa209b29 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Sun, 6 Feb 2022 05:41:43 +
Subject: [PATCH v1] Add new header file for replication slot common data
structures
This makes the external tools/modules to use the replication
slot data structures.
---
src/backend/replication/slot.c| 39
src/include/replication/slot.h| 82 +---
src/include/replication/slot_common.h | 135 ++
3 files changed, 136 insertions(+), 120 deletions(-)
create mode 100644 src/include/replication/slot_common.h
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index e5e0cf8768..42dc297a03 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -50,45 +50,6 @@
#include "storage/procarray.h"
#include "utils/builtins.h"
-/*
- * Replication slot on-disk data structure.
- */
-typedef struct ReplicationSlotOnDisk
-{
- /* first part of this struct needs to be version independent */
-
- /* data not covered by checksum */
- uint32 magic;
- pg_crc32c checksum;
-
- /* data covered by checksum */
- uint32 version;
- uint32 length;
-
- /*
- * The actual data in the slot that follows can differ based on the above
- * 'version'.
- */
-
- ReplicationSlotPersistentData slotdata;
-} ReplicationSlotOnDisk;
-
-/* size of version independent data */
-#define ReplicationSlotOnDiskConstantSize \
- offsetof(ReplicationSlotOnDisk, slotdata)
-/* size of the part of the slot not covered by the checksum */
-#define ReplicationSlotOnDiskNotChecksummedSize \
- offsetof(ReplicationSlotOnDisk, version)
-/* size of the part covered by the checksum */
-#define ReplicationSlotOnDiskChecksummedSize \
- sizeof(ReplicationSlotOnDisk) - ReplicationSlotOnDiskNotChecksummedSize
-/* size of the slot data that is version dependent */
-#define ReplicationSlotOnDiskV2Size \
- sizeof(ReplicationSlotOnDisk) - ReplicationSlotOnDiskConstantSize
-
-#define SLOT_MAGIC 0x1051CA1 /* format identifier */
-#define SLOT_VERSION 2 /* version for new files */
-
/* Control array for replication slot management */
ReplicationSlotCtlData *ReplicationSlotCtl = NULL;
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index f833b1d6dc..657d91f44d 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -15,89 +15,9 @@
#include "storage/lwlock.h"
#include "storage/shmem.h"
#include "storage/spin.h"
+#include "replication/slot_common.h"
#include "replication/walreceiver.h"
-/*
- * Behaviour of replication slots, upon release or crash.
- *
- * Slots marked as PERSISTENT are crash-safe and will not be dropped when
- * released. Slots marked as EPHEMERAL will be dropped when released or after
- * restarts. Slots marked TEMPORARY will be dropped at the end of a session
- * or on error.
- *
- * EPHEMERAL is used as a not-quite-ready state when creating persistent
- * slots. EPHEMERAL slots can be made PERSISTENT by calling
- * ReplicationSlotPersist(). For a slot that goes away at the end of a
- * session, TEMPORARY is the appropriate choice.
- */
-typedef enum ReplicationSlotPersistency
-{
- RS_PERSISTENT,
- RS_EPHEMERAL,
- RS_TEMPORARY
-} ReplicationSlotPersistency;
-
-/*
- * On-Disk data of a replication slot, preserved across restarts.
- */
-typedef struct ReplicationSlotPersistentData
-{
- /* The slot's identifier */
- NameData name;
-
- /* database the slot is active on */
- Oid database;
-
- /*
- * The slot's behaviour when being dropped (or restored after a crash).
- */
- ReplicationSlotPersistency persistency;
-
- /*
- * xmin horizon for data
- *
- * NB: This may represent a value that hasn't been written to disk yet;
- * see notes for effective_xmin, below.
- */
- TransactionId xmin;
-
- /*
- * xmin horizon for catalog tuples
- *
- * NB: This may represent a value that hasn't been written to disk yet;
- * see notes for effective_xmin, below.
- */
- TransactionId catalog_xmin;
-
- /* oldest LSN that might be required by this replication slot */
- XLogRecPtr restart_lsn;
-
- /* restart_lsn is copied here when the slot is invalidated */
- XLogRecPtr invalidated_at;
-
- /*
- * Oldest LSN that the client
Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
On 2022/02/07 12:02, Kyotaro Horiguchi wrote: - If any later checkpoint/restartpoint has been established, just skip remaining task then return false. (!chkpt_was_latest) (I'm not sure this can happen, though.) - we update control file only when archive recovery is still ongoing. This comment seems to conflict with what your PoC patch does. Because with the patch, ControlFile->checkPoint and ControlFile->checkPointCopy seem to be updated even when ControlFile->state != DB_IN_ARCHIVE_RECOVERY. I agree with what your PoC patch does for now. When we're not in archive recovery state, checkpoint and REDO locations in pg_control should be updated but min recovery point should be reset to invalid one (which instruments that subsequent crash recovery should replay all available WAL files). - Otherwise reset minRecoveryPoint then continue. Do you have any thoughts or opinions? Regarding chkpt_was_latest, whether the state is DB_IN_ARCHIVE_RECOVERY or not, if checkpoint and redo locations in pg_control are updated, IMO we don't need to skip the "remaining tasks". Since those locations are updated and subsequent crash recovery will start from that redo location, for example, ISTM that we can safely delete old WAL files prior to the redo location as the "remaining tasks". Thought? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: pg_walfile_name uses XLByteToPrevSeg
At Mon, 07 Feb 2022 13:21:53 +0900 (JST), Kyotaro Horiguchi
wrote in
> At Fri, 4 Feb 2022 14:50:57 -0800, Nathan Bossart
> wrote in
> > On Fri, Feb 04, 2022 at 09:17:54AM -0500, Robert Haas wrote:
> > > On Fri, Feb 4, 2022 at 9:05 AM Ashutosh Bapat
> > > wrote:
> > >> And it gives some surprising results as well
> > >> ---
> > >> #select pg_walfile_name('0/0'::pg_lsn);
> > >> pg_walfile_name
> > >> --
> > >> 000100FF
> > >> (1 row)
> > >>
> > >
> > > Yeah, that seems wrong.
> >
> > It looks like it's been this way for a while (704ddaa).
> > pg_walfile_name_offset() has the following comment:
> >
> > * Note that a location exactly at a segment boundary is taken to be in
> > * the previous segment. This is usually the right thing, since the
> > * expected usage is to determine which xlog file(s) are ready to archive.
> >
> > I see a couple of discussions about this as well [0] [1].
> >
> > [0]
> > https://www.postgresql.org/message-id/flat/1154384790.3226.21.camel%40localhost.localdomain
> > [1]
> > https://www.postgresql.org/message-id/flat/15952.1154827205%40sss.pgh.pa.us
>
> Yes, its the deliberate choice of design, or a kind of
> questionable-but-unoverturnable decision. I think there are many
> external tools conscious of this behavior.
>
> It is also described in the documentation.
>
> https://www.postgresql.org/docs/current/functions-admin.html
> > When the given write-ahead log location is exactly at a write-ahead
> > log file boundary, both these functions return the name of the
> > preceding write-ahead log file. This is usually the desired behavior
> > for managing write-ahead log archiving behavior, since the preceding
> > file is the last one that currently needs to be archived.
I forgot to mentino, but I don't think we need to handle the
wrap-around case of the function.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pg_walfile_name uses XLByteToPrevSeg
At Fri, 4 Feb 2022 14:50:57 -0800, Nathan Bossart
wrote in
> On Fri, Feb 04, 2022 at 09:17:54AM -0500, Robert Haas wrote:
> > On Fri, Feb 4, 2022 at 9:05 AM Ashutosh Bapat
> > wrote:
> >> And it gives some surprising results as well
> >> ---
> >> #select pg_walfile_name('0/0'::pg_lsn);
> >> pg_walfile_name
> >> --
> >> 000100FF
> >> (1 row)
> >>
> >
> > Yeah, that seems wrong.
>
> It looks like it's been this way for a while (704ddaa).
> pg_walfile_name_offset() has the following comment:
>
> * Note that a location exactly at a segment boundary is taken to be in
> * the previous segment. This is usually the right thing, since the
> * expected usage is to determine which xlog file(s) are ready to archive.
>
> I see a couple of discussions about this as well [0] [1].
>
> [0]
> https://www.postgresql.org/message-id/flat/1154384790.3226.21.camel%40localhost.localdomain
> [1]
> https://www.postgresql.org/message-id/flat/15952.1154827205%40sss.pgh.pa.us
Yes, its the deliberate choice of design, or a kind of
questionable-but-unoverturnable decision. I think there are many
external tools conscious of this behavior.
It is also described in the documentation.
https://www.postgresql.org/docs/current/functions-admin.html
> When the given write-ahead log location is exactly at a write-ahead
> log file boundary, both these functions return the name of the
> preceding write-ahead log file. This is usually the desired behavior
> for managing write-ahead log archiving behavior, since the preceding
> file is the last one that currently needs to be archived.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: 2022-02-10 release announcement draft
On Sun, Feb 06, 2022 at 08:01:02PM -0500, Jonathan S. Katz wrote: > Hi, > > Attached is a draft for the release announcement for the 2022-02-10 > cumulative update release. > > Please review for technical accuracy or if you believe any items should be > added/removed. Please provide feedback no later than 2020-02-10 0:00 AoE[1]. I guess you mean 2022 ;) > * The [`psql`](https://www.postgresql.org/docs/current/app-psql.html) > `\password` command now defaults to setting the password for to the role > defined > by `CURRENT_USER`. Additionally, the role name is now included in the password > prompt. s/for to/for/ > * Build extended statistics for partitioned tables. If you previously added > extended statistics to a partitioned table, you can fix this by running > [`ANALYZE`](https://www.postgresql.org/docs/current/sql-analyze.html). > [`autovacuum`](https://www.postgresql.org/docs/current/routine-vacuuming.html#AUTOVACUUM) > currently does not process these statistics, so you must periodically run > `ANALYZE` on any partitioned tables that are using extended statistics. Instead of "you can fix this" , I suggest to say "you should run ANALYZE on those tables to collect updated statistics." It should say "not process these TABLES" (not stats), and should not say "that are using extended statistics" since partitioned tables should be manually analyzed whether or not they have extended stats. It's good to say "..you should periodically run ANALYZE on partitioned tables to collect updated statistics." (even though this patch doesn't change that). > * Fix crash with > [multiranges](https://www.postgresql.org/docs/current/rangetypes.html) > when extracting a variable-length data types. remove "a" or s/types/types/ > * Disallow altering data type of a partitioned table's columns when the > partitioned table's row type is used as a composite type elsewhere. the data types ? -- Justin
Re: [BUG]Update Toast data failure in logical replication
On Sun, Feb 6, 2022 at 5:04 AM Andres Freund wrote:
>
> On 2022-02-04 17:45:36 +0530, Amit Kapila wrote:
> > diff --git a/contrib/test_decoding/expected/toast.out
> > b/contrib/test_decoding/expected/toast.out
> > index cd03e9d..a757e7d 100644
> > --- a/contrib/test_decoding/expected/toast.out
> > +++ b/contrib/test_decoding/expected/toast.out
> > @@ -77,7 +77,7 @@ SELECT substr(data, 1, 200) FROM
> > pg_logical_slot_get_changes('regression_slot',
> > table public.toasted_key: INSERT: id[integer]:1
> > toasted_key[text]:'1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123
> > COMMIT
> > BEGIN
> > - table public.toasted_key: UPDATE: id[integer]:1
> > toasted_key[text]:unchanged-toast-datum
> > toasted_col1[text]:unchanged-toast-datum
> > toasted_col2[text]:'987654321098765432109876543210987654321098765432109
> > + table public.toasted_key: UPDATE: old-key:
> > toasted_key[text]:'123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678
>
> Hm. This looks weird. What happened to the the change to toasted_col2 that was
> in the "removed" line?
>
> This corresponds to the following statement I think:
> -- test update of a toasted key without changing it
> UPDATE toasted_key SET toasted_col2 = toasted_col1;
> which previously was inserted as:
> INSERT INTO toasted_key(toasted_key, toasted_col1)
> VALUES(repeat('1234567890', 200), repeat('9876543210', 200));
>
> so toasted_col2 should have changed from NULL to repeat('9876543210', 200)
>
Right, and it is getting changed. We are just printing the first 200
characters (by using SQL [1]) from the decoded tuple so what is shown
in the results is the initial 200 bytes. The complete decoded data
after the patch is as follows:
table public.toasted_key: UPDATE: old-key:
toasted_key[text]:'12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890'
new-tuple: id[integer]:2 toasted_key[text]:unchanged-toast-datum
toasted_col1[text]:unchanged-toast-datum
Re: GUC flags
Thanks for working on it. Your test is checking that stuff in sample.conf is actually a GUC and not marked NOT_IN_SAMPLE. But those are both unlikely mistakes to make. The important/interesting test is the opposite: that all GUCs are present in the sample file. It's a lot easier for someone to forget to add a GUC to sample.conf than it is for someone to accidentally add something that isn't a GUC. I'd first parse the GUC-like lines in the file, making a list of gucs_in_file and then compare the two lists. -- Justin
Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
At Mon, 07 Feb 2022 10:16:34 +0900 (JST), Kyotaro Horiguchi
wrote in
> At Fri, 4 Feb 2022 10:59:04 +0900, Fujii Masao
> wrote in
> > On 2022/02/03 15:50, Kyotaro Horiguchi wrote:
> > > By the way, restart point should start only while recoverying, and at
> > > the timeof the start both checkpoint.redo and checkpoint LSN are
> > > already past. We shouldn't update minRecovery point after promotion,
> > > but is there any reason for not updating the checkPoint and
> > > checkPointCopy? If we update them after promotion, the
> > > which-LSN-to-show problem would be gone.
> >
> > I tried to find the reason by reading the past discussion, but have
> > not found that yet.
> >
> > If we update checkpoint and REDO LSN at pg_control in that case, we
> > also need to update min recovery point at pg_control? Otherwise the
> > min recovery point at pg_control still indicates the old LSN that
> > previous restart point set.
>
> I had an assuption that the reason I think it shouldn't update
> minRecoveryPoint is that it has been or is going to be reset to
> invalid LSN by promotion and the checkpoint should refrain from
> touching it.
Hmm.. It doesn't seem to be the case. If a server crashes just after
promotion and before requesting post-promtion checkpoint,
minRecoveryPoint stays at a valid LSN.
(Promoted at 0/728)
Database cluster state: in production
Latest checkpoint location: 0/660
Latest checkpoint's REDO location:0/628
Latest checkpoint's REDO WAL file:00010006
Minimum recovery ending location: 0/790
Min recovery ending loc's timeline: 2
minRecoveryPoint/TLI are ignored in any case where a server in
in-production state is started. In other words, the values are
useless. There's no clear or written reason for unrecording the last
ongoing restartpoint after promotion.
Before fast-promotion was introduced, we shouldn't get there after
end-of-recovery checkpoint (but somehow reached sometimes?) but it is
quite normal nowadays. Or to the contrary, we're expecting it to
happen and it is regarded as a normal checkponit. So we should do
there nowadays are as the follows.
- If any later checkpoint/restartpoint has been established, just skip
remaining task then return false. (!chkpt_was_latest)
(I'm not sure this can happen, though.)
- we update control file only when archive recovery is still ongoing.
- Otherwise reset minRecoveryPoint then continue.
Do you have any thoughts or opinions?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 958220c495..ab8a4d9a1b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9658,6 +9658,7 @@ CreateRestartPoint(int flags)
XLogRecPtr endptr;
XLogSegNo _logSegNo;
TimestampTz xtime;
+ boolchkpt_was_latest = false;
/* Get a local copy of the last safe checkpoint record. */
SpinLockAcquire(>info_lck);
@@ -9752,44 +9753,69 @@ CreateRestartPoint(int flags)
PriorRedoPtr = ControlFile->checkPointCopy.redo;
/*
-* Update pg_control, using current time. Check that it still shows
-* DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do
nothing;
-* this is a quick hack to make sure nothing really bad happens if
somehow
-* we get here after the end-of-recovery checkpoint.
+* Update pg_control, using current time if no later checkpoints have
been
+* performed.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
- if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY &&
- ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
+ if (ControlFile->checkPointCopy.redo < lastCheckPoint.redo)
{
+ chkpt_was_latest = true;
ControlFile->checkPoint = lastCheckPointRecPtr;
ControlFile->checkPointCopy = lastCheckPoint;
/*
-* Ensure minRecoveryPoint is past the checkpoint record.
Normally,
-* this will have happened already while writing out dirty
buffers,
-* but not necessarily - e.g. because no buffers were dirtied.
We do
-* this because a non-exclusive base backup uses
minRecoveryPoint to
-* determine which WAL files must be included in the backup,
and the
-* file (or files) containing the checkpoint record must be
included,
-* at a minimum. Note that for an ordinary restart of recovery
there's
-* no value in having the minimum recovery point any earlier
than this
+* Ensure minRecoveryPoint is past the checkpoint record while
archive
+* recovery is still ongoing. Normally, this will have happened
+* already while writing out dirty buffers,
Re: pg_receivewal - couple of improvements
On Sun, Feb 06, 2022 at 01:01:41PM +0530, Bharath Rupireddy wrote: > With the start position as an option, users can just provide the LSN > from which they want to stream the WAL, in the above case, it can be > from primary's latest checkpoint LSN. This still strikes me as a dangerous thing to have, prone to errors with a bunch of base backups wasted if one does a mistake as it would be very easy to cause holes in the WAL stored, until one has to redeploy from a base backup in urgency. This kind of control is provided by replication slots for new locations, and the current archive location if anything is stored, so I would leave it at that. On top of that, this kind of control is prone to more race conditions with the backend, as a concurrent checkpoint could make the LSN you are looking for irrelevant. -- Michael signature.asc Description: PGP signature
Re: GUC flags
On Sun, Feb 06, 2022 at 02:09:45PM +0900, Michael Paquier wrote:
> Actually, I am thinking that we should implement it before retiring
> completely check_guc, but not in the fashion you are suggesting. I
> would be tempted to add something in the TAP tests as of
> src/test/misc/, where we initialize an instance to get the information
> about all the GUCs from SQL, and map that to the sample file located
> at pg_config --sharedir. I actually have in my patch set for
> pg_upgrade's TAP a perl routine that could be used for this purpose,
> as of the following in Cluster.pm:
I have been poking at that, and this is finishing to be pretty
elegant as of the attached. With this in place, we are able to
cross-check GUCs marked as NOT_IN_SAMPLE (or not) with the contents of
postgresql.conf.sample, so as check_guc could get retired without us
losing much.
I am planning to apply the Cluster.pm part of the patch separately,
for clarity, as I want this routine in place for some other patch.
--
Michael
From 5c3fbf6af5111927aa7a61025abfce87a9bd230e Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 7 Feb 2022 11:32:17 +0900
Subject: [PATCH] Add TAP test to automate check_guc
---
src/test/modules/test_misc/t/003_check_guc.pl | 77 +++
src/test/perl/PostgreSQL/Test/Cluster.pm | 25 ++
2 files changed, 102 insertions(+)
create mode 100644 src/test/modules/test_misc/t/003_check_guc.pl
diff --git a/src/test/modules/test_misc/t/003_check_guc.pl b/src/test/modules/test_misc/t/003_check_guc.pl
new file mode 100644
index 00..cd64fb8b49
--- /dev/null
+++ b/src/test/modules/test_misc/t/003_check_guc.pl
@@ -0,0 +1,77 @@
+# Tests to cross-check the consistency of GUC parameters with
+# postgresql.conf.sample.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->start;
+
+# Grab the names of all the parameters that can be listed in the
+# configuration sample file.
+my $all_params = $node->safe_psql(
+ 'postgres',
+ "SELECT name
+ FROM pg_settings
+ WHERE NOT 'NOT_IN_SAMPLE' = ANY (pg_settings_get_flags(name))
+ ORDER BY 1");
+# Note the lower-case conversion, for consistency.
+my @all_params_array = split("\n", lc($all_params));
+
+# Grab the names of all parameters marked as NOT_IN_SAMPLE.
+my $not_in_sample = $node->safe_psql(
+ 'postgres',
+ "SELECT name
+ FROM pg_settings
+ WHERE 'NOT_IN_SAMPLE' = ANY (pg_settings_get_flags(name))
+ ORDER BY 1");
+# Note the lower-case conversion, for consistency.
+my @not_in_sample_array = split("\n", lc($not_in_sample));
+
+# Find the location of postgresql.conf.sample, based on the information
+# provided by pg_config.
+my $sample_file =
+ $node->config_data('--sharedir') . '/postgresql.conf.sample';
+
+# Read the sample file line-by-line, checking its contents.
+my $num_tests = 0;
+open(my $contents, '<', $sample_file)
+ || die "Could not open $sample_file: $!";
+while (my $line = <$contents>)
+{
+ # Check if this line matches a GUC parameter.
+ if ($line =~ m/^#?([_[:alpha:]]+) (= .*|[^ ]*$)/)
+ {
+ # Lower-case conversion matters for some of the GUCs.
+ my $param_name = lc($1);
+
+ # Ignore some exceptions.
+ next if $param_name eq "include";
+ next if $param_name eq "include_dir";
+ next if $param_name eq "include_if_exists";
+
+ my $marked_all_params = grep(/^$param_name$/, @all_params_array);
+ my $marked_not_in_sample =
+ grep(/^$param_name$/, @not_in_sample_array);
+
+ # All parameters marked as NOT_IN_SAMPLE cannot be in
+ # the sample file.
+ is($marked_not_in_sample, 0,
+ "checked $param_name not marked with NOT_IN_SAMPLE");
+ # All parameters *not* marked as NOT_IN_SAMPLE can be
+ # in the sample file.
+ is($marked_all_params, 1,
+ "checked $param_name in postgresql.conf.sample");
+
+ # Update test count.
+ $num_tests += 2;
+ }
+}
+
+close $contents;
+
+plan tests => $num_tests;
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 265f3ae657..832d119f2e 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -327,6 +327,31 @@ sub install_path
=pod
+=item $node->config_data($option)
+
+Grab some data from pg_config, with $option being the option switch
+used to grab the wanted data.
+
+=cut
+
+sub config_data
+{
+ my ($self, $option) = @_;
+ local %ENV = $self->_get_env();
+
+ my ($stdout, $stderr);
+ my $result =
+ IPC::Run::run [ $self->installed_command('pg_config'), $option ],
+ '>', \$stdout, '2>', \$stderr
+ or die "could not execute pg_config";
+ chomp($stdout);
+ $stdout =~ s/\r$//;
+
+ return $stdout;
+}
+
+=pod
+
=item $node->info()
Return a string containing human-readable diagnostic information (paths, etc)
--
2.34.1
signature.asc
Description: PGP signature
Re: [PATCH] Add min() and max() aggregate functions for xid8
On 2022/02/05 10:46, Ken Kato wrote:
Thank you for the comments.
I sent my old version of patch by mistake.
This is the updated one.
Thanks!
+ PG_RETURN_FULLTRANSACTIONID((FullTransactionIdFollowsOrEquals(fxid1,
fxid2)) ? fxid1 : fxid2);
Basically it's better to use less 80 line length for readability. So how about
change the format of this to the following?
if (FullTransactionIdFollows(fxid1, fxid2))
PG_RETURN_FULLTRANSACTIONID(fxid1);
else
PG_RETURN_FULLTRANSACTIONID(fxid2);
+insert into xid8_tab values ('0'::xid8), ('18446744073709551615'::xid8);
Isn't it better to use '0x'::xid8 instead of
'18446744073709551615'::xid8, to more easily understand that this test uses
maximum number allowed as xid8?
In addition to those two xid8 values, IMO it's better to insert also the xid8
value neither minimum nor maximum xid8 ones, for example, '42'::xid8.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
At Fri, 4 Feb 2022 10:59:04 +0900, Fujii Masao wrote in > > > On 2022/02/03 15:50, Kyotaro Horiguchi wrote: > > On way to take. In that case should we log something like > > "Restartpoint canceled" or something? > > +1 > > > > By the way, restart point should start only while recoverying, and at > > the timeof the start both checkpoint.redo and checkpoint LSN are > > already past. We shouldn't update minRecovery point after promotion, > > but is there any reason for not updating the checkPoint and > > checkPointCopy? If we update them after promotion, the > > which-LSN-to-show problem would be gone. > > I tried to find the reason by reading the past discussion, but have > not found that yet. > > If we update checkpoint and REDO LSN at pg_control in that case, we > also need to update min recovery point at pg_control? Otherwise the > min recovery point at pg_control still indicates the old LSN that > previous restart point set. I had an assuption that the reason I think it shouldn't update minRecoveryPoint is that it has been or is going to be reset to invalid LSN by promotion and the checkpoint should refrain from touching it. -- Kyotaro Horiguchi NTT Open Source Software Center
2022-02-10 release announcement draft
Hi, Attached is a draft for the release announcement for the 2022-02-10 cumulative update release. Please review for technical accuracy or if you believe any items should be added/removed. Please provide feedback no later than 2020-02-10 0:00 AoE[1]. Thanks, Jonathan [1] https://en.wikipedia.org/wiki/Anywhere_on_Earth The PostgreSQL Global Development Group has released an update to all supported versions of PostgreSQL, including 14.2, 13.6, 12.10, 11.15, and 10.20. This release fixes over 55 bugs reported over the last three months. For the full list of changes, please review the [release notes](https://www.postgresql.org/docs/release/). Bug Fixes and Improvements -- This update fixes over 55 bugs that were reported in the last several months. The issues listed below affect PostgreSQL 14. Some of these issues may also affect other supported versions of PostgreSQL. Included in this release: * Fix for a low probability scenario of index corruption when a HOT ([heap-only tuple](https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/heap/README.HOT;hb=HEAD)) chain changes state during `VACUUM`. Encountering this issue is unlikely, but if you are concerned, please consider [reindexing](https://www.postgresql.org/docs/current/sql-reindex.html). * Fix for using [`REINDEX CONCURRENTLY`](https://www.postgresql.org/docs/current/sql-reindex.html) on [TOAST](https://www.postgresql.org/docs/current/storage-toast.html) table indexes to prevent corruption. You can fix any TOAST indexes by [reindexing](https://www.postgresql.org/docs/current/sql-reindex.html) them again. * The [`psql`](https://www.postgresql.org/docs/current/app-psql.html) `\password` command now defaults to setting the password for to the role defined by `CURRENT_USER`. Additionally, the role name is now included in the password prompt. * Build extended statistics for partitioned tables. If you previously added extended statistics to a partitioned table, you can fix this by running [`ANALYZE`](https://www.postgresql.org/docs/current/sql-analyze.html). [`autovacuum`](https://www.postgresql.org/docs/current/routine-vacuuming.html#AUTOVACUUM) currently does not process these statistics, so you must periodically run `ANALYZE` on any partitioned tables that are using extended statistics. * Fix crash with [`ALTER STATISTICS`](https://www.postgresql.org/docs/current/sql-alterstatistics.html) when the statistics object is dropped concurrently. * Fix crash with [multiranges](https://www.postgresql.org/docs/current/rangetypes.html) when extracting a variable-length data types. * Several fixes to the query planner that lead to incorrect query results. * Several fixes for query plan [memoization](https://www.postgresql.org/docs/current/runtime-config-query.html#GUC-ENABLE-MEMOIZE). * Fix startup of a physical replica to tolerate transaction ID wraparound. * When using logical replication, avoid duplicate transmission of a partitioned table's data when the publication includes both the child and parent tables. * Disallow altering data type of a partitioned table's columns when the partitioned table's row type is used as a composite type elsewhere. * Disallow [`ALTER TABLE ... DROP NOT NULL`](https://www.postgresql.org/docs/current/sql-altertable.html) for a column that is part of a replica identity index. * Several fixes for caching that correct logical replication behavior and improve performance. * Fix memory leak when updating expression indexes. * Avoid leaking memory during [`REASSIGN OWNED BY`](https://www.postgresql.org/docs/current/sql-reassign-owned.html) operations that reassign ownership of many objects. * Fix display of whole-row variables appearing in `INSERT ... VALUES` rules. * Fix race condition that could lead to failure to localize error messages that are reported early in multi-threaded use of libpq or ecpglib. * Fix [`psql`](https://www.postgresql.org/docs/current/app-psql.html) `\d` command for identifying parent triggers. * Fix failures on Windows when using the terminal as data source or destination. This affected the psql `\copy` command and using [`pg_recvlogical`](https://www.postgresql.org/docs/current/app-pgrecvlogical.html) with `-f -`. * Fix the [`pg_dump`](https://www.postgresql.org/docs/current/app-pgdump.html) `--inserts` and `--column-inserts` modes to handle tables that contain both generated and dropped columns. * Fix edge cases in how [`postgres_fdw`](https://www.postgresql.org/docs/current/postgres-fdw.html) handles asynchronous queries. These errors could lead to crashes or incorrect results when attempting to run parallel scans of foreign tables. For the full list of changes available, please review the [release notes](https://www.postgresql.org/docs/release/). Updating All PostgreSQL update releases are cumulative. As with other minor releases, users are not required to dump and reload their database or use `pg_upgrade` in order to apply
Re: pg_upgrade should truncate/remove its logs before running
> On Feb 6, 2022, at 7:39 PM, Tom Lane wrote: > > Michael Paquier writes: >>> On Sun, Feb 06, 2022 at 08:32:59AM -0500, Andrew Dunstan wrote: >>> But the commit really shouldn't have happened until we know that most >>> buildfarm owners have installed it. It should have waited wait not just >>> for the release but for widespread deployment. Otherwise we will just >>> lose any logging for an error that might appear. > >> Would it be better if I just revert the change for now then and do it >> again in one/two weeks? > > I don't see a need to revert it. > > I note, though, that there's still not been any email to the buildfarm > owners list about this update. > > It’s stuck in moderation Cheers Andrew
Re: pg_upgrade should truncate/remove its logs before running
Michael Paquier writes: > On Sun, Feb 06, 2022 at 08:32:59AM -0500, Andrew Dunstan wrote: >> But the commit really shouldn't have happened until we know that most >> buildfarm owners have installed it. It should have waited wait not just >> for the release but for widespread deployment. Otherwise we will just >> lose any logging for an error that might appear. > Would it be better if I just revert the change for now then and do it > again in one/two weeks? I don't see a need to revert it. I note, though, that there's still not been any email to the buildfarm owners list about this update. regards, tom lane
Re: pg_upgrade should truncate/remove its logs before running
On Sun, Feb 06, 2022 at 08:32:59AM -0500, Andrew Dunstan wrote: > *sigh* Sometimes I have a mind like a sieve. I prepped the release a few > days ago and meant to come back the next morning and send out emails > announcing it, as well as rolling it out to my animals, and got diverted > so that didn't happen and it slipped my mind. I'll go and do those > things now. Thanks. I saw the release listed after a couple of days of hibernation, and that one week went by since, so I thought that the timing was pretty good. I did not check the buildfarm members though, sorry about that. > But the commit really shouldn't have happened until we know that most > buildfarm owners have installed it. It should have waited wait not just > for the release but for widespread deployment. Otherwise we will just > lose any logging for an error that might appear. Would it be better if I just revert the change for now then and do it again in one/two weeks? The buildfarm is green, so keeping things as they are does not sound like a huge deal to me, either, for this case. FWIW, I have already switched my own animal to use the newest buildfarm client. -- Michael signature.asc Description: PGP signature
Re: GSoC 2022
Hi, On 01/20/22 14:33, Ilaria Battiston wrote: > Unsurprisingly, we'll need to have an Ideas page again, so I've gone ahead > and created one (copying last year's): > > https://wiki.postgresql.org/wiki/GSoC_2022 I've added a project idea about the ongoing PL/Java refactoring. Regards, -Chap
Re: Release notes for February minor releases
On 2/6/22 3:42 PM, Andres Freund wrote: Hi, On 2022-02-06 14:22:25 -0500, Jonathan S. Katz wrote: On 2/6/22 1:44 PM, Andres Freund wrote: I'm working on the release announcement and have been following this thread. Are there steps we can provide to help a user detect that this occurred, even though it's a low-probability? Not realiably currently: https://postgr.es/m/20220205221742.qylnkze5ykc4mabv%40alap3.anarazel.de https://www.postgresql.org/message-id/20220204041935.gf4uwbdxddq6r...@alap3.anarazel.de Thanks. For the release announcement then, it seems like the phrasing would to indicate the probability of hitting this issue is low, but if you are concerned, you should reindex. I should have the first release announcement draft later tonight, which I'll put on its usual separate thread. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: Use generation context to speed up tuplesorts
Hi: On Thu, Jan 20, 2022 at 9:31 AM David Rowley wrote: > > As of now, I still believe we'll need Tomas' patches to allow the > block size to grow up to a maximum size. I think those patches are > likely needed before we think about making tuplesort use generation > contexts. The reason I believe this is that we don't always have good > estimates for the number of tuples we're going to sort. +1 I spent some times to study the case here and my current thought is: we can discuss/commit the minimum committable changes which should be beneficial for some cases and no harm for others. Tomas's patch 0002 would make there are no more blocks needed if we switch to GenerationContext (compared with Standard Context). and David's patch can obviously reduce total memory usage and improve the performance. so IMO Tomas's patch 0002 + David's patch is a committable patchset at first round. and Tomas's 0001 patch would be good to have as well. I double checked Tomas's 0002 patch, it looks good to me. and then applied David's patch with ALLOCSET_DEFAULT_SIZES, testing the same workload. Here is the result (number is tps): work_mem = '4GB' | Test Case | master | patched | |---++-| | Test 1|306 | 406 | | Test 2|225 | 278 | | Test 3|202 | 248 | | Test 4|184 | 218 | | Test 5|281 | 360 | work_mem = '4MB' | Test Case | master | patched | |---++-| | Test 1|124 | 409 | | Test 2|106 | 280 | | Test 3|100 | 249 | | Test 4|97 | 218 | | Test 5|120 | 369 | I didn't get the performance improvement as much as David's at the beginning, I think that is because David uses the ALLOCSET_DEFAULT_MAXSIZE directly which will need less number of times for memory allocation. AFAICS, Tomas's patch 0002 + David's patch should be ready for commit for round 1. We can try other opportunities like use rows estimation to allocate initial memory and GenerationContext improves like 0003/0004. Would this work? -- Best Regards Andy Fan
Re: [RFC] building postgres with meson
On 2/6/22 13:39, Andres Freund wrote: > Hi, > > On 2022-02-06 12:06:41 -0500, Andrew Dunstan wrote: >> Here's a patch. I've tested the perl piece on master and it works fine. >> It applies cleanly down to 9.4, which is before we got transform modules >> (9.5) which fail if we just omit doing this platform-specific piece. > Given https://postgr.es/m/34e972bc-6e75-0754-9e6d-cde2518773a1%40dunslane.net > wouldn't it make sense to simply remove the pexports/gendef logic instead of > moving to gendef? > I haven't found a way to fix the transform builds if we do that. So let's leave that as a separate exercise unless you have a solution for that - this patch is really trivial. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Release notes for February minor releases
Hi, On 2022-02-06 14:22:25 -0500, Jonathan S. Katz wrote: > On 2/6/22 1:44 PM, Andres Freund wrote: > I'm working on the release announcement and have been following this thread. > > Are there steps we can provide to help a user detect that this occurred, > even though it's a low-probability? Not realiably currently: https://postgr.es/m/20220205221742.qylnkze5ykc4mabv%40alap3.anarazel.de https://www.postgresql.org/message-id/20220204041935.gf4uwbdxddq6r...@alap3.anarazel.de Greetings, Andres Freund
Re: Release notes for February minor releases
On Sun, Feb 06, 2022 at 02:22:25PM -0500, Jonathan S. Katz wrote: > I'm working on the release announcement and have been following this thread. > > Are there steps we can provide to help a user detect that this occurred, > even though it's a low-probability? It's the same question as raised here: https://www.postgresql.org/message-id/61fd9a5b.1c69fb81.88a14.5556%40mx.google.com -- Justin
Re: Release notes for February minor releases
On 2/6/22 1:44 PM, Andres Freund wrote: Hi, On 2022-02-06 13:09:41 -0500, Tom Lane wrote: Andres Freund writes: That's obviously to complicated for the release notes. Trying to make it more understandable I came up with the following, which still does not seem great: ... How do you like this wording? I'm also going to swap the order of this item and the TOAST locking item, since that one is seeming like it's much more relevant to most people. +1 I'm working on the release announcement and have been following this thread. Are there steps we can provide to help a user detect that this occurred, even though it's a low-probability? Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: Release notes for February minor releases
Hi, On 2022-02-06 13:09:41 -0500, Tom Lane wrote: > Andres Freund writes: > > That's obviously to complicated for the release notes. Trying to make it > > more > > understandable I came up with the following, which still does not seem > > great: > > ... > > How do you like this wording? > I'm also going to swap the order of this item and the TOAST locking > item, since that one is seeming like it's much more relevant to > most people. +1 Thanks!
Re: [RFC] building postgres with meson
Hi, On 2022-02-06 12:06:41 -0500, Andrew Dunstan wrote: > Here's a patch. I've tested the perl piece on master and it works fine. > It applies cleanly down to 9.4, which is before we got transform modules > (9.5) which fail if we just omit doing this platform-specific piece. Given https://postgr.es/m/34e972bc-6e75-0754-9e6d-cde2518773a1%40dunslane.net wouldn't it make sense to simply remove the pexports/gendef logic instead of moving to gendef? Greetings, Andres Freund
Re: Release notes for February minor releases
Andres Freund writes: > That's obviously to complicated for the release notes. Trying to make it more > understandable I came up with the following, which still does not seem great: > ... How do you like this wording? Fix corruption of HOT chains when a RECENTLY_DEAD tuple changes state to fully DEAD during page pruning (Andres Freund) It was possible for VACUUM to remove a recently-dead tuple while leaving behind a redirect item that pointed to it. When the tuple's item slot is later re-used by some new tuple, that tuple would be seen as part of the pre-existing HOT chain, creating a form of index corruption. If this has happened, reindexing the table should repair the damage. However, this is an extremely low-probability scenario, so we do not recommend reindexing just on the chance that it might have happened. I'm also going to swap the order of this item and the TOAST locking item, since that one is seeming like it's much more relevant to most people. regards, tom lane
Re: [PATCH v2] use has_privs_for_role for predefined roles
Joe Conway writes: > I'd like to pick this patch up and see it through to commit/push. > Presumably that will include back-patching to all supported pg versions. > Before I go through the effort to back-patch, does anyone want to argue > that this should *not* be back-patched? Hm, I'm -0.5 or so. I think changing security-related behaviors in a stable branch is a hard sell unless you are closing a security hole. This is a fine improvement for HEAD but I'm inclined to leave the back branches alone. regards, tom lane
Re: [RFC] building postgres with meson
On 10/13/21 16:06, Andrew Dunstan wrote:
> On 10/13/21 1:26 PM, Andres Freund wrote:
>>> pexports will be in the resulting path, and the build will use the
>>> native compiler.
>> I don't see pexports anywhere in the msys installation. I can see it
>> available
>> on sourceforge, and I see a few others asking where to get it from in the
>> context of msys, and being pointed to manually downloading it.
>
>
> Weird. fairywren has it, which means that it must have been removed from
> the packages at some stage, fairly recently as fairywren isn't that old.
> I just confirmed the absence on a 100% fresh install.
>
>
> It is in Strawberry's c/bin directory.
>
>
>> Seems like we should consider using gendef instead of pexports, given it's
>> available in msys?
>
> Yeah. It's missing on my ancient msys animal (frogmouth), but it doesn't
> build --with-perl.
>
>
> jacana seems to have it.
>
>
> If you prep a patch I'll test it.
>
>
Here's a patch. I've tested the perl piece on master and it works fine.
It applies cleanly down to 9.4, which is before we got transform modules
(9.5) which fail if we just omit doing this platform-specific piece.
Before that only plpython uses pexports, and we're not committed to
supporting plpython at all on old branches.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
commit 823b36d98b
Author: Andrew Dunstan
Date: Sun Feb 6 10:53:06 2022 -0500
Use gendef instead of pexports for building windows .def files
Modern msys systems lack pexports but have gendef instead, so use that.
Discussion: https://postgr.es/m/20d4fa3a-e0f1-0ac4-1657-0698ee1c5...@dunslane.net>
diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile
index 919d46453f..a2e6410f53 100644
--- a/src/pl/plperl/GNUmakefile
+++ b/src/pl/plperl/GNUmakefile
@@ -48,7 +48,7 @@ lib$(perlwithver).a: $(perlwithver).def
dlltool --dllname $(perlwithver).dll --def $(perlwithver).def --output-lib lib$(perlwithver).a
$(perlwithver).def: $(PERLDLL)
- pexports $^ > $@
+ gendef - $^ > $@
endif # win32
diff --git a/src/pl/plpython/Makefile b/src/pl/plpython/Makefile
index 9e95285af8..a83ae8865c 100644
--- a/src/pl/plpython/Makefile
+++ b/src/pl/plpython/Makefile
@@ -69,7 +69,7 @@ libpython${pytverstr}.a: python${pytverstr}.def
dlltool --dllname python${pytverstr}.dll --def python${pytverstr}.def --output-lib libpython${pytverstr}.a
python${pytverstr}.def:
- pexports $(PYTHONDLL) > $@
+ gendef - $(PYTHONDLL) > $@
endif # win32
diff --git a/src/pl/tcl/Makefile b/src/pl/tcl/Makefile
index 1e7740da3f..25e65189b6 100644
--- a/src/pl/tcl/Makefile
+++ b/src/pl/tcl/Makefile
@@ -46,7 +46,7 @@ lib$(tclwithver).a: $(tclwithver).def
dlltool --dllname $(tclwithver).dll --def $(tclwithver).def --output-lib lib$(tclwithver).a
$(tclwithver).def: $(TCLDLL)
- pexports $^ > $@
+ gendef - $^ > $@
endif # win32
Re: [PATCH v2] use has_privs_for_role for predefined roles
On 1/4/22 16:51, Joshua Brindle wrote: On Tue, Jan 4, 2022 at 3:56 PM Tom Lane wrote: "Bossart, Nathan" writes: > On 11/12/21, 12:34 PM, "Joshua Brindle" wrote: >> All of these and also adminpack.sgml updated. I think that is all of >> them but docs broken across lines and irregular wording makes it >> difficult. > LGTM. I've marked this as ready-for-committer. This needs another rebase --- it's trying to adjust file_fdw/output/file_fdw.source, which no longer exists (fix the regular file_fdw.out, instead). regards, tom lane Attached, thanks I'd like to pick this patch up and see it through to commit/push. Presumably that will include back-patching to all supported pg versions. Before I go through the effort to back-patch, does anyone want to argue that this should *not* be back-patched? Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: BufferAlloc: don't take two simultaneous locks
Hello, Yura. A one additional moment: > 1332: Assert((oldFlags & (BM_PIN_COUNT_WAITER | BM_IO_IN_PROGRESS)) == 0); > 1333: CLEAR_BUFFERTAG(buf->tag); > 1334: buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK); > 1335: UnlockBufHdr(buf, buf_state); I think there is no sense to unlock buffer here because it will be locked after a few moments (and no one is able to find it somehow). Of course, it should be unlocked in case of collision. BTW, I still think is better to introduce some kind of hash_update_hash_key and use it. It may look like this: // should be called with oldPartitionLock acquired // newPartitionLock hold on return // oldPartitionLock and newPartitionLock are not taken at the same time // if newKeyPtr is present - existingEntry is removed bool hash_update_hash_key_or_remove( HTAB *hashp, void *existingEntry, const void *newKeyPtr, uint32 newHashValue, LWLock *oldPartitionLock, LWLock *newPartitionLock ); Thanks, Michail.
Re: pg_walinspect - a new extension to get raw WAL data and WAL stats
On Mon, Jan 31, 2022 at 4:40 PM Greg Stark wrote: > So I looked at this patch and I have the same basic question as Bruce. > Do we really want to expose every binary tool associated with Postgres > as an extension? Obviously this is tempting for cloud provider users > which is not an unreasonable argument. But it does have consequences. > > 1) Some things like pg_waldump are running code that is not normally > under user control. This could have security issues or reliability > issues. For what it's worth, I am generally in favor of having something like this in PostgreSQL. I think it's wrong of us to continue assuming that everyone has command-line access. Even when that's true, it's not necessarily convenient. If you choose to use a relational database, you may be the sort of person who likes SQL. And if you are, you may want to have the database tell you what's going on via SQL rather than command-line tools or operating system utilities. Imagine if we didn't have pg_stat_activity and you had to get that information by running a separate binary. Would anyone like that? Why is this case any different? A few years ago we exposed data from pg_control via SQL and similar concerns were raised - but it turns out to be pretty useful. I don't know why this shouldn't be equally useful. Sure, there's some duplication in functionality, but it's not a huge maintenance burden for the project, and people (including me) like having it available. I think the same things will be true here. If decoding WAL causes security problems, that's something we better fix, because WAL is constantly decoded on standbys and via logical decoding on systems all over the place. I agree that we can't let users supply their own hand-crafted WAL records to be decoded without causing more trouble than we can handle, but if it's not safe to decode the WAL the system generated than we are in a lot of trouble already. I hasten to say that I'm not endorsing every detail or indeed any detail of the proposed patch, and some of the concerns you mention later sound well-founded to me. But I disagree with the idea that we shouldn't have both a command-line utility that roots through files on disk and an SQL interface that works with a running system. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_upgrade should truncate/remove its logs before running
On 2/6/22 02:17, Tom Lane wrote: > Michael Paquier writes: >> On Sun, Feb 06, 2022 at 01:58:21AM -0500, Tom Lane wrote: >>> As already mentioned, there's been no notice to buildfarm owners ... >>> so has Andrew actually made a release? >> There has been one as of 8 days ago: >> https://github.com/PGBuildFarm/client-code/releases > [ scrapes buildfarm logs ... ] > > Not even Andrew's own buildfarm critters are using it, so > permit me leave to doubt that he thinks it's fully baked. > > Andrew? > > *sigh* Sometimes I have a mind like a sieve. I prepped the release a few days ago and meant to come back the next morning and send out emails announcing it, as well as rolling it out to my animals, and got diverted so that didn't happen and it slipped my mind. I'll go and do those things now. But the commit really shouldn't have happened until we know that most buildfarm owners have installed it. It should have waited wait not just for the release but for widespread deployment. Otherwise we will just lose any logging for an error that might appear. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Unclear problem reports
On Sun, Feb 6, 2022 at 12:58 AM Noah Misch wrote:
>
> On Sun, Feb 06, 2022 at 12:41:52AM +0100, Magnus Hagander wrote:
> > On Sun, Feb 6, 2022 at 12:02 AM Noah Misch wrote:
> > > On Fri, Feb 04, 2022 at 01:36:46PM -0500, Bruce Momjian wrote:
> > > > On Wed, Feb 2, 2022 at 07:21:19PM -0700, David G. Johnston wrote:
> > > > > Have pg_lister queue up a check for, say, two or three days after the
> > > > > bug
> > > > > reporting form is filled out. If the report hasn't been responded to
> > > > > by
> > > > > someone other than the OP send out a reply that basically says:
> > > > >
> > > > > We're sorry your message hasn't yet attracted a response. If your
> > > > > report falls
> > > > > into the category of "tech support for a malfunctioning server", and
> > > > > this
> > > > > includes error messages that are difficult or impossible to replicate
> > > > > in a
> > > > > development environment (maybe give some examples), you may wish to
> > > > > consider
> > > > > eliciting paid professional help. Please see this page on our
> > > > > website for a
> > > > > directory of companies that provide such services. The PostgreSQL
> > > > > Core Project
> > > > > itself refrains from making recommendations since many, if not all,
> > > > > of these
> > > > > companies contribute back to the project in order to keep it both
> > > > > free and open
> > > > > source.
> > > >
> > > > Yes, that is an idea. I have canned email responses for common issues
> > > > like PGAdmin questions on the bugs list, but for these cases, I don't
> > > > know if someone might actually know the answer, and I don't know how
> > > > long to wait for an answer. Should we be going the other way and state
> > > > on the bugs submission page,
> > > > https://www.postgresql.org/account/submitbug/:
> > > >
> > > > If you are having a serious problem with the software and do not
> > > > receive a reply, consider additional support channels, including
> > > > professional support (https://www.postgresql.org/support/).
> > >
> > > https://www.postgresql.org/account/submitbug/ does say to "ensure you have
> > > read" https://www.postgresql.org/docs/current/bug-reporting.html, which
> > > says,
> > > "If you need help immediately, consider obtaining a commercial support
> > > contract." Hence, this expands on an existing message and makes it more
> > > prominent. I think that's reasonable, though I'm not sure it's a net win.
> > > One could also edit the page that appears after submitting a bug.
> > > Editing a
> > > web page is superior to using canned email responses, for two reasons:
> > >
> > > - Canned responses are noise to every list reader other than the OP.
> > > - Canned responses make the list feel like a sales funnel rather than a
> > > normal
> > > free software mailing list.
> >
> > All bug reports posted through the bug form gets moderated before
> > they're passed through. Moderators also have access to a set of
> > pre-defined canned responses (such as the example of where they should
> > go to report pgadmin bugs). It will also catch posts made directly to
> > -bugs by people who are not subscribed, but people who are subscribed
> > will not have their posts moderated by default.
> >
> > If we're talking about people submitting bug reports, åperhaps a lot
> > can be done with (1) a couple of more such responses and (2) more
> > clear instructions fo the moderators of when they are supposed to use
> > them.
> >
> > Error on the side of letting them through is probably always the best
> > choice, but if it's clear that it can be better handled by a canned
> > response, we do have an actual system for that.
>
> I was referring to David G. Johnston's proposal to send canned email responses
> when a thread (presumably containing a not-obviously-unreasonable PostgreSQL
> question or report) gets no replies in N days. The proposed email directed
> the user toward paid professional services. Sending such a message at
> moderation time would solve the noise problem, but it would make the "sales
> funnel" problem worse. (Also, I figure moderators won't know at moderation
> time which messages will get zero replies.) For which part of $SUBJECT did
> you envision using new moderator responses?
It depends. For some of them it would, and I believe they are used for
that as well. But yes, it will only be able to replace those cases
where people send a canned response today ("report pgadmin bugs over
here instead", "you need to talk to your cloud vendor" and things like
that). It won't help for anything more advanced than that.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
