Re: Remove redundant HandleWalWriterInterrupts()
On Thu, Jan 25, 2024 at 4:40 AM Nathan Bossart wrote: > > On Wed, Jan 24, 2024 at 07:30:17PM +0530, Bharath Rupireddy wrote: > > On Wed, Jan 24, 2024 at 5:50 PM Fujii Masao wrote: > >> Because of commit 1bdd54e662, the code of HandleWalWriterInterrupts() > >> became the same as HandleMainLoopInterrupts(). So I'd like to propose to > >> remove HandleWalWriterInterrupts() and make walwriter use > >> HandleMainLoopInterrupts() instead for improved code simplicity. Thought? > >> > >> Patch attached. > > > > Nice catch. Indeed they both are the same after commit 1bdd54e662. The > > patch LGTM. > > LGTM, too. Thank you both for reviewing! I've pushed the patch. Regards, -- Fujii Masao
Re: dblink query interruptibility
On Thu, Jan 25, 2024 at 5:45 AM Noah Misch wrote:
>
> On Thu, Jan 25, 2024 at 04:23:39AM +0900, Fujii Masao wrote:
> > I found that this patch was committed at d3c5f37dd5 and changed the
> > error message in postgres_fdw slightly. Here's an example:
> >
> > #1. Begin a new transaction.
> > #2. Execute a query accessing to a foreign table, like SELECT * FROM
> >
> > #3. Terminate the *remote* session corresponding to the foreign table.
> > #4. Commit the transaction, and then currently the following error
> > message is output.
> >
> > ERROR: FATAL: terminating connection due to administrator command
> > server closed the connection unexpectedly
> > This probably means the server terminated abnormally
> > before or while processing the request.
> >invalid socket
> >
> > Previously, before commit d3c5f37dd5, the error message at #4 did not
> > include "invalid socket." Now, after the commit, it does. Is this
> > change intentional?
>
> No. It's a consequence of an intentional change in libpq call sequence, but I
> was unaware I was changing an error message.
>
> > + /* Consume whatever data is available from the socket */
> > + if (PQconsumeInput(conn) == 0)
> > + {
> > + /* trouble; expect PQgetResult() to return NULL */
> > + break;
> > + }
> > + }
> > +
> > + /* Now we can collect and return the next PGresult */
> > + return PQgetResult(conn);
> >
> > This code appears to cause the change. When the remote session ends,
> > PQconsumeInput() returns 0 and marks conn->socket as invalid.
> > Subsequent PQgetResult() calls pqWait(), detecting the invalid socket
> > and appending "invalid socket" to the error message.
> >
> > I think the "invalid socket" message is unsuitable in this scenario,
> > and PQgetResult() should not be called after PQconsumeInput() returns
> > 0. Thought?
>
> The documentation is absolute about the necessity of PQgetResult():
The documentation looks unclear to me regarding what should be done
when PQconsumeInput() returns 0. So I'm not sure if PQgetResult()
must be called even in that case.
As far as I read some functions like libpqrcv_PQgetResult() that use
PQconsumeInput(), it appears that they basically report the error message
using PQerrorMessage(), without calling PQgetResult(),
when PQconsumeInput() returns 0.
Regards,
--
Fujii Masao
Re: dblink query interruptibility
On Wed, Nov 22, 2023 at 10:29 AM Noah Misch wrote:
>
> === Background
>
> Something as simple as the following doesn't respond to cancellation. In
> v15+, any DROP DATABASE will hang as long as it's running:
>
> SELECT dblink_exec(
> $$dbname='$$||current_database()||$$' port=$$||current_setting('port'),
> 'SELECT pg_sleep(15)');
>
> https://postgr.es/m/4b584c99.8090...@enterprisedb.com proposed a fix back in
> 2010. Latches and the libpqsrv facility have changed the server programming
> environment since that patch. The problem statement also came up here:
>
> On Thu, Dec 08, 2022 at 06:08:15PM -0800, Andres Freund wrote:
> > dblink.c uses a lot of other blocking libpq functions, which obviously also
> > isn't ok.
>
>
> === Key decisions
>
> This patch adds to libpqsrv facility.
I found that this patch was committed at d3c5f37dd5 and changed the
error message in postgres_fdw slightly. Here's an example:
#1. Begin a new transaction.
#2. Execute a query accessing to a foreign table, like SELECT * FROM
#3. Terminate the *remote* session corresponding to the foreign table.
#4. Commit the transaction, and then currently the following error
message is output.
ERROR: FATAL: terminating connection due to administrator command
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
invalid socket
Previously, before commit d3c5f37dd5, the error message at #4 did not
include "invalid socket." Now, after the commit, it does. Is this
change intentional?
+ /* Consume whatever data is available from the socket */
+ if (PQconsumeInput(conn) == 0)
+ {
+ /* trouble; expect PQgetResult() to return NULL */
+ break;
+ }
+ }
+
+ /* Now we can collect and return the next PGresult */
+ return PQgetResult(conn);
This code appears to cause the change. When the remote session ends,
PQconsumeInput() returns 0 and marks conn->socket as invalid.
Subsequent PQgetResult() calls pqWait(), detecting the invalid socket
and appending "invalid socket" to the error message.
I think the "invalid socket" message is unsuitable in this scenario,
and PQgetResult() should not be called after PQconsumeInput() returns
0. Thought?
Regards,
--
Fujii Masao
Re: Network failure may prevent promotion
On Wed, Jan 24, 2024 at 8:29 PM Fujii Masao wrote: > > On Tue, Jan 23, 2024 at 6:43 PM Heikki Linnakangas wrote: > > There's an existing AmWalReceiverProcess() macro too. Let's use that. > > +1 > > > Hmm, but doesn't bgworker_die() have that problem with exit(1)ing in the > > signal handler? > > Yes, that's a problem. This issue was raised sometimes so far, > but has not been resolved yet. > > > I also wonder if we should replace SignalHandlerForShutdownRequest() > > completely with die(), in all processes? The difference is that > > SignalHandlerForShutdownRequest() uses ShutdownRequestPending, while > > die() uses ProcDiePending && InterruptPending to indicate that the > > signal was received. Or do some of the processes want to check for > > ShutdownRequestPending only at specific places, and don't want to get > > terminated at the any random CHECK_FOR_INTERRUPTS()? > > For example, checkpointer seems to want to handle a shutdown request > only when no other checkpoint is in progress because initiating a shutdown > checkpoint while another checkpoint is running could lead to issues. This my comment is not right... Sorry for noise. Regards, -- Fujii Masao
Remove redundant HandleWalWriterInterrupts()
Hi, Because of commit 1bdd54e662, the code of HandleWalWriterInterrupts() became the same as HandleMainLoopInterrupts(). So I'd like to propose to remove HandleWalWriterInterrupts() and make walwriter use HandleMainLoopInterrupts() instead for improved code simplicity. Thought? Patch attached. Regards, -- Fujii Masao v1-0001-Remove-redundant-HandleWalWriterInterrupts.patch Description: Binary data
Re: Network failure may prevent promotion
On Tue, Jan 23, 2024 at 6:43 PM Heikki Linnakangas wrote: > There's an existing AmWalReceiverProcess() macro too. Let's use that. +1 > Hmm, but doesn't bgworker_die() have that problem with exit(1)ing in the > signal handler? Yes, that's a problem. This issue was raised sometimes so far, but has not been resolved yet. > I also wonder if we should replace SignalHandlerForShutdownRequest() > completely with die(), in all processes? The difference is that > SignalHandlerForShutdownRequest() uses ShutdownRequestPending, while > die() uses ProcDiePending && InterruptPending to indicate that the > signal was received. Or do some of the processes want to check for > ShutdownRequestPending only at specific places, and don't want to get > terminated at the any random CHECK_FOR_INTERRUPTS()? For example, checkpointer seems to want to handle a shutdown request only when no other checkpoint is in progress because initiating a shutdown checkpoint while another checkpoint is running could lead to issues. Also I just wonder if even walreceiver can exit safely at any random CHECK_FOR_INTERRUPTS()... Regards, -- Fujii Masao
Re: Network failure may prevent promotion
On Tue, Jan 23, 2024 at 1:23 PM Kyotaro Horiguchi
wrote:
>
> At Mon, 22 Jan 2024 13:29:10 -0800, Andres Freund wrote
> in
> > Hi,
> >
> > On 2024-01-19 12:28:05 +0900, Michael Paquier wrote:
> > > On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote:
> > > > Given that commit 728f86fec6 that introduced this issue was not strictly
> > > > required, perhaps we should just revert it for v16.
> > >
> > > Is there a point in keeping 728f86fec6 as well on HEAD? That does not
> > > strike me as wise to keep that in the tree for now. If it needs to be
> > > reworked, looking at this problem from scratch would be a safer
> > > approach.
> >
> > IDK, I think we'll introduce this type of bug over and over if we don't fix
> > it
> > properly.
>
> Just to clarify my position, I thought that 728f86fec6 was heading the
> right direction. Considering the current approach to signal handling
> in walreceiver, I believed that it would be better to further
> generalize in this direction rather than reverting. That's why I
> proposed that patch.
Regarding the patch, here are the review comments.
+/*
+ * Is current process a wal receiver?
+ */
+bool
+IsWalReceiver(void)
+{
+ return WalRcv != NULL;
+}
This looks wrong because WalRcv can be non-NULL in processes other
than walreceiver.
- pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
+ pqsignal(SIGTERM, WalRcvShutdownSignalHandler); /* request shutdown */
Can't we just use die(), instead?
Regards,
--
Fujii Masao
Re: Network failure may prevent promotion
On Thu, Jan 18, 2024 at 10:42 PM Heikki Linnakangas wrote: > Given that commit 728f86fec6 that introduced this issue was not strictly > required, perhaps we should just revert it for v16. +1 for the revert. This issue should be fixed in the upcoming minor release since it might cause unexpected delays in failover times. Regards, -- Fujii Masao
Re: pg_rewind with cascade standby doesn't work well
On 2023/09/20 12:04, Michael Paquier wrote: This is a known issue. I guess that the same as this thread and this CF entry: https://commitfest.postgresql.org/44/4244/ https://www.postgresql.org/message-id/flat/zarvomifjze7f...@paquier.xyz I think this is a separate issue, and we should still use Kuwamura-san's patch even after the one you posted on the thread gets accepted. BTW, I was able to reproduce the assertion failure Kuwamura-san reported, even after applying your latest patch from the thread. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: pg_logical_emit_message() misses a XLogFlush()
On 2023/08/16 16:51, Michael Paquier wrote: Anyway, attached is a patch to add a 4th argument "flush" that defaults to false. Thoughts about this version are welcome. When the "transactional" option is set to true, WAL including the record generated by the pg_logical_emit_message() function is flushed at the end of the transaction based on the synchronous_commit setting. However, in the current patch, if "transactional" is set to false and "flush" is true, the function flushes the WAL immediately without considering synchronous_commit. Is this the intended behavior? I'm not sure how the function should work in this case, though. Though I don't understand the purpose of this option fully yet, is flushing the WAL sufficient? Are there scenarios where the function should ensure that the WAL is not only flushed but also replicated to the standby? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Transaction timeout
On 2022/12/19 5:53, Andrey Borodin wrote: On Wed, Dec 7, 2022 at 1:30 PM Andrey Borodin wrote: I hope to address other feedback on the weekend. Thanks for implementing this feature! While testing v4 patch, I noticed it doesn't handle the COMMIT AND CHAIN case correctly. When COMMIT AND CHAIN is executed, I believe the transaction timeout counter should reset and start from zero with the next transaction. However, it appears that the current v4 patch doesn't reset the counter in this scenario. Can you confirm this? With the v4 patch, I found that timeout errors no longer occur during the idle in transaction phase. Instead, they occur when the next statement is executed. Is this the intended behavior? I thought some users might want to use the transaction timeout feature to prevent prolonged transactions and promptly release resources (e.g., locks) in case of a timeout, similar to idle_in_transaction_session_timeout. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Allow pg_archivecleanup to remove backup history files
On 2023/05/31 10:51, torikoshia wrote:
Update the patch according to the advice.
Thanks for updating the patches! I have small comments regarding 0002 patch.
+
+ Remove backup history files.
Isn't it better to document clearly which backup history files to be removed? For
example, "In addition to removing WAL files, remove backup history files with
prefixes logically preceding the oldestkeptwalfile.".
printf(_(" -n, --dry-run dry run, show the names of the files
that would be removed\n"));
+ printf(_(" -b, --clean-backup-history clean up files including backup
history files\n"));
Shouldn't -b option be placed in alphabetical order?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: [DOCS] alter_foreign_table.sgml typo
On 2023/06/08 0:53, Fujii Masao wrote: On 2023/06/07 23:25, Mehmet Emin KARAKAŞ wrote: Fixed typo in SQL. Current: ALTER FOREIGN TABLE myschema.distributors OPTIONS (ADD opt1 'value', SET opt2 'value2', DROP opt3 'value3'); Fixed: ALTER FOREIGN TABLE myschema.distributors OPTIONS (ADD opt1 'value', SET opt2 'value2', DROP opt3); Drop options do not get a value. Thanks for the report! I agree with your findings and the patch looks good to me. I will commit the patch barring any objection. Pushed. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Return value of pg_promote()
On 2023/06/07 2:00, Laurenz Albe wrote: On Tue, 2023-06-06 at 16:35 +0530, Ashutosh Sharma wrote: At present, pg_promote() returns true to the caller on successful promotion of standby, however it returns false in multiple scenarios which includes: 1) The SIGUSR1 signal could not be sent to the postmaster process. 2) The postmaster died during standby promotion. 3) Standby couldn't be promoted within the specified wait time. For an application calling this function, if pg_promote returns false, it is hard to interpret the reason behind it. So I think we should *only* allow pg_promote to return false when the server could not be promoted in the given wait time and in other scenarios it should just throw an error (FATAL, ERROR ... depending on the type of failure that occurred). Please let me know your thoughts on this change. thanks.! As the original author, I'd say that that sounds reasonable, particularly in case #1. If the postmaster dies, we are going to die too, so it probably doesn't matter much. But I think an error is certainly also correct in that case. +1 Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [DOCS] alter_foreign_table.sgml typo
On 2023/06/07 23:25, Mehmet Emin KARAKAŞ wrote: Fixed typo in SQL. Current: ALTER FOREIGN TABLE myschema.distributors OPTIONS (ADD opt1 'value', SET opt2 'value2', DROP opt3 'value3'); Fixed: ALTER FOREIGN TABLE myschema.distributors OPTIONS (ADD opt1 'value', SET opt2 'value2', DROP opt3); Drop options do not get a value. Thanks for the report! I agree with your findings and the patch looks good to me. I will commit the patch barring any objection. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: is_superuser is not documented
On 2023/06/07 23:15, Joseph Koshakow wrote: I think I may have discovered a reason why is_superuser is intentionally undocumented. is_superuser is not updated if a role's superuser attribute is changed by another session. Therefore, is_superuser may show you an incorrect stale value. Perhaps this can be fixed with a show_hook? Otherwise it's probably best not to document a GUC that can show an incorrect value. Or we can correct the description of is_superuser, for example, "True if the current role had superuser privileges when it connected to the database. Note that this parameter doesn't always indicate the current superuser status of the role."? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
On 2023/04/21 16:39, Jelte Fennema wrote:
In my opinion, PQconnectPoll and PQgetCancel should use the same
parsing function or PQconnectPoll should set parsed values, making
unnecessary for PQgetCancel to parse the same parameter
again.
Yes, I totally agree. So I think patch 0002 looks fine.
It seems like we have reached a consensus to push the 0002 patch.
As for back-patching, although the issue it fixes is trivial,
it may be a good idea to back-patch to v12 where parse_int_param()
was added, for easier back-patching in the future. Therefore
I'm thinking to push the 0002 patch at first and back-patch to v12.
Additionally, PQgetCancel should set appropriate error messages
for all failure modes.
I don't think that PQgetCancel should ever set error messages on the
provided conn object though. It's not part of the documented API and
it's quite confusing since there's actually no error on the connection
itself. That this happens for the keepalive parameter was an
unintended sideeffect of 5987feb70b combined with the fact that the
parsing is different. All those parsing functions should never error,
because setting up the connection should already have checked them.
So I think the newly added libpq_append_conn_error calls in patch 0001
should be removed. The AF_UNIX check and the new WARNING in pg_fdw
seem fine though.
Sounds reasonable to me.
Regarding the WARNING message, another idea is to pass the return value
of PQgetCancel() directly to PQcancel() as follows. If NULL is passed,
PQcancel() will detect it and set the proper error message to errbuf.
Then the warning message "WARNING: could not send cancel request:
PQcancel() -- no cancel object supplied" is output. This approach is
similar to how dblink_cancel_query() does. Thought?
cancel = PQgetCancel(conn);
if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
{
ereport(WARNING,
(errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("could not send cancel request: %s",
errbuf)));
PQfreeCancel(cancel);
return false;
}
----
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
pg_stat_io for the startup process
Hi, Regarding pg_stat_io for the startup process, I noticed that the counters are only incremented after the startup process exits, not during WAL replay in standby mode. This is because pgstat_flush_io() is only called when the startup process exits. Shouldn't it be called during WAL replay as well to report IO statistics by the startup process even in standby mode? Also, the pg_stat_io view includes a row with backend_type=startup and context=vacuum, but it seems that the startup process doesn't perform any I/O operations with BAS_VACUUM. If this understanding is right, shouldn't we omit this row from the view? Additionally, I noticed that the view also includes a row with backend_type=startup and context=bulkread / bulkwrite. Do these operations actually occur during startup process? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Fix documentation for max_wal_size and min_wal_size
On 2023/04/22 17:39, Michael Paquier wrote: On Tue, Apr 18, 2023 at 01:46:21AM -0700, sirisha chamarthi wrote: "The default size is 80MB. However, if you have changed the WAL segment size from the default of 16MB with the initdb option --wal-segsize, it will be five times the segment size." Yes, I think that something close to that would be OK. Do others have any comments or extra ideas to offer? If the WAL segment size is changed from the default value of 16MB during initdb, the value of min_wal_size in postgresql.conf is set to five times the new segment size by initdb. However, pg_settings.boot_val (the value of min_wal_size used when the parameter is not otherwise set) is still 80MB. So if pg_settings.boot_val should be documented as the default value, the current description seems OK. Or we can clarify that the default value of min_wal_size is 80MB, but when initdb is run with a non-default segment size, the value in postgresql.conf is changed to five times the new segment size? This will help users better understand the behavior of the setting and how it is affected by changes made during initdb. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
On 2023/04/14 18:59, Etsuro Fujita wrote: The primary message basically should avoid reference to implementation details such as specific structure names like PGcancel, shouldn't it, as per the error message style guide? I do not think that PGcancel is that specific, as it is described in the user-facing documentation [1]. (In addition, the error message I proposed was created by copying the existing error message "could not create OpenSSL BIO structure" in contrib/sslinfo.c.) I think that mentioning PGcancel in the error message could be confusing for average users who are just running a query on a foreign table and encounter the error message after pressing Ctrl-C. They may not understand why the PGcancel struct is referenced in the error message while accessing foreign tables. It could be viewed as an internal detail that is not necessary for the user to know. Although the primary message is the same, the supplemental message provides additional context that can help distinguish which function is reporting the message. If the user is familiar with the PQgetCancel/PQcancel internals, this is true, but if not, I do not think this is always true. Consider this error message, for example: 2023-04-14 17:48:55.862 JST [24344] WARNING: could not send cancel request: invalid integer value "999" for connection option "keepalives" It would be hard for users without the knowledge about those internals to distinguish that from this message. For average users, I think it would be good to use a more distinguishable error message. In this case, I believe that they should be able to understand that an invalid integer value "999" was specified in the "keepalives" connection option, which caused the warning message. Then, they would need to check the setting of the "keepalives" option and correct it if necessary. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
On 2023/04/13 15:13, Etsuro Fujita wrote: I am not 100% sure that it is a good idea to use the same error message "could not send cancel request" for the PQgetCancel() and PQcancel() cases, because they are different functions. How about "could not create PGcancel structure” or something like that, for the The primary message basically should avoid reference to implementation details such as specific structure names like PGcancel, shouldn't it, as per the error message style guide? former case, so we can distinguish the former error from the latter? Although the primary message is the same, the supplemental message provides additional context that can help distinguish which function is reporting the message. Therefore, I'm fine with the current primary message in the 0001 patch. However, I'm open to any better message ideas. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
On 2023/04/13 11:00, Kyotaro Horiguchi wrote:
Agreed, it seems to be a leftover when we moved to parse_int_param()
in that area.
It looks like there was an oversight in commit e7a2217978. I've attached a
patch (0002) that updates PQconnectPoll() to use parse_int_param() for parsing
the keepalives parameter.
As this change is not directly related to the bug fix, it may not be necessary
to back-patch it to the stable versions, I think. Thought?
To clarify, are you suggesting that PQgetCancel() should
only parse the parameters for TCP connections
if cancel->raddr.addr.ss_family != AF_UNIX?
If so, I think that's a good idea.
You're right. I used connip in the diff because I thought it provided
the same condition, but in a simpler way.
I made a modification to the 0001 patch. It will now allow PQgetCancel() to
parse and interpret TCP connection parameters only when the connection is not
made through a Unix-domain socket.
However, I notcied that PQgetCancel() doesn't set errbuf.. So, I'm
fine with your proposal.
Ok.
I think it is important to inform the user when an error
occurs and a cancel request cannot be sent, as this information
can help them identify the cause of the problem (such as
setting an overly large value for the keepalives parameter).
Although I view it as an internal error, I agree with emitting some
error messages in that situation.
Ok.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom c46e1b06e68cf761bacbfe5b6d35ccf5fcbbb39d Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Wed, 12 Apr 2023 02:17:56 +0900
Subject: [PATCH v2 1/2] postgres_fdw: Fix bug causing unnecessary wait for
cancel request reply.
In the event of a local transaction being aborted while a query is
running on a remote server in postgres_fdw, the extension sends
a cancel request to the remote server. However, if PQgetCancel()
returned NULL and no cancel request was issued, postgres_fdw
could still wait for the reply to the cancel request, causing unnecessary
wait time with a 30 second timeout.
To fix this issue, this commit ensures that postgres_fdw only waits for
a reply if a cancel request was actually issued. Additionally,
this commit improves PQgetCancel() to set error messages in certain
error cases, such as when out of memory happens and malloc() fails.
Moreover, this commit enhances postgres_fdw to report a warning
message when PQgetCancel() returns NULL, explaining the reason for
the NULL value.
---
contrib/postgres_fdw/connection.c | 8 +++
src/interfaces/libpq/fe-connect.c | 81 ++-
2 files changed, 56 insertions(+), 33 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 75d93d6ead..c8bebe7b14 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -1384,6 +1384,14 @@ pgfdw_cancel_query_begin(PGconn *conn)
}
PQfreeCancel(cancel);
}
+ else
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+errmsg("could not send cancel request: %s",
+ pchomp(PQerrorMessage(conn);
+ return false;
+ }
return true;
}
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index fcd3d0d9a3..6558d098b5 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4709,11 +4709,17 @@ PQgetCancel(PGconn *conn)
return NULL;
if (conn->sock == PGINVALID_SOCKET)
+ {
+ libpq_append_conn_error(conn, "connection is not open");
return NULL;
+ }
cancel = malloc(sizeof(PGcancel));
if (cancel == NULL)
+ {
+ libpq_append_conn_error(conn, "out of memory");
return NULL;
+ }
memcpy(>raddr, >raddr, sizeof(SockAddr));
cancel->be_pid = conn->be_pid;
@@ -4724,40 +4730,49 @@ PQgetCancel(PGconn *conn)
cancel->keepalives_idle = -1;
cancel->keepalives_interval = -1;
cancel->keepalives_count = -1;
- if (conn->pgtcp_user_timeout != NULL)
- {
- if (!parse_int_param(conn->pgtcp_user_timeout,
-
>pgtcp_user_timeout,
-conn,
"tcp_user_timeout"))
- goto fail;
- }
- if (conn->keepalives != NULL)
- {
- if (!parse_int_param(conn->keepalives,
->keepalives,
-conn, "keepalives"))
- goto fail;
- }
Re: is_superuser is not documented
On 2023/04/12 5:41, Joseph Koshakow wrote: Having said all that I actually think this is the best place for is_superuser since it doesn't seem to fit in anywhere else. Yeah, I also could not find more appropriate place for is_superuser than there. I was implying that I thought it would have made more sense for is_superuser to be implemented as a function, behave as a function, and not be visible via SHOW. However, there may have been a good reason not to do this and it may already be too late for that. The is_superuser parameter is currently marked as GUC_REPORT and its value is automatically reported to a client. If we change it to a function, we will need to add functionality to automatically report the return value of the function to a client, which could be overkill. In my opinion, this is ready to be committed. Thanks! Given that we have already exceeded the feature freeze date, I'm thinking to commit this change at the next CommitFest. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
On 2023/04/12 12:00, Kyotaro Horiguchi wrote: At Wed, 12 Apr 2023 03:36:01 +0900, Fujii Masao wrote in Attached patch fixes this issue. It ensures that postgres_fdw only waits for a reply if a cancel request is actually issued. Additionally, it improves PQgetCancel() to set error messages in certain error cases, such as when out of memory occurs and malloc() fails. Moreover, it enhances postgres_fdw to report a warning message when PQgetCancel() returns NULL, explaining the reason for the NULL value. Thought? I wondered why the connection didn't fail in the first place. After digging into it, I found (or remembered) that local (or AF_UNIX) connections ignore the timeout value at making a connection. I think BTW, you can reproduce the issue even when using a TCP connection instead of a Unix domain socket by specifying a very large number in the "keepalives" connection parameter for the foreign server. Here is an example: - create server loopback foreign data wrapper postgres_fdw options (host '127.0.0.1', port '5432', keepalives '999'); - The reason behind this issue is that PQconnectPoll() parses the "keepalives" parameter value by simply using strtol(), while PQgetCancel() uses parse_int_param(). To fix this issue, it might be better to update PQconnectPoll() so that it uses parse_int_param() for parsing the "keepalives" parameter. the real issue here is that PGgetCancel is unnecessarily checking its value and failing as a result. Otherwise there would be no room for failure in the call to PQgetCancel() at that point in the example case. PQconnectPoll should remove the ignored parameters at connection or PQgetCancel should ingore the unreferenced (or unchecked) parameters. For example, the below diff takes the latter way and seems working (for at least AF_UNIX connections) To clarify, are you suggesting that PQgetCancel() should only parse the parameters for TCP connections if cancel->raddr.addr.ss_family != AF_UNIX? If so, I think that's a good idea. Of course, it's not great that pgfdw_cancel_query_begin() ignores the result from PQgetCancel(), but I think we don't need another ereport. Can you please clarify why you suggest avoiding outputting the warning message when PQgetCancel() returns NULL? I think it is important to inform the user when an error occurs and a cancel request cannot be sent, as this information can help them identify the cause of the problem (such as setting an overly large value for the keepalives parameter). Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Issue in postgres_fdw causing unnecessary wait for cancel request reply
Hi,
When using postgres_fdw, in the event of a local transaction being
aborted while a query is running on a remote server,
postgres_fdw sends a cancel request to the remote server.
However, if PQgetCancel() returned NULL and no cancel request was issued,
I found that postgres_fdw could still wait for the reply to
the cancel request, causing unnecessary wait time with a 30 second timeout.
For example, the following queries can reproduce the issue:
create extension postgres_fdw;
create server loopback foreign data wrapper postgres_fdw options
(tcp_user_timeout 'a');
create user mapping for public server loopback;
create view t as select 1 as i from pg_sleep(100);
create foreign table ft (i int) server loopback options (table_name 't');
select * from ft;
Press Ctrl-C while running the above SELECT query.
Attached patch fixes this issue. It ensures that postgres_fdw only waits
for a reply if a cancel request is actually issued. Additionally,
it improves PQgetCancel() to set error messages in certain error cases,
such as when out of memory occurs and malloc() fails. Moreover,
it enhances postgres_fdw to report a warning message when PQgetCancel()
returns NULL, explaining the reason for the NULL value.
Thought?
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom 26293ade92ce6fa0bbafac4a95f634e485f4aab6 Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Wed, 12 Apr 2023 02:17:56 +0900
Subject: [PATCH v1] postgres_fdw: Fix bug causing unnecessary wait for cancel
request reply.
In the event of a local transaction being aborted while a query is
running on a remote server in postgres_fdw, the extension sends
a cancel request to the remote server. However, if PQgetCancel()
returned NULL and no cancel request was issued, postgres_fdw
could still wait for the reply to the cancel request, causing unnecessary
wait time with a 30 second timeout.
To fix this issue, this commit ensures that postgres_fdw only waits for
a reply if a cancel request was actually issued. Additionally,
this commit improves PQgetCancel() to set error messages in certain
error cases, such as when out of memory happens and malloc() fails.
Moreover, this commit enhances postgres_fdw to report a warning
message when PQgetCancel() returns NULL, explaining the reason for
the NULL value.
---
contrib/postgres_fdw/connection.c | 8
src/interfaces/libpq/fe-connect.c | 6 ++
2 files changed, 14 insertions(+)
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 2969351e9a..e3c79158d8 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -1344,6 +1344,14 @@ pgfdw_cancel_query_begin(PGconn *conn)
}
PQfreeCancel(cancel);
}
+ else
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+errmsg("could not send cancel request: %s",
+ pchomp(PQerrorMessage(conn);
+ return false;
+ }
return true;
}
diff --git a/src/interfaces/libpq/fe-connect.c
b/src/interfaces/libpq/fe-connect.c
index 40fef0e2c8..eeb4d9a745 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -4703,11 +4703,17 @@ PQgetCancel(PGconn *conn)
return NULL;
if (conn->sock == PGINVALID_SOCKET)
+ {
+ libpq_append_conn_error(conn, "connection is not open");
return NULL;
+ }
cancel = malloc(sizeof(PGcancel));
if (cancel == NULL)
+ {
+ libpq_append_conn_error(conn, "out of memory");
return NULL;
+ }
memcpy(>raddr, >raddr, sizeof(SockAddr));
cancel->be_pid = conn->be_pid;
--
2.40.0
Re: is_superuser is not documented
On 2023/04/08 23:53, Joseph Koshakow wrote: On Mon, Apr 3, 2023 at 10:47 AM Fujii Masao mailto:masao.fu...@oss.nttdata.com>> wrote: > Yes, the patch has not been committed yet because of lack of review comments. > Do you have any review comments on this patch? > Or you think it's ready for committer? I'm not very familiar with this code, so I'm not sure how much my review is worth, but maybe it will spark some discussion. Thanks for the comments! > Yes, this patch moves the descriptions of is_superuser to config.sgml > and changes its group to PRESET_OPTIONS. is_superuser feels a little out of place in this file. All of the options here apply to the entire PostgreSQL server, while is_superuser only applies to the current session. Aren't other preset options like lc_collate, lc_ctype and server_encoding similar to is_superuser? They seem to behave in a similar way as their settings can be different for each connection depending on the connected database. I'm not familiar with the origins of is_superuser and it may be too late for this, but it seems like is_superuser would fit in much better as a system information function [0] rather than a GUC. Particularly in the Session Information Functions. I understand your point, but I think it would be more confusing to document is_superuser there because it's defined and behaves differently from session information functions like current_user. For instance, the value of is_superuser can be displayed using SHOW command, while current_user cannot. Therefore, it's better to keep is_superuser separate from the session information functions. As a side note server_version, server_encoding, lc_collate, and lc_ctype all appear in both the preset options section of config.sgml and in show.sgml. I'm not sure what the logic is for just including these three parameters in show.sgml, but I think we should either include all of the preset options or none of them. Agreed. I think that it's better to just treat them as GUCs and remove their descriptions from show.sgml. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [Proposal] Add foreign-server health checks infrastructure
On 2023/03/13 16:05, Hayato Kuroda (Fujitsu) wrote: Thank you so much for your reviewing! Now we can wait comments from senior members and committers. Thanks for working on this patch! Regarding 0001 patch, on second thought, to me, it seems odd to expose a function that doesn't have anything to directly do with PostgreSQL as a libpq function. The function simply calls poll() on the socket with POLLRDHUP if it is supported. While it is certainly convenient to have this function, I'm not sure that it fits within the scope of libpq. Thought? Regarding 0002 patch, the behavior of postgres_fdw_verify_connection_states() in regards to multiple connections using different user mappings seems not well documented. The function seems to return false if either of those connections has been closed. This behavior means that it's difficult to identify which connection has been closed when there are multiple ones to the given server. To make it easier to identify that, it could be helpful to extend the postgres_fdw_verify_connection_states() function so that it accepts a unique connection as an input instead of just the server name. One suggestion is to extend the function so that it accepts both the server name and the user name used for the connection, and checks the specified connection. If only the server name is specified, the function should check all connections to the server and return false if any of them are closed. This would be helpful since there is typically only one connection to the server in most cases. Additionally, it would be helpful to extend the postgres_fdw_get_connections() function to also return the user name used for each connection, as currently there is no straightforward way to identify it. The function name "postgres_fdw_verify_connection_states" may seem unnecessarily long to me. A simpler name like "postgres_fdw_verify_connection" may be enough? The patch may not be ready for commit due to the review comments, and with the feature freeze approaching in a few days, it may not be possible to include this feature in v16. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: is_superuser is not documented
On 2023/04/01 22:34, Joseph Koshakow wrote: The patch updated the guc table for is_superuser in src/backend/utils/misc/guc_tables.c Yes, this patch moves the descriptions of is_superuser to config.sgml and changes its group to PRESET_OPTIONS. However, when I look at the code on master I don't see this update Yes, the patch has not been committed yet because of lack of review comments. Do you have any review comments on this patch? Or you think it's ready for committer? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Is psSocketPoll doing the right thing?
On 2023/02/09 11:50, Kyotaro Horiguchi wrote: Hello. While looking a patch, I found that pqSocketPoll passes through the result from poll(2) to the caller and throws away revents. If I understand it correctly, poll() *doesn't* return -1 nor errno by the reason it has set POLLERR, POLLHUP, POLLNVAL, and POLLRDHUP for some of the target sockets, and returns 0 unless poll() itself failed to work. As far as I understand correctly, poll() returns >0 if "revents" has either of those bits, not 0 nor -1. You're thinking that pqSocketPoll() should check "revents" and return -1 if either of those bits is set? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: make_ctags: use -I option to ignore pg_node_attr macro
On 2023/02/08 20:17, Tatsuo Ishii wrote:
Attached is the v2 patch.
Thanks for the patch!
With the patch, I got the following error when executing make_etags..
$ ./src/tools/make_etags
etags: invalid option -- 'e'
Try 'etags --help' for a complete list of options.
sort: No such file or directory
Oops. Thank you for pointing it out. BTW, just out of curiosity, do
you have etags on you Mac?
Yes.
$ etags --version
etags (GNU Emacs 28.2)
Copyright (C) 2022 Free Software Foundation, Inc.
This program is distributed under the terms in ETAGS.README
This is the comment for the commit d1e2a380cb. I found that make_etags
with
an invalid option reported the following usage message mentioning
make_ctags
(not make_etags). Isn't this confusing?
$ ./src/tools/make_etags -a
Usage: /.../make_ctags [-e][-n]
That's hard to fix without some code duplication. We decided that
make_etags is not a symlink to make_ctags, rather execs make_ctags. Of
course we could let make_etags perform the same option check, but I
doubt it's worth the trouble.
How about just applying the following into make_etags?
+if [ $# -gt 1 ] || ( [ $# -eq 1 ] && [ $1 != "-n" ] )
+then echo "Usage: $0 [-n]"
+ exit 1
+fi
Anyway, attached is the v3 patch.
Thanks for updating the patch!
With the patch, make_etags caused the following error messages
on my MacOS.
$ ./src/tools/make_etags
No such file or directory
No such file or directory
To fix this error, probaby we should get rid of double-quotes
from "$FLAGS" "$IGNORE_IDENTIFIES" in the following command.
- xargs ctags $MODE -a -f $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
+ xargs $PROG $MODE $TAGS_OPT $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
+ elsectags --help 2>&1 | grep -- -e >/dev/null
+ # Note that "ctags --help" does not always work. Even certain
ctags does not have the option.
This code seems to assume that there is non-Exuberant ctags
supporting -e option. But does such ctags really exist?
I fixed the above issues and refactored the code.
Attached is the updated version of the patch. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/src/tools/make_ctags b/src/tools/make_ctags
index 102881667b..aa7d7b573f 100755
--- a/src/tools/make_ctags
+++ b/src/tools/make_ctags
@@ -9,15 +9,19 @@ then echo $usage
exit 1
fi
-MODE=
+EMACS_MODE=
NO_SYMLINK=
+IS_EXUBERANT=
+PROG="ctags"
+TAGS_OPT="-a -f"
TAGS_FILE="tags"
+FLAGS=
+IGNORE_IDENTIFIES=
while [ $# -gt 0 ]
do
if [ $1 = "-e" ]
- thenMODE="-e"
- TAGS_FILE="TAGS"
+ thenEMACS_MODE="Y"
elif [ $1 = "-n" ]
thenNO_SYMLINK="Y"
else
@@ -27,15 +31,24 @@ do
shift
done
-command -v ctags >/dev/null || \
+if [ ! "$EMACS_MODE" ]
+then (command -v ctags >/dev/null) || \
{ echo "'ctags' program not found" 1>&2; exit 1; }
+fi
-trap "ret=$?; rm -rf /tmp/$$; exit $ret" 0 1 2 3 15
-rm -f ./$TAGS_FILE
-
-IS_EXUBERANT=""
ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
+if [ "$EMACS_MODE" ]
+then TAGS_FILE="TAGS"
+ if [ "$IS_EXUBERANT" ]
+ then PROG="ctags -e"
+ else(command -v etags >/dev/null) || \
+ { echo "neither 'etags' nor exuberant 'ctags' program not
found" 1>&2; exit 1; }
+ PROG="etags"
+ TAGS_OPT="-a -o"
+ fi
+fi
+
# List of kinds supported by Exuberant Ctags 5.8
# generated by ctags --list-kinds
# --c-kinds was called --c-types before 2003
@@ -56,20 +69,23 @@ ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
if [ "$IS_EXUBERANT" ]
then FLAGS="--c-kinds=+dfmstuv"
-else FLAGS="-dt"
+elif [ ! "$EMACS_MODE" ]
+then FLAGS="-dt"
fi
# Use -I option to ignore a macro
if [ "$IS_EXUBERANT" ]
then IGNORE_IDENTIFIES="-I pg_node_attr+"
-else IGNORE_IDENTIFIES=
fi
+trap "ret=$?; rm -rf /tmp/$$; exit $ret" 0 1 2 3 15
+rm -f ./$TAGS_FILE
+
# this is outputting the tags into the file 'tags', and appending
find `pwd`/ \( -name tmp_install -prune -o -name tmp_check -prune \) \
-o \( -name "*.[chly]" -o -iname "*makefile*" -o -name "*.mk" -o -name
"*.in" \
-o -name "*.sql" -o -name "*.p[lm]" \) -type f -print |
- xargs ctags $MODE -a -f $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
+ xargs $PROG $TAGS_OPT $TAGS_FILE $FLAGS $IGNORE_IDENTIFIES
# Exuberant tags has a header that we cannot sort in with the other entries
# so we skip the sort step
diff --git a/src/tools/make_etags b/src/tools/make_etags
index afc57e3e89..7e51bb4c4e 100755
--- a/src/tools/make_etags
+++ b/src/tools/make_etags
@@ -1,5 +1,11 @@
#!/bin/sh
-# src/tools/make_etags
+
+# src/tools/make_etags [-n]
+
+if [ $# -gt 1 ] || ( [ $# -eq 1 ] && [ $1 != "-n" ] )
+then echo "Usage: $0 [-n]"
+ exit 1
+fi
cdir=`dirname $0`
dir=`(cd $cdir && pwd)`
Re: make_ctags: use -I option to ignore pg_node_attr macro
On 2023/02/08 9:49, Tatsuo Ishii wrote: I am not sure if this is good way to check if ctags supports "-e" or not. + thenctags --version 2>&1 | grep -- -e >/dev/null Perhaps, "--help" might be intended rather than "--version" to check supported options? Yeah, that was my mistake. Even so, ctags would have other option whose name contains "-e" than Emacs support, so this check could end in a wrong result. Therefore, it seems to me that it is better to check immediately if etags is available in case that we don't have Exuberant-type ctags. That makes sense. Attached is the v2 patch. Thanks for the patch! With the patch, I got the following error when executing make_etags.. $ ./src/tools/make_etags etags: invalid option -- 'e' Try 'etags --help' for a complete list of options. sort: No such file or directory + if [ $? != 0 -a -z "$ETAGS_EXISTS" ] + thenecho "'ctags' does not support emacs mode and etags does not exist" 1>&2; exit 1 + fi This code can be reached after "rm -f ./$TAGS_FILE" is executed in make_ctags. But we should check whether the required program has been already installed and exit immediately if not, before modifying anything? This is the comment for the commit d1e2a380cb. I found that make_etags with an invalid option reported the following usage message mentioning make_ctags (not make_etags). Isn't this confusing? $ ./src/tools/make_etags -a Usage: /.../make_ctags [-e][-n] Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: make_ctags: use -I option to ignore pg_node_attr macro
On 2022/10/19 13:25, Tatsuo Ishii wrote: Thanks. the v6 patch pushed to master branch. Since this commit, make_etags has started failing to generate tags files with the following error messages, on my MacOS. $ src/tools/make_etags /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ctags: illegal option -- e usage: ctags [-BFadtuwvx] [-f tagsfile] file ... sort: No such file or directory In my MacOS, non-Exuberant ctags is installed and doesn't support -e option. But the commit changed make_etags so that it always calls ctags with -e option via make_ctags. This seems the cause of the above failure. IS_EXUBERANT="" ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y" make_ctags has the above code and seems to support non-Exuberant ctags. If so, we should revert the changes of make_etags by the commit and make make_etags work with that ctags? Or, we should support only Exuberant-type ctags (btw, I'm ok with this) and get rid of something like the above code? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Refactoring postgres_fdw/connection.c
On 2023/01/29 19:31, Etsuro Fujita wrote: I agree that if the name of an existing function was bad, we should rename it, but I do not think the name pgfdw_get_cleanup_result is bad; I think it is good in the sense that it well represents what the function waits for. The patch you proposed changes pgfdw_get_cleanup_result to cover the timed_out==NULL case, but I do not think it is a good idea to rename it for such a minor reason. That function is used in all supported versions, so that would just make back-patching hard. As far as I understand, the function name pgfdw_get_cleanup_result is used because it's used to get the result during abort cleanup as the comment tells. OTOH new function is used even not during abort clean, e.g., pgfdw_get_result() calls that new function. So I don't think that pgfdw_get_cleanup_result is good name in some places. If you want to leave pgfdw_get_cleanup_result for the existing uses, we can leave that and redefine it so that it just calls the workhorse function pgfdw_get_result_timed. Yeah, this is intentional; in commit 04e706d42, I coded this to match the behavior in the non-parallel-commit mode, which does not call CHECK_FOR_INTERRUPT. But could you tell me why? My concern about doing so is that WaitLatchOrSocket is rather expensive, so it might lead to useless overhead in most cases. pgfdw_get_result() and pgfdw_get_cleanup_result() already call WaitLatchOrSocket() and CHECK_FOR_INTERRUPTS(). That is, during commit phase, they are currently called when receiving the result of COMMIT TRANSACTION command from remote server. Why do we need to worry about their overhead only when executing DEALLOCATE ALL? Anyway, this changes the behavior, so you should show the evidence that this is useful. I think this would be beyond refactoring, though. Isn't it useful to react the interrupts (e.g., shutdown requests) soon even during waiting for the result of DEALLOCATE ALL? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: ps command does not show walsender's connected db
On 2022/10/07 18:43, bt22nakamorit wrote:
2022-10-07 16:59 Fujii Masao wrote:
s/subscriber/publisher ?
I did not understand what this means.
Sorry for confusing wording... I was just trying to say that walsender
is connected to a database of the publisher instead of subscriber.
Thanks for the patch!
-
+ printf("fork child process\n");
+ printf(" am_walsender: %d\n", am_walsender);
+ printf(" am_db_walsender: %d\n", am_db_walsender);
The patch seems to include the debug code accidentally.
Except this, the patch looks good to me.
I apologize for the silly mistake.
I edited the patch and attached it to this mail.
Thanks for updating the patch! LGTM.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Make ON_ERROR_STOP stop on shell script failure
On 2022/09/30 16:54, bt22nakamorit wrote: 2022-09-21 11:45 に Fujii Masao wrote: We can execute the shell commands via psql in various ways other than \! meta-command. For example, * `command` * \g | command * \gx | command * \o | command * \w | command * \copy ... program 'command' ON_ERROR_STOP should handle not only \! but also all the above in the same way? One concern about this patch is that some applications already depend on the current behavior of ON_ERROR_STOP, i.e., psql doesn't stop even when the shell command returns non-zero exit code. If so, we might need to extend ON_ERROR_STOP so that it accepts the following setting values. * off - don't stop even when either sql or shell fails (same as the current behavior) * on or sql - stop only whensql fails (same as the current behavior) * shell - stop only when shell fails * all - stop when either sql or shell fails Thought? Regards, I agree that some applications may depend on the behavior of previous ON_ERROR_STOP. I created a patch that implements off, on, shell, and all option for ON_ERROR_STOP. I also edited the code for \g, \o, \w, and \set in addition to \! to return exit status of shell commands for ON_ERROR_STOP. There were discussions regarding the error messages for when shell command fails. I have found that \copy already handles exit status of shell commands when it executes one, so I copied the messages from there. More specifically, I referred to """bool do_copy(const char *args)""" in src/bin/psql/copy.c Any feedback would be very much appreciated. Thanks for updating the patch! The patch failed to be applied into the master cleanly. Could you rebase it? patching file src/bin/psql/common.c Hunk #1 succeeded at 94 (offset 4 lines). Hunk #2 succeeded at 104 (offset 4 lines). Hunk #3 succeeded at 133 (offset 4 lines). Hunk #4 succeeded at 1869 with fuzz 1 (offset 1162 lines). Hunk #5 FAILED at 2624. 1 out of 5 hunks FAILED -- saving rejects to file src/bin/psql/common.c.rej Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: ps command does not show walsender's connected db
On 2022/10/06 22:30, bt22nakamorit wrote:
Hi,
When walsender process is evoked for logical replication, walsender is
connected to a database of the subscriber.
Naturally, ones would want the name of the connected database to show in the
entry of ps command for walsender.
In detail, running ps aux during the logical replication shows results like the
following:
postgres=# \! ps aux | grep postgres;
...
ACTC-I\+ 14575 0.0 0.0 298620 14228 ? Ss 18:22 0:00 postgres:
walsender ACTC-I\nakamorit [local] S
However, since walsender is connected to a database of the subscriber in
logical replication,
s/subscriber/publisher ?
it should show the database name, as in the following:
postgres=# \! ps aux | grep postgres
...
ACTC-I\+ 15627 0.0 0.0 298624 13936 ? Ss 15:45 0:00 postgres:
walsender ACTC-I\nakamorit postgres
Showing the database name should not apply in streaming replication though
since walsender process is not connected to any specific database.
The attached patch adds the name of the connected database to the ps entry of
walsender in logical replication, and not in streaming replication.
Thoughts?
+1
Thanks for the patch!
-
+ printf("fork child process\n");
+ printf(" am_walsender: %d\n", am_walsender);
+ printf(" am_db_walsender: %d\n", am_db_walsender);
The patch seems to include the debug code accidentally.
Except this, the patch looks good to me.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: list of acknowledgments for PG15
On 2022/09/08 21:13, Peter Eisentraut wrote: Attached is the plain-text list of acknowledgments for the PG15 release notes, current through REL_15_BETA4. Please check for problems such as wrong sorting, duplicate names in different variants, or names in the wrong order etc. (Note that the current standard is given name followed by surname, independent of cultural origin.) I'd propose to add "Tatsuhiro Nakamori" whose patch was back-patched to v15 last week, into the list. Thought? https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=249b0409b181311bb1c375311e43eb767b5c3bdd Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATIONFrom 74a89cac092969208067143d25743b40ddbfbfb0 Mon Sep 17 00:00:00 2001 From: Fujii Masao Date: Sat, 1 Oct 2022 12:51:45 +0900 Subject: [PATCH] Update list of acknowledgments in release nodes. --- doc/src/sgml/release-15.sgml | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/src/sgml/release-15.sgml b/doc/src/sgml/release-15.sgml index 9b752e26f2..7b18bfefa2 100644 --- a/doc/src/sgml/release-15.sgml +++ b/doc/src/sgml/release-15.sgml @@ -3900,6 +3900,7 @@ Author: Etsuro Fujita Takamichi Osumi Takayuki Tsunakawa Takeshi Ideriha +Tatsuhiro Nakamori Tatsuhito Kasahara Tatsuo Ishii Tatsuro Yamada -- 2.37.1
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
On 2022/09/22 16:43, Michael Paquier wrote: Added that part before pg_backup_stop() now where it makes sense with the refactoring. I have put my hands on 0001, and finished with the attached, that includes many fixes and tweaks. Some of the variable renames felt out of place, while some comments were overly verbose for their purpose, though for the last part we did not lose any information in the last version proposed. Thanks for updating the patch! This looks better to me. + MemSet(backup_state, 0, sizeof(BackupState)); + MemSet(backup_state->name, '\0', sizeof(backup_state->name)); The latter MemSet() is not necessary because the former already resets that with zero, is it? + pfree(tablespace_map); + tablespace_map = NULL; + } + + tablespace_map = makeStringInfo(); tablespace_map doesn't need to be reset to NULL here. /* Free structures allocated in TopMemoryContext */ - pfree(label_file->data); - pfree(label_file); + pfree(backup_label->data); + pfree(backup_label); + backup_label = NULL; This source comment is a bit misleading, isn't it? Because the memory for backup_label is allocated under the memory context other than TopMemoryContext. +#include "access/xlog.h" +#include "access/xlog_internal.h" +#include "access/xlogbackup.h" +#include "utils/memutils.h" Seems "utils/memutils.h" doesn't need to be included. + XLByteToSeg(state->startpoint, stopsegno, wal_segment_size); + XLogFileName(stopxlogfile, state->starttli, stopsegno, wal_segment_size); + appendStringInfo(result, "STOP WAL LOCATION: %X/%X (file %s)\n", + LSN_FORMAT_ARGS(state->startpoint), stopxlogfile); state->stoppoint and state->stoptli should be used instead of state->startpoint and state->starttli? + pfree(tablespace_map); + tablespace_map = NULL; + pfree(backup_state); + backup_state = NULL; It's harmless to set tablespace_map and backup_state to NULL after pfree(), but it's also unnecessary at least here. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Fix snapshot name for SET TRANSACTION documentation
On 2022/09/21 14:40, Fujii Masao wrote: On 2022/09/21 12:01, Japin Li wrote: Hi hackers, In 6c2003f8a1bbc7c192a2e83ec51581c018aa162f, we change the snapshot name when exporting snapshot, however, there is one place we missed update the snapshot name in documentation. Attach a patch to fix it. Thanks for the patch! Looks good to me. Pushed. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Query Jumbling for CALL and SET utility statements
On 2022/09/19 15:29, Drouvot, Bertrand wrote:
Please find attached v6 taking care of the remarks mentioned above.
Thanks for updating the patch!
+SET pg_stat_statements.track_utility = TRUE;
+
+-- PL/pgSQL procedure and pg_stat_statements.track = all
+-- we drop and recreate the procedures to avoid any caching funnies
+SET pg_stat_statements.track_utility = FALSE;
Could you tell me why track_utility is enabled just before it's disabled?
Could you tell me what actually happens if we don't drop and
recreate the procedures? I'd like to know what "any caching funnies" are.
+SELECT pg_stat_statements_reset();
+CALL MINUS_TWO(3);
+CALL MINUS_TWO(7);
+CALL SUM_TWO(3, 8);
+CALL SUM_TWO(7, 5);
+
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
This test set for the procedures is executed with the following
four conditions, respectively. Do we really need all of these tests?
track = top, track_utility = true
track = top, track_utility = false
track = all, track_utility = true
track = all, track_utility = false
+begin;
+prepare transaction 'tx1';
+insert into test_tx values (1);
+commit prepared 'tx1';
The test set of 2PC commands is also executed with track_utility = on
and off, respectively. But why do we need to run that test when
track_utility = off?
- if (query->utilityStmt)
+ if (query->utilityStmt && !jstate)
{
if (pgss_track_utility &&
!PGSS_HANDLED_UTILITY(query->utilityStmt))
"pgss_track_utility" should be
"pgss_track_utility || FORCE_TRACK_UTILITY(parsetree)" theoretically?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
On 2022/09/20 20:43, Bharath Rupireddy wrote:
- if (strlen(backupidstr) > MAXPGPATH)
+ if (state->name_overflowed == true)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("backup label too long (max %d bytes)",
It does not strike me as a huge issue to force a truncation of such
backup label names. 1024 is large enough for basically all users,
in my opinion. Or you could just truncate in the internal logic, but
still complain at MAXPGPATH - 1 as the last byte would be for the zero
termination. In short, there is no need to complicate things with
name_overflowed.
We currently allow MAXPGPATH bytes of label name excluding null
termination. I don't want to change this behaviour. In the attached v9
patch, I'm truncating the larger names to MAXPGPATH + 1 bytes in
backup state (one extra byte for representing that the name has
overflown, and another extra byte for null termination).
This looks much complicated to me.
Instead of making allocate_backup_state() or reset_backup_state()
store the label name in BackupState before do_pg_backup_start(),
how about making do_pg_backup_start() do that after checking its length?
Seems this can simplify the code very much.
If so, ISTM that we can replace allocate_backup_state() and
reset_backup_state() with just palloc0() and MemSet(), respectively.
Also we can remove fill_backup_label_name().
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Fix snapshot name for SET TRANSACTION documentation
On 2022/09/21 12:01, Japin Li wrote: Hi hackers, In 6c2003f8a1bbc7c192a2e83ec51581c018aa162f, we change the snapshot name when exporting snapshot, however, there is one place we missed update the snapshot name in documentation. Attach a patch to fix it. Thanks for the patch! Looks good to me. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022/09/21 0:51, Alvaro Herrera wrote:
The rules starting at line 4111 make me a bit nervous, since nowhere
we're restricting them to operating only on MERGE lines. I don't think
it's a real problem since USING is not terribly common anyway. Likewise
for the ones with WHEN [NOT] MATCHED. I kinda wish we had a way to
search for stuff like "keyword MERGE appears earlier in the command",
but we don't have that.
Yeah, I was thinking the same when updating the patch.
How about adding something like PartialMatches() that checks whether
the keywords are included in the input string or not? If so, we can restrict
some tab-completion rules to operating only on MERGE, as follows. I attached
the WIP patch (0002 patch) that introduces PartialMatches().
Is this approach over-complicated? Thought?
+ else if (PartialMatches("MERGE", "INTO", MatchAny, "USING") ||
+PartialMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING") ||
+PartialMatches("MERGE", "INTO", MatchAny, MatchAny,
"USING"))
+ {
+ /* Complete MERGE INTO ... ON with target table attributes */
+ if (TailMatches("INTO", MatchAny, "USING", MatchAny, "ON"))
+ COMPLETE_WITH_ATTR(prev4_wd);
+ else if (TailMatches("INTO", MatchAny, "AS", MatchAny, "USING", MatchAny,
"AS", MatchAny, "ON"))
+ COMPLETE_WITH_ATTR(prev8_wd);
+ else if (TailMatches("INTO", MatchAny, MatchAny, "USING", MatchAny,
MatchAny, "ON"))
+ COMPLETE_WITH_ATTR(prev6_wd);
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom 671b4dff2e259b4d148dcbfaee0611fc2bddce85 Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Wed, 21 Sep 2022 11:46:59 +0900
Subject: [PATCH 1/2] psql: Improve tab-completion for MERGE.
---
src/bin/psql/tab-complete.c | 102 +++-
1 file changed, 67 insertions(+), 35 deletions(-)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index f3465adb85..820f47d23a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1669,7 +1669,7 @@ psql_completion(const char *text, int start, int end)
"COMMENT", "COMMIT", "COPY", "CREATE", "DEALLOCATE", "DECLARE",
"DELETE FROM", "DISCARD", "DO", "DROP", "END", "EXECUTE",
"EXPLAIN",
"FETCH", "GRANT", "IMPORT FOREIGN SCHEMA", "INSERT INTO",
"LISTEN", "LOAD", "LOCK",
- "MERGE", "MOVE", "NOTIFY", "PREPARE",
+ "MERGE INTO", "MOVE", "NOTIFY", "PREPARE",
"REASSIGN", "REFRESH MATERIALIZED VIEW", "REINDEX", "RELEASE",
"RESET", "REVOKE", "ROLLBACK",
"SAVEPOINT", "SECURITY LABEL", "SELECT", "SET", "SHOW", "START",
@@ -3641,7 +3641,7 @@ psql_completion(const char *text, int start, int end)
*/
else if (Matches("EXPLAIN"))
COMPLETE_WITH("SELECT", "INSERT INTO", "DELETE FROM", "UPDATE",
"DECLARE",
- "MERGE", "EXECUTE", "ANALYZE",
"VERBOSE");
+ "MERGE INTO", "EXECUTE", "ANALYZE",
"VERBOSE");
else if (HeadMatches("EXPLAIN", "(*") &&
!HeadMatches("EXPLAIN", "(*)"))
{
@@ -3660,12 +3660,12 @@ psql_completion(const char *text, int start, int end)
}
else if (Matches("EXPLAIN", "ANALYZE"))
COMPLETE_WITH("SELECT", "INSERT INTO", "DELETE FROM", "UPDATE",
"DECLARE",
- "MERGE", "EXECUTE", "VERBOSE");
+ "MERGE INTO", "EXECUTE", "VERBOSE");
else if (Matches("EXPLAIN", "(*)") ||
Matches("EXPLAIN", "VERBOSE") ||
Matches("EXPLAIN", "ANALYZE", "VERBOSE"))
COMPLETE_WITH("SE
Re: Make ON_ERROR_STOP stop on shell script failure
On 2022/09/20 15:15, bt22nakamorit wrote: I thought that this action is rather unexpected since, based on the word """ON_ERROR_STOP""", ones may expect that failures of shell scripts should halt the incoming instructions as well. One clear solution is to let failures of shell script stop incoming queries just like how errors of SQLs do currently. Thoughts? +1 I edited the documentation for ON_ERROR_STOP. Any other suggestions? Thanks for the patch! Could you add it to the next CommitFest so that we don't forget it? We can execute the shell commands via psql in various ways other than \! meta-command. For example, * `command` * \g | command * \gx | command * \o | command * \w | command * \copy ... program 'command' ON_ERROR_STOP should handle not only \! but also all the above in the same way? One concern about this patch is that some applications already depend on the current behavior of ON_ERROR_STOP, i.e., psql doesn't stop even when the shell command returns non-zero exit code. If so, we might need to extend ON_ERROR_STOP so that it accepts the following setting values. * off - don't stop even when either sql or shell fails (same as the current behavior) * on or sql - stop only whensql fails (same as the current behavior) * shell - stop only when shell fails * all - stop when either sql or shell fails Thought? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
On 2022/09/18 23:09, Bharath Rupireddy wrote:
On Sun, Sep 18, 2022 at 1:23 PM Bharath Rupireddy
wrote:
cfbot fails [1] with v6 patch. I made a silly mistake by not checking
the output of "make check-world -j 16" fully, I just saw the end
message "All tests successful." before posting the v6 patch.
The failure is due to perform_base_backup() accessing BackupState's
tablespace_map without a null check, so I fixed it.
Sorry for the noise. Please review the attached v7 patch further.
Thanks for updating the patch!
=# SELECT * FROM pg_backup_start('test', true);
=# SELECT * FROM pg_backup_stop();
LOG: server process (PID 15651) was terminated by signal 11: Segmentation
fault: 11
DETAIL: Failed process was running: SELECT * FROM pg_backup_stop();
With v7 patch, pg_backup_stop() caused the segmentation fault.
=# SELECT * FROM pg_backup_start(repeat('test', 1024));
ERROR: backup label too long (max 1024 bytes)
STATEMENT: SELECT * FROM pg_backup_start(repeat('test', 1024));
=# SELECT * FROM pg_backup_start(repeat('testtest', 1024));
LOG: server process (PID 15844) was terminated by signal 11: Segmentation
fault: 11
DETAIL: Failed process was running: SELECT * FROM
pg_backup_start(repeat('testtest', 1024));
When I specified longer backup label in the second run of pg_backup_start()
after the first run failed, it caused the segmentation fault.
+ state = (BackupState *) palloc0(sizeof(BackupState));
+ state->name = pstrdup(name);
pg_backup_start() calls allocate_backup_state() and allocates the memory for
the input backup label before checking its length in do_pg_backup_start().
This can cause the memory for backup label to be allocated too much
unnecessary. I think that the maximum length of BackupState.name should
be MAXPGPATH (i.e., maximum allowed length for backup label).
The definition of SessionBackupState enum type also should be in xlogbackup.h?
Correct. Basically, all the backup related code from xlog.c,
xlogfuncs.c and elsewhere can go to xlogbackup.c/.h. I will focus on
that refactoring patch once this gets in.
Understood.
Yeah, but they have to be carried from do_pg_backup_stop() to
pg_backup_stop() or callers and also instead of keeping tablespace_map
in BackupState and others elsewhere don't seem to be a good idea to
me. IMO, BackupState is a good place to contain all the information
that's carried across various functions.
In v7 patch, since pg_backup_stop() calls build_backup_content(),
backup_label and history_file seem not to be carried from do_pg_backup_stop()
to pg_backup_stop(). This makes me still think that it's better not to include
them in BackupState...
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: is_superuser is not documented
On 2022/09/14 14:27, bt22kawamotok wrote:
I update patch to reflect master update.
Thanks for updating the patch!
+
+Shows whether the current user is a superuser or not.
+
How about adding the note about when this parameter can change,
like we do for in_hot_standby docs? I applied this change to the patch.
Attached is the updated version of the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 700914684d..0f910d580c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -10584,6 +10584,23 @@ dynamic_library_path =
'C:\tools\postgresql;H:\my_project\lib;$libdir'
+
+ is_superuser (boolean)
+
+ is_superuser configuration
parameter
+
+
+
+
+Reports whether the current user is a superuser.
+Within a session, this can change when the current user is changed by
+SET ROLE,
+
+SET SESSION AUTHORIZATION, etc.
+
+
+
+
lc_collate (string)
diff --git a/doc/src/sgml/ref/show.sgml b/doc/src/sgml/ref/show.sgml
index b3747b119f..d8721be805 100644
--- a/doc/src/sgml/ref/show.sgml
+++ b/doc/src/sgml/ref/show.sgml
@@ -100,15 +100,6 @@ SHOW ALL
-
-
-IS_SUPERUSER
-
-
- True if the current role has superuser privileges.
-
-
-
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 550e95056c..1e57ee16b8 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -989,11 +989,10 @@ struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
{
- /* Not for general use --- used by SET SESSION AUTHORIZATION */
- {"is_superuser", PGC_INTERNAL, UNGROUPED,
+ {"is_superuser", PGC_INTERNAL, PRESET_OPTIONS,
gettext_noop("Shows whether the current user is a
superuser."),
NULL,
- GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL |
GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ GUC_REPORT | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
},
_auth_is_superuser,
false,
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022/09/16 11:46, bt22kawamotok wrote:
Thanks for updating.
+ COMPLETE_WITH("UPDATE", "DELETE", "DO NOTHING");
"UPDATE" is always followed by "SET", so why not complement it with
"UPDATE SET"?
Thanks for reviewing.
That's a good idea!
I create new patch v7.
Thanks for updating the patch!
I applied the following changes to the patch. Attached is the updated version
of the patch.
The tab-completion code for MERGE was added in the middle of that for LOCK
TABLE.
This would be an oversight of the commit that originally supported
tab-completion
for MERGE. I fixed this issue.
+ else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny) ||
+TailMatches("MERGE", "INTO", MatchAny,
MatchAnyExcept("AS")))
COMPLETE_WITH("USING");
This can cause to complete "MERGE INTO USING" with "USING" unexpectedly.
I fixed this issue by replacing MatchAnyExcept("AS") with
MatchAnyExcept("USING|AS").
I added some comments.
"MERGE" was tab-completed with just after "EXPLAIN" or "EXPLAIN ANALYZE", etc.
Since "INTO" always follows "MERGE", it's better to complete with "MERGE INTO"
there. I replaced "MERGE" with "MERGE INTO" in those tab-completions.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index f3465adb85..23f2a87991 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1669,7 +1669,7 @@ psql_completion(const char *text, int start, int end)
"COMMENT", "COMMIT", "COPY", "CREATE", "DEALLOCATE", "DECLARE",
"DELETE FROM", "DISCARD", "DO", "DROP", "END", "EXECUTE",
"EXPLAIN",
"FETCH", "GRANT", "IMPORT FOREIGN SCHEMA", "INSERT INTO",
"LISTEN", "LOAD", "LOCK",
- "MERGE", "MOVE", "NOTIFY", "PREPARE",
+ "MERGE INTO", "MOVE", "NOTIFY", "PREPARE",
"REASSIGN", "REFRESH MATERIALIZED VIEW", "REINDEX", "RELEASE",
"RESET", "REVOKE", "ROLLBACK",
"SAVEPOINT", "SECURITY LABEL", "SELECT", "SET", "SHOW", "START",
@@ -3641,7 +3641,7 @@ psql_completion(const char *text, int start, int end)
*/
else if (Matches("EXPLAIN"))
COMPLETE_WITH("SELECT", "INSERT INTO", "DELETE FROM", "UPDATE",
"DECLARE",
- "MERGE", "EXECUTE", "ANALYZE",
"VERBOSE");
+ "MERGE INTO", "EXECUTE", "ANALYZE",
"VERBOSE");
else if (HeadMatches("EXPLAIN", "(*") &&
!HeadMatches("EXPLAIN", "(*)"))
{
@@ -3660,12 +3660,12 @@ psql_completion(const char *text, int start, int end)
}
else if (Matches("EXPLAIN", "ANALYZE"))
COMPLETE_WITH("SELECT", "INSERT INTO", "DELETE FROM", "UPDATE",
"DECLARE",
- "MERGE", "EXECUTE", "VERBOSE");
+ "MERGE INTO", "EXECUTE", "VERBOSE");
else if (Matches("EXPLAIN", "(*)") ||
Matches("EXPLAIN", "VERBOSE") ||
Matches("EXPLAIN", "ANALYZE", "VERBOSE"))
COMPLETE_WITH("SELECT", "INSERT INTO", "DELETE FROM", "UPDATE",
"DECLARE",
- "MERGE", "EXECUTE");
+ "MERGE INTO", "EXECUTE");
/* FETCH && MOVE */
@@ -4065,58 +4065,90 @@ psql_completion(const char *text, int start, int end)
else if (HeadMatches("LOCK") && TailMatches("IN", "SHARE"))
COMPLETE_WITH("MODE", "ROW EXCLUSIVE MODE",
"UPDATE EXCLUSIVE MODE");
+
+
Re: [Proposal] Add foreign-server health checks infrastructure
On 2022/03/04 15:17, kuroda.hay...@fujitsu.com wrote:
Hi Hackers,
It's not happy, but I'm not sure about a good solution. I made a timer
reschedule
if connection lost had detected. But if queries in the transaction are quite
short,
catching SIGINT may be fail.
Attached uses another way: sets pending flags again if DoingCommandRead is true.
If a remote server goes down while it is in idle_in_transaction,
next query will fail because of ereport(ERROR).
How do you think?
Sounds ok to me.
Thanks for updating the patches!
These failed to be applied to the master branch cleanly. Could you update them?
+ this option relies on kernel events exposed by Linux, macOS,
s/this/This
+ GUC_check_errdetail("pgfdw_health_check_interval must be set to 0 on
this platform");
The actual parameter name "postgres_fdw.health_check_interval"
should be used for the message instead of internal variable name.
+ /* Register a timeout for checking remote servers */
+ pgfdw_health_check_timeout = RegisterTimeout(USER_TIMEOUT,
pgfdw_connection_check);
This registered signal handler does lots of things. But that's not acceptable
and they should be performed outside signal handler. No?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
On 2022/09/17 16:18, Bharath Rupireddy wrote: Good idea. It makes a lot more sense to me, because xlog.c is already a file of 9000 LOC. I've created xlogbackup.c/.h files and added the new code there. Once this patch gets in, I can offer my hand to move do_pg_backup_start() and do_pg_backup_stop() from xlog.c and if okay, pg_backup_start() and pg_backup_stop() from xlogfuncs.c to xlogbackup.c/.h. Then, we might have to create new get/set APIs for XLogCtl fields that do_pg_backup_start() and do_pg_backup_stop() access. The definition of SessionBackupState enum type also should be in xlogbackup.h? We need to carry tablespace_map contents from do_pg_backup_start() till pg_backup_stop(), backup_label and history_file too are easy to carry across. Hence it will be good to have all of them i.e. tablespace_map, backup_label and history_file in the BackupState structure. IMO, this way is good. backup_label and history_file are not carried between pg_backup_start() and _stop(), so don't need to be saved in BackupState. Their contents can be easily created from other saved fields in BackupState, if necessary. So at least for me it's better to get rid of them from BackupState and don't allocate TopMemoryContext memory for them. Something unrelated to your patch that I am noticing while scanning the area is that we have been always lazy in freeing the label file data allocated in TopMemoryContext when using the SQL functions if the backup is aborted. We are not talking about this much amount of memory each time a backup is performed, but that could be a cause for memory bloat in a server if the same session is used and backups keep failing, as the data is freed only on a successful pg_backup_stop(). Base backups through the replication protocol don't care about that as the code keeps around the same pointer for the whole duration of perform_base_backup(). Trying to tie the cleanup of the label file with the abort phase would be the cause of more complications with do_pg_backup_stop(), and xlog.c has no need to know about that now. Just a remark for later. Yeah, I think that can be solved by passing in backup_state to do_pg_abort_backup(). If okay, I can work on this too as 0002 patch in this thread or we can discuss this separately to get more attention after this refactoring patch gets in. Or, to avoid such memory bloat, how about allocating the memory for backup_state only when it's NULL? I'm attaching v5 patch with the above review comments addressed, please review it further. Thanks for updating the patch! + charstartxlogfile[MAXFNAMELEN_BACKUP]; /* backup start WAL file */ + charstopxlogfile[MAXFNAMELEN_BACKUP]; /* backup stop WAL file */ These file names seem not necessary in BackupState because they can be calculated from other fields like startpoint and starttli, etc when making backup_label and history file contents. If we remove them from BackupState, we can also remove the definition of MAXFNAMELEN_BACKUP macro from xlogbackup.h. + /* construct backup_label contents */ + build_backup_content(state, false); In basebackup case, build_backup_content() is called unnecessary twice because do_pg_stop_backup() and its caller, perform_base_backup() call that. This makes me think that it's better to get rid of the call to build_backup_content() from do_pg_backup_stop(). Instead its callers, perform_base_backup() and pg_backup_stop() should call that. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Query Jumbling for CALL and SET utility statements
On 2022/09/09 19:11, Drouvot, Bertrand wrote:
IME if your application relies on 2PC it's very likely that you will hit the
exact same problems described in your original email.
The utility commands for cursor like DECLARE CURSOR seem to have the same issue
and can cause lots of pgss entries. For example, when we use postgres_fdw and
execute "SELECT * FROM WHERE id = 10" five times in the same
transaction, the following commands are executed in the remote PostgreSQL server
and recorded as pgss entries there.
DECLARE c1 CURSOR FOR ...
DECLARE c2 CURSOR FOR ...
DECLARE c3 CURSOR FOR ...
DECLARE c4 CURSOR FOR ...
DECLARE c5 CURSOR FOR ...
FETCH 100 FROM c1
FETCH 100 FROM c2
FETCH 100 FROM c3
FETCH 100 FROM c4
FETCH 100 FROM c5
CLOSE c1
CLOSE c2
CLOSE c3
CLOSE c4
CLOSE c5
Furthermore, if the different query on foreign table is executed in the next
transaction, it may reuse the same cursor name previously used by another query.
That is, different queries can cause the same FETCH command like
"FETCH 100 FROM c1". This would be also an issue.
I'm not sure if the patch should also handle cursor cases. We can implement
that separately later if necessary.
I don't think that the patch should include the fix for cursor cases. It can be
implemented separately later if necessary.
Attached v5 to normalize 2PC commands too, so that we get things like:
+ case T_VariableSetStmt:
+ {
+ VariableSetStmt *stmt = (VariableSetStmt *)
node;
+
+ /* stmt->name is NULL for RESET ALL */
+ if (stmt->name)
+ {
+ APP_JUMB_STRING(stmt->name);
+ JumbleExpr(jstate, (Node *) stmt->args);
With the patch, "SET ... TO DEFAULT" and "RESET ..." are counted as the same
query.
Is this intentional? Which might be ok because their behavior is basically the
same.
But I'm afaid which may cause users to be confused. For example, they may fail
to
find the pgss entry for RESET command they ran and just wonder why the command
was
not recorded. To avoid such confusion, how about appending stmt->kind to the
jumble?
Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Add last_vacuum_index_scans in pg_stat_all_tables
On 2022/09/16 13:23, Ken Kato wrote: Regression is failing on all platforms; please correct that and resubmit the patch. Hi, Thank you for the review! I fixed it and resubmitting the patch. Could you tell me why the number of index scans should be tracked for each table? Instead, isn't it enough to have one global counter, to check whether the current setting of maintenance_work_mem is sufficient or not? That is, I'm thinking to have something like pg_stat_vacuum view that reports, for example, the number of vacuum runs, the total number of index scans, the maximum number of index scans by one vacuum run, the number of cancellation of vacuum because of lock conflicts, etc. If so, when these global counters are high or increasing, we can think that it may worth tuning maintenance_work_mem. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Refactoring postgres_fdw/connection.c
On 2022/09/05 15:17, Etsuro Fujita wrote:
+1 for that refactoring. Here are a few comments about the 0001 patch:
Thanks for reviewing the patch!
I'm not sure it's a good idea to change the function's name, because
that would make backpatching hard. To avoid that, how about
introducing a workhorse function for pgfdw_get_result and
pgfdw_get_cleanup_result, based on the latter function as you
proposed, and modifying the two functions so that they call the
workhorse function?
That's possible. We can revive pgfdw_get_cleanup_result() and
make it call pgfdw_get_result_timed(). Also, with the patch,
pgfdw_get_result() already works in that way. But I'm not sure
how much we should be concerned about back-patch "issue"
in this case. We usually rename the internal functions
if new names are better.
@@ -1599,13 +1572,9 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
entry = (ConnCacheEntry *) lfirst(lc);
/* Ignore errors (see notes in pgfdw_xact_callback) */
- while ((res = PQgetResult(entry->conn)) != NULL)
- {
- PQclear(res);
- /* Stop if the connection is lost (else we'll loop infinitely) */
- if (PQstatus(entry->conn) == CONNECTION_BAD)
- break;
- }
+ pgfdw_get_result_timed(entry->conn, 0, , NULL);
+ PQclear(res);
The existing code prevents interruption, but this would change to
allow it. Do we need this change?
You imply that we intentially avoided calling CHECK_FOR_INTERRUPT()
there, don't you? But could you tell me why?
I have to agree with Horiguchi-san, because as mentioned by him, 1)
there isn't enough duplicate code in the two bits to justify merging
them into a single function, and 2) the 0002 patch rather just makes
code complicated. The current implementation is easy to understand,
so I'd vote for leaving them alone for now.
(I think the introduction of pgfdw_abort_cleanup is good, because that
de-duplicated much code that existed both in pgfdw_xact_callback and
in pgfdw_subxact_callback, which would outweigh the downside of
pgfdw_abort_cleanup that it made code somewhat complicated due to the
logic to handle both the transaction and subtransaction cases within
that single function. But 0002 is not the case, I think.)
The function pgfdw_exec_pre_commit() that I newly introduced consists
of two parts; issue the transaction-end command based on
parallel_commit setting and issue DEALLOCATE ALL. The first part is
duplicated between pgfdw_xact_callback() and pgfdw_subxact_callback(),
but the second not (i.e., it's used only by pgfdw_xact_callback()).
So how about getting rid of that non duplicated part from
pgfdw_exec_pre_commit()?
It gives the same feeling with 0002.
I have to agree with Horiguchi-san on this as well; the existing
single-purpose functions are easy to understand, so I'd vote for
leaving them alone.
Ok, I will reconsider 0003 patch. BTW, parallel abort patch that
you're proposing seems to add new function pgfdw_finish_abort_cleanup()
with the similar structure as the function added by 0003 patch.
So probably it's helpful for us to consider this together :)
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022/09/14 14:08, bt22kawamotok wrote:
When I tried to apply this patch, I got the following warning, please fix it.
Other than that, I think everything is fine.
$ git apply fix_tab_completion_merge_v4.patch
fix_tab_completion_merge_v4.patch:38: trailing whitespace.
else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
fix_tab_completion_merge_v4.patch:39: indent with spaces.
TailMatches("USING", MatchAny, "AS", MatchAny, "ON",
MatchAny) ||
fix_tab_completion_merge_v4.patch:40: indent with spaces.
TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny))
fix_tab_completion_merge_v4.patch:53: trailing whitespace.
else if (TailMatches("WHEN", "MATCHED") ||
warning: 4 lines add whitespace errors.
Thanks for reviewing.
I fixed the problem and make patch v5.
Please check it.
Thanks for updating the patch!
+ else if (TailMatches("MERGE", "INTO", MatchAny, "USING") ||
+TailMatches("MERGE", "INTO", MatchAny, MatchAny,
"USING") ||
+TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
+ else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING") ||
+TailMatches("MERGE", "INTO", MatchAny, MatchAny,
"USING"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
The latter seems redundant and can be removed. The former seems to
cover all the cases where the latter covers.
Not only table but also view, foreign table, etc can be specified after
USING in MERGE command. So ISTM that Query_for_list_of_selectables
should be used at the above tab-completion, instead of Query_for_list_of_tables.
Thought?
+ else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
+TailMatches("USING", MatchAny, "ON", MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")) ||
+TailMatches("USING", MatchAny, "AS", MatchAny, "ON",
MatchAny) ||
+TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")) ||
+TailMatches("USING", MatchAny, MatchAny, "ON",
MatchAny) ||
+TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")))
"When" should be "WHEN"?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: is_superuser is not documented
On 2022/09/13 17:25, bt22kawamotok wrote:
Thanks for the patch!
+
+ is_superuser (boolean)
You need to add this entry just after that of "in_hot_standby" because
the descriptions of preset parameters should be placed in alphabetical
order in the docs.
+
+ Reports whether the user is superuser or not.
Isn't it better to add "current" before "user", e.g.,
"Reports whether the current user is a superuser"?
/* Not for general use --- used by SET SESSION AUTHORIZATION */
- {"is_superuser", PGC_INTERNAL, UNGROUPED,
+ {"is_superuser", PGC_INTERNAL, PRESET_OPTIONS,
This comment should be rewritten or removed because "Not for general
use" part is not true.
- GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL |
GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ GUC_REPORT | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE |
GUC_DISALLOW_IN_FILE
As Tom commented upthread, GUC_NO_RESET_ALL flag should be removed
because it's not necessary when PGC_INTERNAL context (i.e., in this context,
RESET ALL is prohibit by defaulted) is used.
With the patch, make check failed. You need to update
src/test/regress/expected/guc.out.
IS_SUPERUSER
True if the current role has superuser privileges.
I found that the docs of SET command has the above description about
is_superuser.
This description seems useless if we document the is_superuser GUC
itself. So isn't
it better to remove this description?
Thank you for your review.
I create new patch add_document_is_superuser_v2.
please check it.
Thanks for updating the patch!
The patch looks good to me.
- /* Not for general use --- used by SET SESSION AUTHORIZATION */
{"session_authorization", PGC_USERSET, UNGROUPED,
If we don't document session_authorization and do expect that
it's usually used by SET/SHOW SESSION AUTHORIZATION,
this comment doesn't need to be removed, I think.
Could you register this patch into the next Commit Fest
so that we don't forget it?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: is_superuser is not documented
On 2022/09/12 17:13, bt22kawamotok wrote:
On the other hand, it seems pretty silly that it's GUC_REPORT if
we want to consider it private. I've not checked the git history,
but I bet that flag was added later with no thought about context.
If we are going to document this then we should at least remove
the GUC_NO_SHOW_ALL flag and rewrite the comment. I wonder whether
the GUC_NO_RESET_ALL flag is needed either --- seems like the
PGC_INTERNAL context protects it sufficiently.
I wonder why this one is marked USERSET where the other is not.
You'd think both of them need similar special-casing about how
to handle SET.
SET SESSION AUTHORIZATION command changes the setting of session_authorization
by calling set_config_option() with PGC_SUSET/USERSET and PGC_S_SESSION in
ExecSetVariableStmt(). So SET SESSION AUTHORIZATION command may fail
unless session_authorization uses PGC_USERSET.
OTOH, SET SESSION AUTHORIZATION causes to call the assign hook for
session_authorization and it changes is_superuser by using PGC_INTERNAL and
PGC_S_DYNAMIC_DEFAULT. So is_superuser doesn't need to use PGC_USERSET.
I think that session_authorization also can use PGC_INTERNAL if we add
the special-handling in SET SESSION AUTHORIZATION command. But it seems a bit
overkill to me.
Thanks for your review.
I have created a patch in response to your suggestion.
I wasn't sure about USERSET, so I only created documentation for is_superuser.
Thanks for the patch!
+
+ is_superuser (boolean)
You need to add this entry just after that of "in_hot_standby" because
the descriptions of preset parameters should be placed in alphabetical
order in the docs.
+
+Reports whether the user is superuser or not.
Isn't it better to add "current" before "user", e.g.,
"Reports whether the current user is a superuser"?
/* Not for general use --- used by SET SESSION AUTHORIZATION */
- {"is_superuser", PGC_INTERNAL, UNGROUPED,
+ {"is_superuser", PGC_INTERNAL, PRESET_OPTIONS,
This comment should be rewritten or removed because "Not for general
use" part is not true.
- GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL |
GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ GUC_REPORT | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE |
GUC_DISALLOW_IN_FILE
As Tom commented upthread, GUC_NO_RESET_ALL flag should be removed
because it's not necessary when PGC_INTERNAL context (i.e., in this context,
RESET ALL is prohibit by defaulted) is used.
With the patch, make check failed. You need to update
src/test/regress/expected/guc.out.
IS_SUPERUSER
True if the current role has superuser privileges.
I found that the docs of SET command has the above description about
is_superuser.
This description seems useless if we document the is_superuser GUC itself. So
isn't
it better to remove this description?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Refactoring postgres_fdw/connection.c
On 2022/07/27 10:36, Kyotaro Horiguchi wrote: At Tue, 26 Jul 2022 18:33:04 +0900, Fujii Masao wrote in I'm not sure the two are similar with each other. The new function pgfdw_exec_pre_commit() looks like a merger of two isolated code paths intended to share a seven-line codelet. I feel the code gets a bit harder to understand after the change. I mildly oppose to this part. If so, we can pgfdw_exec_pre_commit() into two, one is the common function that sends or executes the command (i.e., calls do_sql_command_begin() or do_sql_command()), and another is the function only for toplevel. The latter function calls the common function and then executes DEALLOCATE ALL things. But this is not the way that other functions like pgfdw_abort_cleanup() is implemented. Those functions have both codes for toplevel and !toplevel (i.e., subxact), and run the processings depending on the argument "toplevel". So I'm thinking that pgfdw_exec_pre_commit() implemented in the same way is better. I didn't see it from that viewpoint but I don't think that unconditionally justifies other refactoring. If we merge pgfdw_finish_pre_(sub)?commit_cleanup()s this way, in turn pgfdw_subxact_callback() and pgfdw_xact_callback() are going to be almost identical except event IDs to handle. But I don't think we would want to merge them. I don't think they are so identical because (as you say) they have to handle different event IDs. So I agree we don't want to merge them. A concern on 0002 is that it is hiding the subxact-specific steps from the subxact callback. It would look reasonable if it were called from two or more places for each topleve and !toplevel, but actually it has only one caller for each. So I think that pgfdw_exec_pre_commit should not do that and should be renamed to pgfdw_commit_remote() or something. On the other hand pgfdw_finish_pre_commit() hides toplevel-specific steps from the caller so the same argument holds. So you conclusion is to rename pgfdw_exec_pre_commit() to pgfdw_commit_remote() or something? Another point that makes me concern about the patch is the new function takes an SQL statement, along with the toplevel flag. I guess the reason is that the command for subxact (RELEASE SAVEPOINT %d) requires the current transaction level. However, the values isobtainable very cheap within the cleanup functions. So I propose to get rid of the parameter "sql" from the two functions. Yes, that's possible. That is, pgfdw_exec_pre_commit() can construct the query string by executing the following codes, instead of accepting the query as an argument. But one downside of this approach is that the following codes are executed for every remote subtransaction entries. Maybe it's cheap to construct the query string as follows, but I'd like to avoid any unnecessray overhead if possible. So the patch makes the caller, pgfdw_subxact_callback(), construct the query string only once and give it to pgfdw_exec_pre_commit(). curlevel = GetCurrentTransactionNestLevel(); snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel); Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Fix annotations nextFullXid
On 2022/07/27 1:29, Zhang Mingli wrote: Thanks! Pushed. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Refactoring postgres_fdw/connection.c
On 2022/07/26 19:26, Etsuro Fujita wrote: Thanks for working on this! I'd like to review this after the end of the current CF. Thanks! Could you add this to the next CF? Yes. https://commitfest.postgresql.org/39/3782/ Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Refactoring postgres_fdw/connection.c
On 2022/07/26 16:25, Kyotaro Horiguchi wrote: Agree to that refactoring. And it looks fine to me. Thanks for reviewing the patches! I'm not sure the two are similar with each other. The new function pgfdw_exec_pre_commit() looks like a merger of two isolated code paths intended to share a seven-line codelet. I feel the code gets a bit harder to understand after the change. I mildly oppose to this part. If so, we can pgfdw_exec_pre_commit() into two, one is the common function that sends or executes the command (i.e., calls do_sql_command_begin() or do_sql_command()), and another is the function only for toplevel. The latter function calls the common function and then executes DEALLOCATE ALL things. But this is not the way that other functions like pgfdw_abort_cleanup() is implemented. Those functions have both codes for toplevel and !toplevel (i.e., subxact), and run the processings depending on the argument "toplevel". So I'm thinking that pgfdw_exec_pre_commit() implemented in the same way is better. It gives the same feeling with 0002. Considering that pending_deallocate becomes non-NIL only when toplevel, 38 lines out of 66 lines of the function are the toplevel-dedicated stuff. Same as above. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: remove more archiving overhead
On 2022/07/09 2:18, Nathan Bossart wrote: On Fri, Jul 08, 2022 at 01:02:51PM -0400, David Steele wrote: I think I wrote this before I'd had enough coffee. "fully persisted to storage" can mean many things depending on the storage (Posix, CIFS, S3, etc.) so I think this is fine. The basic_archive module is there for people who would like implementation details for Posix. Yes. But some users might want to use basic_archive without any changes. For them, how about documenting how basic_archive works in basic_archive docs? I'm just thinking that it's worth exposing the information commented on the top of basic_archive.c. Anyway, since the patches look good to me, I pushed them. Thanks a lot! If necessary, we can keep improving the docs later. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Fix annotations nextFullXid
On 2022/07/23 14:01, Zhang Mingli wrote: Hi, VariableCacheData.nextFullXid is renamed to nextXid in commit https://github.com/postgres/postgres//commit/fea10a64340e529805609126740a540c8f9daab4 <https://github.com/postgres/postgres//commit/fea10a64340e529805609126740a540c8f9daab4> Fix the annotations for less confusion. Thanks for the patch! LGTM. I will commit it. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Remove useless arguments in ReadCheckpointRecord().
On 2022/07/26 9:42, Kyotaro Horiguchi wrote:
At Sun, 24 Jul 2022 22:40:16 -0400, Tom Lane wrote in
Fujii Masao writes:
On 2022/07/22 17:31, Kyotaro Horiguchi wrote:
I believed that it is recommended to move to the style not having the
outmost parens. That style has been introduced by e3a87b4991.
I read the commit log, but I'm not sure yet if it's really recommended to
remove extra parens even from the existing calls to errmsg(). Removing extra
parens can interfere with back-patching of the changes around those errmsg(),
can't it?
Right, so I wouldn't be in a hurry to change existing calls. If you're
editing an ereport call for some other reason, it's fine to remove the
excess parens in it, because you're creating a backpatch hazard there
anyway. But otherwise, I think such changes are make-work in themselves
and risk creating more make-work for somebody else later.
So, I meant to propose to remove extra parens for errmsg()'s where the
message string is edited. Is it fine in that criteria?
Even in that case, removing parens may interfere with the back-patch. For
example, please imagine the case where wasShutdown is changed to be set to true
in the following code and this changed is back-patched to v15. If we modify
only the log message in the following errmsg() and leave the parens around
that, git cherry-pick of the change of wasShutdown to v15 would be completed
successfully. But if we remove the parens, git cherry-pick would fail.
ereport(FATAL,
(errmsg("could not locate required checkpoint record"),
errhint("If you are restoring from a backup, touch
\"%s/recovery.signal\" and add required recovery options.\n"
"If you are not restoring from a backup, try removing
the file \"%s/backup_label\".\n"
"Be careful: removing \"%s/backup_label\"
will result in a corrupt cluster if restoring from a backup.",
DataDir, DataDir, DataDir)));
wasShutdown = false;/* keep compiler quiet */
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Refactoring postgres_fdw/connection.c
Hi,
When reviewing the postgres_fdw parallel-abort patch [1], I found that
there are several duplicate codes in postgres_fdw/connection.c.
Which seems to make it harder to review the patch changing connection.c.
So I'd like to remove such duplicate codes and refactor the functions
in connection.c. I attached the following three patches.
There are two functions, pgfdw_get_result() and pgfdw_get_cleanup_result(),
to get a query result. They have almost the same code, call PQisBusy(),
WaitLatchOrSocket(), PQconsumeInput() and PQgetResult() in the loop,
but only pgfdw_get_cleanup_result() allows its callers to specify the timeout.
0001 patch transforms pgfdw_get_cleanup_result() to the common function
to get a query result and makes pgfdw_get_result() use it instead of
its own (duplicate) code. The patch also renames pgfdw_get_cleanup_result()
to pgfdw_get_result_timed().
pgfdw_xact_callback() and pgfdw_subxact_callback() have similar codes to
issue COMMIT or RELEASE SAVEPOINT commands. 0002 patch adds the common function,
pgfdw_exec_pre_commit(), for that purpose, and changes those functions
so that they use the common one.
pgfdw_finish_pre_commit_cleanup() and pgfdw_finish_pre_subcommit_cleanup()
have similar codes to wait for the results of COMMIT or RELEASE SAVEPOINT
commands.
0003 patch adds the common function, pgfdw_finish_pre_commit(), for that
purpose,
and replaces those functions with the common one.
That is, pgfdw_finish_pre_commit_cleanup() and
pgfdw_finish_pre_subcommit_cleanup()
are no longer necessary and 0003 patch removes them.
[1] https://commitfest.postgresql.org/38/3392/
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom aa115d03880968c2e5bab68415e06e17a337a45b Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Mon, 25 Jul 2022 17:25:24 +0900
Subject: [PATCH 1/3] Refactor pgfdw_get_result() and
pgfdw_get_cleanup_result().
---
contrib/postgres_fdw/connection.c | 125 +++---
1 file changed, 47 insertions(+), 78 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 939d114f02..cbee285480 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -108,8 +108,8 @@ static void pgfdw_reset_xact_state(ConnCacheEntry *entry,
bool toplevel);
static bool pgfdw_cancel_query(PGconn *conn);
static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
bool
ignore_errors);
-static bool pgfdw_get_cleanup_result(PGconn *conn, TimestampTz endtime,
-
PGresult **result, bool *timed_out);
+static bool pgfdw_get_result_timed(PGconn *conn, TimestampTz endtime,
+ PGresult
**result, bool *timed_out);
static void pgfdw_abort_cleanup(ConnCacheEntry *entry, bool toplevel);
static void pgfdw_finish_pre_commit_cleanup(List *pending_entries);
static void pgfdw_finish_pre_subcommit_cleanup(List *pending_entries,
@@ -799,53 +799,12 @@ pgfdw_exec_query(PGconn *conn, const char *query,
PgFdwConnState *state)
PGresult *
pgfdw_get_result(PGconn *conn, const char *query)
{
- PGresult *volatile last_res = NULL;
-
- /* In what follows, do not leak any PGresults on an error. */
- PG_TRY();
- {
- for (;;)
- {
- PGresult *res;
-
- while (PQisBusy(conn))
- {
- int wc;
-
- /* Sleep until there's something to do */
- wc = WaitLatchOrSocket(MyLatch,
-
WL_LATCH_SET | WL_SOCKET_READABLE |
-
WL_EXIT_ON_PM_DEATH,
-
PQsocket(conn),
- -1L,
PG_WAIT_EXTENSION);
- ResetLatch(MyLatch);
-
- CHECK_FOR_INTERRUPTS();
-
- /* Data available in socket? */
- if (wc & WL_SOCKET_READABLE)
- {
- if (!PQconsumeInput(conn))
- pgfdw_report_error(ERROR, NULL,
conn, false, query);
- }
- }
-
- res = PQgetResult(conn);
- if (res == NULL)
- break; /* query is complete */
+ PGresult *result = NULL;
- PQclear(last
Re: Remove useless arguments in ReadCheckpointRecord().
On 2022/07/22 17:31, Kyotaro Horiguchi wrote: I believed that it is recommended to move to the style not having the outmost parens. That style has been introduced by e3a87b4991. I read the commit log, but I'm not sure yet if it's really recommended to remove extra parens even from the existing calls to errmsg(). Removing extra parens can interfere with back-patching of the changes around those errmsg(), can't it? Anyway, at first I pushed the 0001 patch that removes useless arguments in ReadCheckpointRecord(). Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: postgres_fdw: Fix bug in checking of return value of PQsendQuery().
On 2022/07/21 23:41, Japin Li wrote: On Thu, 21 Jul 2022 at 22:22, Fujii Masao wrote: Hi, I found that fetch_more_data_begin() in postgres_fdw reports an error when PQsendQuery() returns the value less than 0 as follows though PQsendQuery() can return only 1 or 0. I think this is a bug. Attached is the patch that fixes this bug. This needs to be back-ported to v14 where async execution was supported in postgres_fdw. if (PQsendQuery(fsstate->conn, sql) < 0) pgfdw_report_error(ERROR, NULL, fsstate->conn, false, fsstate->query); Regards, +1. However, I think check whether the result equals 0 or 1 might be better. Maybe. I just used "if (!PQsendQuery())" style because it's used in postgres_fdw elsewhere. Anyway, the patch works correctly. Thanks for the review! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Remove useless arguments in ReadCheckpointRecord().
On 2022/07/21 14:54, Kyotaro Horiguchi wrote:
I agree to removing the two parameters. And agree to let
ReadCheckpointRecord not conscious of the location source.
Thanks for the review!
At Thu, 21 Jul 2022 11:45:23 +0900, Fujii Masao
wrote in
Agreed. Attached is the updated version of the patch.
Thanks for the review!
- (errmsg("could not locate required checkpoint record"),
+ (errmsg("could not locate a valid checkpoint record in backup_label
file"),
"in backup_label" there looks *to me* need some verb..
Sorry, I failed to understand your point. Could you clarify your point?
By the way,
this looks like a good chance to remove the (now) extra parens around
errmsg() and friends.
For example:
- (errmsg("could not locate a valid checkpoint record in backup_label
file"),
+ errmsg("could not locate checkpoint record spcified in backup_label
file"),
- (errmsg("could not locate a valid checkpoint record in control file")));
+ errmsg("could not locate checkpoint record recorded in control file")));
Because it's recommended not to put parenthesis just before errmsg(), you mean?
I'm ok to remove such parenthesis, but I'd like understand why before doing
that. I was thinking that either having or not having parenthesis in front of
errmsg() is ok, so there are many calls to errmsg() with parenthesis, in
xlogrecovery.c.
+ (errmsg("invalid checkpoint record")));
Is it useful to show the specified LSN there?
Yes, LSN info would be helpful also for debugging.
I separated the patch into two; one is to remove useless arguments in
ReadCheckpointRecord(), another is to improve log messages. I added LSN info in
log messages in the second patch.
+ (errmsg("invalid resource manager ID in checkpoint
record")));
We have a similar message "invalid resource manager ID %u at
%X/%X". Since the caller explains that it is a checkpoint record, we
can share the message here.
+1
+ (errmsg("invalid xl_info in checkpoint
record")));
(It is not an issue of this patch, though) I don't think this is
appropriate for user-facing message. Counldn't we say "unexpected
record type: %x" or something like?
The proposed log message doesn't look like an improvement. But I have no better
one. So I left the message as it is, in the patch, for now.
+ (errmsg("invalid length of checkpoint
record")));
We have "record with invalid length at %X/%X" or "invalid record
length at %X/%X: wanted %u, got %u". Counld we reuse any of them?
+1
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom 1a7f5b66fed158306ed802cb08b328c2f8d47f20 Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Wed, 20 Jul 2022 23:06:44 +0900
Subject: [PATCH 1/2] Remove useless arguments in ReadCheckpointRecord().
---
src/backend/access/transam/xlogrecovery.c | 84 ---
1 file changed, 15 insertions(+), 69 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index 5d6f1b5e46..e383c2123a 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -422,8 +422,8 @@ static XLogPageReadResult
WaitForWALToBecomeAvailable(XLogRecPtr RecPtr,
XLogRecPtr replayLSN,
bool nonblocking);
static int emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
-static XLogRecord *ReadCheckpointRecord(XLogPrefetcher *xlogprefetcher,
XLogRecPtr RecPtr,
-
int whichChkpt, bool report, TimeLineID replayTLI);
+static XLogRecord *ReadCheckpointRecord(XLogPrefetcher *xlogprefetcher,
+
XLogRecPtr RecPtr, TimeLineID replayTLI);
static bool rescanLatestTimeLine(TimeLineID replayTLI, XLogRecPtr replayLSN);
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool
notfoundOk);
@@ -605,7 +605,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool
*wasShutdown_ptr,
* When a backup_label file is present, we want to roll forward
from
* the checkpoint it identifies, rather than using pg_control.
*/
- record = ReadCheckpointRecord(xlogprefetcher, CheckPointLoc, 0,
true,
+ record = ReadCheckpointRecord(xlogpr
postgres_fdw: Fix bug in checking of return value of PQsendQuery().
Hi, I found that fetch_more_data_begin() in postgres_fdw reports an error when PQsendQuery() returns the value less than 0 as follows though PQsendQuery() can return only 1 or 0. I think this is a bug. Attached is the patch that fixes this bug. This needs to be back-ported to v14 where async execution was supported in postgres_fdw. if (PQsendQuery(fsstate->conn, sql) < 0) pgfdw_report_error(ERROR, NULL, fsstate->conn, false, fsstate->query); Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATIONFrom 538f33a17f3623cec54768a7325d64f8e97abe06 Mon Sep 17 00:00:00 2001 From: Fujii Masao Date: Thu, 21 Jul 2022 22:52:50 +0900 Subject: [PATCH] postgres_fdw: Fix bug in checking of return value of PQsendQuery(). When postgres_fdw begins an asynchronous data fetch, it submits FETCH query by using PQsendQuery(). If PQsendQuery() fails and returns 0, postgres_fdw should report an error. But, previously, postgres_fdw reported an error only when the return value is less than 0, though PQsendQuery() never return the values other than 0 and 1. Therefore postgres_fdw could not handle the failure to send FETCH query in an asynchronous data fetch. This commit fixes postgres_fdw so that it reports an error when PQsendQuery() returns 0. Back-patch to v14 where asynchronous execution was supported in postgres_fdw. Author: Fujii Masao Reviewed-by: Discussion: https://postgr.es/m/ --- contrib/postgres_fdw/postgres_fdw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index f3b93954ee..048db542d3 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -7070,7 +7070,7 @@ fetch_more_data_begin(AsyncRequest *areq) snprintf(sql, sizeof(sql), "FETCH %d FROM c%u", fsstate->fetch_size, fsstate->cursor_number); - if (PQsendQuery(fsstate->conn, sql) < 0) + if (!PQsendQuery(fsstate->conn, sql)) pgfdw_report_error(ERROR, NULL, fsstate->conn, false, fsstate->query); /* Remember that the request is in process */ -- 2.36.0
Re: Remove useless arguments in ReadCheckpointRecord().
On 2022/07/21 0:29, Bharath Rupireddy wrote:
How about we transform the following messages into something like below?
(errmsg("could not locate a valid checkpoint record"))); after
ReadCheckpointRecord() for control file cases to "could not locate
valid checkpoint record in control file"
(errmsg("could not locate required checkpoint record"), after
ReadCheckpointRecord() for backup_label case to "could not locate
valid checkpoint record in backup_label file"
The above messages give more meaningful and unique info to the users.
Agreed. Attached is the updated version of the patch.
Thanks for the review!
May be unrelated, IIRC, for the errors like ereport(PANIC,
(errmsg("could not locate a valid checkpoint record"))); we wanted to
add a hint asking users to consider running pg_resetwal to fix the
issue. The error for ReadCheckpointRecord() in backup_label file
cases, already gives a hint errhint("If you are restoring from a
backup, touch \"%s/recovery.signal\" .. Adding the hint of running
pg_resetwal (of course with a caution that it can cause inconsistency
in the data and use it as a last resort as described in the docs)
helps users and support engineers a lot to mitigate the server down
cases quickly.
+1 to add some hint messages. But I'm not sure if it's good to hint the use of
pg_resetwal because, as you're saying, it should be used as a last resort and
has some big risks like data loss, corruption, etc. That is, I'm concerned
about that some users might quickly run pg_resetwal because hint message says
that, without reading the docs nor understanding those risks.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom a5e45acb753e9d93074a25a2fc409c693c46630c Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Wed, 20 Jul 2022 23:06:44 +0900
Subject: [PATCH] Remove useless arguments in ReadCheckpointRecord().
---
src/backend/access/transam/xlogrecovery.c | 88 +--
1 file changed, 17 insertions(+), 71 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index 5d6f1b5e46..010f0cd7b2 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -422,8 +422,8 @@ static XLogPageReadResult
WaitForWALToBecomeAvailable(XLogRecPtr RecPtr,
XLogRecPtr replayLSN,
bool nonblocking);
static int emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
-static XLogRecord *ReadCheckpointRecord(XLogPrefetcher *xlogprefetcher,
XLogRecPtr RecPtr,
-
int whichChkpt, bool report, TimeLineID replayTLI);
+static XLogRecord *ReadCheckpointRecord(XLogPrefetcher *xlogprefetcher,
+
XLogRecPtr RecPtr, TimeLineID replayTLI);
static bool rescanLatestTimeLine(TimeLineID replayTLI, XLogRecPtr replayLSN);
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool
notfoundOk);
@@ -605,7 +605,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool
*wasShutdown_ptr,
* When a backup_label file is present, we want to roll forward
from
* the checkpoint it identifies, rather than using pg_control.
*/
- record = ReadCheckpointRecord(xlogprefetcher, CheckPointLoc, 0,
true,
+ record = ReadCheckpointRecord(xlogprefetcher, CheckPointLoc,
CheckPointTLI);
if (record != NULL)
{
@@ -638,7 +638,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool
*wasShutdown_ptr,
else
{
ereport(FATAL,
- (errmsg("could not locate required
checkpoint record"),
+ (errmsg("could not locate a valid
checkpoint record in backup_label file"),
errhint("If you are restoring from a
backup, touch \"%s/recovery.signal\" and add required recovery options.\n"
"If you are not
restoring from a backup, try removing the file \"%s/backup_label\".\n"
"Be careful: removing
\"%s/backup_label\" will result in a corrupt cluster if restoring from a
backup.",
@@ -744,7 +744,7 @@
Re: Improve description of XLOG_RUNNING_XACTS
On 2022/07/21 10:13, Masahiko Sawada wrote: Hi, I realized that standby_desc_running_xacts() in standbydesc.c doesn't describe subtransaction XIDs. I've attached the patch to improve the description. +1 The patch looks good to me. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Remove useless arguments in ReadCheckpointRecord().
Hi,
I'd like to propose to remove "whichChkpt" and "report" arguments in ReadCheckpointRecord().
"report" is obviously useless because it's always true, i.e., there are two callers of the function and they
always specify true as "report".
"whichChkpt" indicates where the specified checkpoint location came from,
pg_control or backup_label. This information is used to log different messages as follows.
switch (whichChkpt)
{
case 1:
ereport(LOG,
(errmsg("invalid primary checkpoint
link in control file")));
break;
default:
ereport(LOG,
(errmsg("invalid checkpoint link in
backup_label file")));
break;
}
return NULL;
...
switch (whichChkpt)
{
case 1:
ereport(LOG,
(errmsg("invalid primary checkpoint
record")));
break;
default:
ereport(LOG,
(errmsg("invalid checkpoint
record")));
break;
}
return NULL;
...
But the callers of ReadCheckpointRecord() already output different log messages depending
on where the invalid checkpoint record came from. So even if ReadCheckpointRecord()
doesn't use "whichChkpt", i.e., use the same log message in both pg_control and
backup_label cases, users can still identify where the invalid checkpoint record came
from, by reading the log message.
Also when whichChkpt = 0, "primary checkpoint" is used in the log message and
sounds confusing because, as far as I recall correctly, we removed the concept of primary
and secondary checkpoints before.
Therefore I think that it's better to remove "whichChkpt" argument in
ReadCheckpointRecord().
Patch attached. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom a33e557a18cf5c45bbcf47f1cf92e26998f1cd67 Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Wed, 20 Jul 2022 23:06:44 +0900
Subject: [PATCH] Remove useless arguments in ReadCheckpointRecord().
---
src/backend/access/transam/xlogrecovery.c | 84 ---
1 file changed, 15 insertions(+), 69 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c
b/src/backend/access/transam/xlogrecovery.c
index 5d6f1b5e46..e383c2123a 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -422,8 +422,8 @@ static XLogPageReadResult
WaitForWALToBecomeAvailable(XLogRecPtr RecPtr,
XLogRecPtr replayLSN,
bool nonblocking);
static int emode_for_corrupt_record(int emode, XLogRecPtr RecPtr);
-static XLogRecord *ReadCheckpointRecord(XLogPrefetcher *xlogprefetcher,
XLogRecPtr RecPtr,
-
int whichChkpt, bool report, TimeLineID replayTLI);
+static XLogRecord *ReadCheckpointRecord(XLogPrefetcher *xlogprefetcher,
+
XLogRecPtr RecPtr, TimeLineID replayTLI);
static bool rescanLatestTimeLine(TimeLineID replayTLI, XLogRecPtr replayLSN);
static int XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
XLogSource source, bool
notfoundOk);
@@ -605,7 +605,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool
*wasShutdown_ptr,
* When a backup_label file is present, we want to roll forward
from
* the checkpoint it identifies, rather than using pg_control.
*/
- record = ReadCheckpointRecord(xlogprefetcher, CheckPointLoc, 0,
true,
+ record = ReadCheckpointRecord(xlogprefetcher, CheckPointLoc,
CheckPointTLI);
if (record != NULL)
{
@@ -744,7 +744,7 @@ InitWalRecovery(ControlFileData *ControlFile, bool
*wasShutdown_ptr,
CheckPointTLI = ControlFile->checkPointCopy.ThisTimeLineID;
RedoStartLSN = ControlFile->checkPointCopy.redo;
RedoStartTLI = ControlFile->checkPointC
Re: Backup command and functions can cause assertion failure and segmentation fault
On 2022/07/16 11:36, Michael Paquier wrote:
I was thinking about doing that only on HEAD. One thing interesting
about this patch is that it can also be used as a point of reference
for other future things.
Ok, here are review comments:
+my $connstr =
+ $node->connstr('postgres') . " replication=database dbname=postgres";
Since the result of connstr() includes "dbname=postgres", you don't need to add
"dbname=postgres" again.
+# The psql command should fail on pg_stop_backup().
Typo: s/pg_stop_backup/pg_stop_backup
I reported two trouble cases; they are the cases where BASE_BACKUP is canceled
and terminated, respectively. But you added the test only for one of them. Is
this intentional?
Since one of them failed to be applied to v14 or before cleanly, I
also created the patch for those back branches. So I attached three
patches.
Fine by me.
I pushed these bugfix patches at first. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Add a test for "cannot truncate foreign table"
On 2022/07/15 16:52, Fujii Masao wrote: On 2022/07/08 17:03, Etsuro Fujita wrote: Rather than doing so, I'd vote for adding a test case to file_fdw, as proposed in the patch, because that would be much simpler and much less expensive. So ISTM that most agreed to push Nagata-san's patch adding the test for TRUNCATE on foreign table with file_fdw. So barring any objection, I will commit the patch. Pushed. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Add function to return backup_label and tablespace_map
On 2022/07/08 23:11, David Steele wrote: Looks like I made that more complicated than it needed to be: select * from pg_backup_stop(...) \gset \pset tuples_only on \pset format unaligned \o /backup_path/backup_label select :'labelfile'; Thanks! I had completely forgotten \gset command. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Add a test for "cannot truncate foreign table"
On 2022/07/08 17:03, Etsuro Fujita wrote: Rather than doing so, I'd vote for adding a test case to file_fdw, as proposed in the patch, because that would be much simpler and much less expensive. So ISTM that most agreed to push Nagata-san's patch adding the test for TRUNCATE on foreign table with file_fdw. So barring any objection, I will commit the patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Backup command and functions can cause assertion failure and segmentation fault
On 2022/07/14 17:00, Michael Paquier wrote:
On Fri, Jul 08, 2022 at 08:56:14AM -0400, Robert Haas wrote:
On Thu, Jul 7, 2022 at 10:58 AM Fujii Masao wrote:
But if many think that it's worth adding the test, I will give a
try. But even in that case, I think it's better to commit the
proposed patch at first to fix the bug, and then to write the patch
adding the test.
I have looked at that in details,
Thanks!
and it is possible to rely on
pg_stat_activity.wait_event to be BaseBackupThrottle, which would make
ISTM that you can also use pg_stat_progress_basebackup.phase.
sure that the checkpoint triggered at the beginning of the backup
finishes and that we are in the middle of the base backup. The
command for the test should be a psql command with two -c switches
without ON_ERROR_STOP, so as the second pg_backup_stop() starts after
BASE_BACKUP is cancelled using the same connection, for something like
that:
psql -c "BASE_BACKUP (CHECKPOINT 'fast', MAX_RATE 32);" \
-c "select pg_backup_stop()" "replication=database"
The last part of the test should do a pump_until() and capture "backup
is not in progress" from the stderr output of the command run.
This is leading me to the attached, that crashes quickly without the
fix and passes with the fix.
Thanks for the patch! But I'm still not sure if it's worth adding only this
test for the corner case while we don't have basic tests for BASE_BACKUP,
pg_backup_start and pg_backup_stop.
BTW, if we decide to add that test, are you planning to back-patch it?
It's true that we don't really have good test coverage of write-ahead
logging and recovery, but this doesn't seem like the most important
thing to be testing in that area, either, and developing stable tests
for stuff like this can be a lot of work.
Well, stability does not seem like a problem to me here.
I do kind of feel like the patch is fixing two separate bugs. The
change to SendBaseBackup() is fixing the problem that, because there's
SQL access on replication connections, we could try to start a backup
in the middle of another backup by mixing and matching the two
different methods of doing backups. The change to do_pg_abort_backup()
is fixing the fact that, after aborting a base backup, we don't reset
the session state properly so that another backup can be tried
afterwards.
I don't know if it's worth committing them separately - they are very
small fixes. But it would probably at least be good to highlight in
the commit message that there are two different issues.
Grouping both fixes in the same commit sounds fine by me. No
objections from here.
This sounds fine to me, too. On the other hand, it's also fine for me to push
the changes separately so that we can easily identify each change later. So I
separated the patch into two ones.
Since one of them failed to be applied to v14 or before cleanly, I also created
the patch for those back branches. So I attached three patches.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONFrom 1d110bf0ff3bfb508374930eb8947a2a0d5ffe5e Mon Sep 17 00:00:00 2001
From: Fujii Masao
Date: Tue, 12 Jul 2022 09:31:57 +0900
Subject: [PATCH] Prevent BASE_BACKUP in the middle of another backup in the
same session.
Multiple non-exclusive backups are able to be run conrrently in different
sessions. But, in the same session, only one non-exclusive backup can be
run at the same moment. If pg_backup_start (pg_start_backup in v14 or before)
is called in the middle of another non-exclusive backup in the same session,
an error is thrown.
However, previously, in logical replication walsender mode, even if that
walsender session had already called pg_backup_start and started
a non-exclusive backup, it could execute BASE_BACKUP command and
start another non-exclusive backup. Which caused subsequent pg_backup_stop
to throw an error because BASE_BACKUP unexpectedly reset the session state
marked by pg_backup_start.
This commit prevents BASE_BACKUP command in the middle of another
non-exclusive backup in the same session.
Back-patch to all supported branches.
Author: Fujii Masao
Reviewed-by: Kyotaro Horiguchi, Masahiko Sawada, Michael Paquier, Robert Haas
Discussion:
https://postgr.es/m/3374718f-9fbf-a950-6d66-d973e027f...@oss.nttdata.com
---
src/backend/replication/basebackup.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/backend/replication/basebackup.c
b/src/backend/replication/basebackup.c
index 95440013c0..637c0ce459 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -949,6 +949,12 @@ SendBaseBackup(BaseBackupCmd *cmd)
{
basebackup_options opt;
bbsink *sink;
+ SessionBackupState status = get_backup_status();
+
+ if (status == SESSION_BACKUP_RUNNING)
+ ereport(ERROR,
+
(errcode(
Re: Support TRUNCATE triggers on foreign tables
On 2022/07/08 17:13, Ian Lawrence Barwick wrote: If we want to add such prevention, we will need similar checks for INSERT/DELETE/UPDATE not only TRUNCATE. However, I think such fix is independent from this and it can be proposed as another patch. Ah OK, makes sense from that point of view. Thanks for the clarification! So I pushed the v2 patch that Yugo-san posted. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Support TRUNCATE triggers on foreign tables
On 2022/07/08 11:19, Yugo NAGATA wrote: You added "foreign tables" for BEFORE statement-level trigger as the above, but ISTM that you also needs to do that for AFTER statement-level trigger. No? Oops, I forgot it. I attached the updated patch. Thanks for updating the patch! LGTM. Barring any objection, I will commit the patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Add function to return backup_label and tablespace_map
Hi,
Since an exclusive backup method was dropped in v15, in v15 or later, we need to create
backup_label and tablespace_map files from the result of pg_backup_stop() when taking a base backup
using low level backup API. One issue when doing this is that; there is no simple way to create
those files from two columns "labelfile" and "spcmapfile" that pg_backup_stop()
returns if we execute it via psql. Probaby we need to store those columns in a temporary file and
run some OS commands or script to separate that file into backup_label and tablespace_map. This is
not simple way, and which would prevent users from migrating their backup scripts using psql from
an exclusive backup method to non-exclusive one, I'm afraid.
To enable us to do that more easily, how about adding the pg_backup_label()
function that returns backup_label and tablespace_map? I'm thinking to make
this function available just after pg_backup_start() finishes, also even after
pg_backup_stop() finishes. For example, this function allows us to take a
backup using the following psql script file.
--
SELECT * FROM pg_backup_start('test');
\! cp -a $PGDATA /backup
SELECT * FROM pg_backup_stop();
\pset tuples_only on
\pset format unaligned
\o /backup/data/backup_label
SELECT labelfile FROM pg_backup_label();
\o /backup/data/tablespace_map
SELECT spcmapfile FROM pg_backup_label();
--
Attached is the WIP patch to add pg_backup_label function. No tests nor docs
have been added yet, but if we can successfully reach the consensus for adding
the function, I will update the patch.
Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/src/backend/access/transam/xlogfuncs.c
b/src/backend/access/transam/xlogfuncs.c
index 02bd919ff6..946b119e41 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -41,8 +41,8 @@
/*
* Store label file and tablespace map during backups.
*/
-static StringInfo label_file;
-static StringInfo tblspc_map_file;
+static StringInfo label_file = NULL;
+static StringInfo tblspc_map_file = NULL;
/*
* pg_backup_start: set up for taking an on-line backup dump
@@ -77,10 +77,18 @@ pg_backup_start(PG_FUNCTION_ARGS)
* Label file and tablespace map file need to be long-lived, since they
* are read in pg_backup_stop.
*/
- oldcontext = MemoryContextSwitchTo(TopMemoryContext);
- label_file = makeStringInfo();
- tblspc_map_file = makeStringInfo();
- MemoryContextSwitchTo(oldcontext);
+ if (label_file == NULL)
+ {
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+ label_file = makeStringInfo();
+ tblspc_map_file = makeStringInfo();
+ MemoryContextSwitchTo(oldcontext);
+ }
+ else
+ {
+ resetStringInfo(label_file);
+ resetStringInfo(tblspc_map_file);
+ }
register_persistent_abort_backup_handler();
@@ -136,13 +144,36 @@ pg_backup_stop(PG_FUNCTION_ARGS)
values[1] = CStringGetTextDatum(label_file->data);
values[2] = CStringGetTextDatum(tblspc_map_file->data);
- /* Free structures allocated in TopMemoryContext */
- pfree(label_file->data);
- pfree(label_file);
- label_file = NULL;
- pfree(tblspc_map_file->data);
- pfree(tblspc_map_file);
- tblspc_map_file = NULL;
+ /* Returns the record as Datum */
+ PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values,
nulls)));
+}
+
+/*
+ * pg_backup_label: return backup_label and tablespace_map
+ */
+Datum
+pg_backup_label(PG_FUNCTION_ARGS)
+{
+ TupleDesc tupdesc;
+ Datum values[2];
+ boolnulls[2];
+
+ /* Initialize attributes information in the tuple descriptor */
+ if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
+
+ MemSet(values, 0, sizeof(values));
+ MemSet(nulls, 0, sizeof(nulls));
+
+ if (label_file != NULL)
+ values[0] = CStringGetTextDatum(label_file->data);
+ else
+ nulls[0] = true;
+
+ if (tblspc_map_file != NULL)
+ values[1] = CStringGetTextDatum(tblspc_map_file->data);
+ else
+ nulls[1] = true;
/* Returns the record as Datum */
PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values,
nulls)));
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 2e41f4d9e8..2f9b0b65f1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6300,6 +6300,12 @@
proallargtypes => '{bool,pg_lsn,text,text}', proargmodes => '{i,o,o,o}',
proargnames =>
Re: Add a test for "cannot truncate foreign table"
On 2022/07/08 0:33, Tom Lane wrote: Fujii Masao writes: On 2022/06/30 10:48, Yugo NAGATA wrote: When a foreign table has handler but doesn't support TRUNCATE, an error "cannot truncate foreign table xxx" occurs. So, what about adding a test this message output? We can add this test for file_fdw because it is one of the such foreign data wrappers. Thanks for the patch! It looks good to me. I changed the status of this patch to ready-for-committer, and will commit it barring any objeciton. This seems like a fairly pointless expenditure of test cycles to me. Perhaps more importantly, what will you do when somebody adds truncate support to that FDW? One idea is to create dummy FDW (like foreign_data.sql regression test does) not supporting TRUNCATE and use it for the test. BTW, file_fdw already has the similar test cases for INSERT, UPDATE and DELETE, as follows. -- updates aren't supported INSERT INTO agg_csv VALUES(1,2.0); ERROR: cannot insert into foreign table "agg_csv" UPDATE agg_csv SET a = 1; ERROR: cannot update foreign table "agg_csv" DELETE FROM agg_csv WHERE a = 100; ERROR: cannot delete from foreign table "agg_csv" Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Support TRUNCATE triggers on foreign tables
On 2022/06/30 19:38, Yugo NAGATA wrote: Hello, I propose supporting TRUNCATE triggers on foreign tables because some FDW now supports TRUNCATE. I think such triggers are useful for audit logging or for preventing undesired truncate. Patch attached. Thanks for the patch! It looks good to me except the following thing. TRUNCATE - Tables + Tables and foreign tables You added "foreign tables" for BEFORE statement-level trigger as the above, but ISTM that you also needs to do that for AFTER statement-level trigger. No? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Add a test for "cannot truncate foreign table"
On 2022/06/30 10:48, Yugo NAGATA wrote: Hello, During checking regression tests of TRUNCATE on foreign tables for other patch [1], I found that there is no test for foreign tables that don't support TRUNCATE. When a foreign table has handler but doesn't support TRUNCATE, an error "cannot truncate foreign table xxx" occurs. So, what about adding a test this message output? We can add this test for file_fdw because it is one of the such foreign data wrappers. I attached a patch. Thanks for the patch! It looks good to me. I changed the status of this patch to ready-for-committer, and will commit it barring any objeciton. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Backup command and functions can cause assertion failure and segmentation fault
On 2022/07/07 9:09, Michael Paquier wrote: On Wed, Jul 06, 2022 at 11:27:58PM +0900, Fujii Masao wrote: For the test, BASE_BACKUP needs to be canceled after it finishes do_pg_backup_start(), i.e., checkpointing, and before it calls do_pg_backup_stop(). So the timing to cancel that seems more severe than the test added in 0475a97f. I'm afraid that some tests can easily cancel the BASE_BACKUP while it's performing a checkpoint in do_pg_backup_start(). So for now I'm thinking to avoid such an unstable test. Hmm. In order to make sure that the checkpoint of the base backup is completed, and assuming that the checkpoint is fast while the base backup has a max rate, you could rely on a query that does a poll_query_until() on pg_control_checkpoint(), no? As long as you use IPC::Run::start, pg_basebackup would be async so the polling query and the cancellation can be done in parallel of it. 0475a97 did almost that, except that it waits for the WAL sender to be started. There seems to be some corner cases where we cannot rely on that. If "spread" checkpoint is already running when BASE_BACKUP is executed, poll_query_until() may report the end of that "spread" checkpoint before BASE_BACKUP internally starts its checkpoint. Which may cause the test to fail. If BASE_BACKUP is accidentally canceled after poll_query_until() reports the end of checkpoint but before do_pg_backup_start() finishes (i.e., before entering the error cleanup block using do_pg_abort_backup callback), the test may fail. Probably we may be able to decrease the risk of those test failures by using some techniques, e.g., adding the fixed wait time before requesting the cancel. But I'm not sure if it's worth adding the test for the corner case issue that I reported at the risk of adding the unstable test. The issue could happen only when both BASE_BACKUP and low level API for backup are eecuted via logical replication walsender mode, and BASE_BACKUP is canceled or terminated. But if many think that it's worth adding the test, I will give a try. But even in that case, I think it's better to commit the proposed patch at first to fix the bug, and then to write the patch adding the test. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: remove more archiving overhead
On 2022/04/08 7:23, Nathan Bossart wrote: On Thu, Feb 24, 2022 at 09:55:53AM -0800, Nathan Bossart wrote: Yes. I found that a crash at an unfortunate moment can produce multiple links to the same file in pg_wal, which seemed bad independent of archival. By fixing that (i.e., switching from durable_rename_excl() to durable_rename()), we not only avoid this problem, but we also avoid trying to archive a file the server is concurrently writing. Then, after a crash, the WAL file to archive should either not exist (which is handled by the archiver) or contain the same contents as any preexisting archives. I moved the fix for this to a new thread [0] since I think it should be back-patched. I've attached a new patch that only contains the part related to reducing archiving overhead. Thanks for updating the patch. It looks good to me. Barring any objection, I'm thinking to commit it. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
On 2022/07/07 16:26, Kyotaro Horiguchi wrote: + * ControlFileLock is not required as we are the only +* updator of these variables. Isn't it better to add "at this time" or something at the end of the comment because only we're not always updator of them? No? Excluding initialization, (I believe) checkpointer is really the only updator of the variables/members. But I'm fine with the addition. Ok, so I modified the patch slightly and pushed it. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
On 2022/03/16 10:29, Kyotaro Horiguchi wrote: At Wed, 16 Mar 2022 09:19:13 +0900 (JST), Kyotaro Horiguchi wrote in In short, I split out the two topics other than checkpoint log to other threads. So, this is about the main topic of this thread, adding LSNs to checkpint log. Other topics have moved to other treads [1], [2] , [3]. I think this is no longer controversial alone. So this patch is now really Read-for-Commiter and is waiting to be picked up. +1 +* ControlFileLock is not required as we are the only +* updator of these variables. Isn't it better to add "at this time" or something at the end of the comment because only we're not always updator of them? No? Barring any objection, I'm thinking to apply the above small change and commit the patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Backup command and functions can cause assertion failure and segmentation fault
On 2022/07/01 15:41, Michael Paquier wrote: On Fri, Jul 01, 2022 at 03:32:50PM +0900, Fujii Masao wrote: Sounds good idea to me. I updated the patch in that way. Attached. Skimming quickly through the thread, this failure requires a termination of a backend running BASE_BACKUP. This is basically something done by the TAP test added in 0475a97f with a WAL sender killed, and MAX_RATE being used to make sure that we have enough time to kill the WAL sender even on fast machines. So you could add a regression test, no? For the test, BASE_BACKUP needs to be canceled after it finishes do_pg_backup_start(), i.e., checkpointing, and before it calls do_pg_backup_stop(). So the timing to cancel that seems more severe than the test added in 0475a97f. I'm afraid that some tests can easily cancel the BASE_BACKUP while it's performing a checkpoint in do_pg_backup_start(). So for now I'm thinking to avoid such an unstable test. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Backup command and functions can cause assertion failure and segmentation fault
On 2022/07/01 15:09, Masahiko Sawada wrote:
The change looks good to me. I've also confirmed the change fixed the issues.
Thanks for the review and test!
@@ -233,6 +233,12 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
StringInfo labelfile;
StringInfo tblspc_map_file;
backup_manifest_info manifest;
+ SessionBackupState status = get_backup_status();
+
+ if (status == SESSION_BACKUP_RUNNING)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("a backup is already in progress in this session")));
I think we can move it to the beginning of SendBaseBackup() so we can
avoid bbsink initialization and cleanup in the error case.
Sounds good idea to me. I updated the patch in that way. Attached.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 8764084e21..c4d956b9c1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8783,6 +8783,8 @@ do_pg_abort_backup(int code, Datum arg)
{
XLogCtl->Insert.forcePageWrites = false;
}
+
+ sessionBackupState = SESSION_BACKUP_NONE;
WALInsertLockRelease();
if (emit_warning)
diff --git a/src/backend/replication/basebackup.c
b/src/backend/replication/basebackup.c
index 95440013c0..637c0ce459 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -949,6 +949,12 @@ SendBaseBackup(BaseBackupCmd *cmd)
{
basebackup_options opt;
bbsink *sink;
+ SessionBackupState status = get_backup_status();
+
+ if (status == SESSION_BACKUP_RUNNING)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("a backup is already in progress in
this session")));
parse_basebackup_options(cmd->options, );
Re: Add index item for MERGE.
On 2022/06/30 18:57, Alvaro Herrera wrote: On 2022-Jun-30, Fujii Masao wrote: Hi, I found that there is no index item for MERGE command, in the docs. Attached is the patch that adds the indexterm for MERGE to merge.sgml. +1 LGTM, thanks. Thanks for the review! Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Backup command and functions can cause assertion failure and segmentation fault
On 2022/07/01 12:05, Kyotaro Horiguchi wrote:
At Fri, 01 Jul 2022 11:56:14 +0900 (JST), Kyotaro Horiguchi
wrote in
At Fri, 01 Jul 2022 11:46:53 +0900 (JST), Kyotaro Horiguchi
wrote in
Please find the attached.
Mmm. It forgot the duplicate-call prevention and query-cancel
handling... The first one is the same as you posted but the second one
is still a problem..
So this is the first cut of that.
Thanks for reviewing the patch!
+ PG_FINALLY();
+ {
endptr = do_pg_backup_stop(labelfile->data, !opt->nowait,
);
}
- PG_END_ENSURE_ERROR_CLEANUP(do_pg_abort_backup, BoolGetDatum(false));
-
+ PG_END_TRY();
This change makes perform_base_backup() call do_pg_backup_stop() even when an
error is reported while taking a backup, i.e., between PG_TRY() and
PG_FINALLY(). Why do_pg_backup_stop() needs to be called in such an error case?
It not only cleans up the backup state but also writes the backup-end WAL
record, waits for WAL archiving. In an error case, I think that only the
cleanup of the backup state is necessary. So it seems ok to use
do_pg_abort_backup() in that case, as it is for now.
So I'm still thinking that the patch I posted is simpler and enough.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Add index item for MERGE.
Hi, I found that there is no index item for MERGE command, in the docs. Attached is the patch that adds the indexterm for MERGE to merge.sgml. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATIONdiff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml index cbb5b38a3e..7f73f3d89c 100644 --- a/doc/src/sgml/ref/merge.sgml +++ b/doc/src/sgml/ref/merge.sgml @@ -4,6 +4,9 @@ PostgreSQL documentation --> + + MERGE + MERGE
Backup command and functions can cause assertion failure and segmentation fault
he session.
To address this issue, I think that we should disallow BASE_BACKUP
to run while a backup is already in progress in the *same* session
as we already do this for pg_backup_start(). Thought? I included
the code to disallow that in the attached patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATIONdiff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 8764084e21..c4d956b9c1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8783,6 +8783,8 @@ do_pg_abort_backup(int code, Datum arg)
{
XLogCtl->Insert.forcePageWrites = false;
}
+
+ sessionBackupState = SESSION_BACKUP_NONE;
WALInsertLockRelease();
if (emit_warning)
diff --git a/src/backend/replication/basebackup.c
b/src/backend/replication/basebackup.c
index 5244823ff8..22255b6ce6 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -233,6 +233,12 @@ perform_base_backup(basebackup_options *opt, bbsink *sink)
StringInfo labelfile;
StringInfo tblspc_map_file;
backup_manifest_info manifest;
+ SessionBackupState status = get_backup_status();
+
+ if (status == SESSION_BACKUP_RUNNING)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+errmsg("a backup is already in progress in
this session")));
/* Initial backup state, insofar as we know it now. */
state.tablespaces = NIL;
Re: [Proposal] Add foreign-server health checks infrastructure
On 2022/02/22 15:41, kuroda.hay...@fujitsu.com wrote: Cfbot is still angry because of missing PGDLLIMPORT, so attached. Thanks for updating the patches! The connection check timer is re-scheduled repeatedly even while the backend is in idle state or is running a local transaction that doesn't access to any foreign servers. I'm not sure if it's really worth checking the connections even in those states. Even without the periodic connection checks, if the connections are closed in those states, subsequent GetConnection() will detect that closed connection and re-establish the connection when starting remote transaction. Thought? When a closed connection is detected in idle-in-transaction state and SIGINT is raised, nothing happens because there is no query running to be canceled by SIGINT. Also in this case the connection check timer gets disabled. So we can still execute queries that don't access to foreign servers, in the same transaction, and then the transaction commit fails. Is this expected behavior? When I shutdowned the foreign server while the local backend is in idle-in-transaction state, the connection check timer was triggered and detected the closed connection. Then when I executed COMMIT command, I got the following WARNING message. Is this a bug? WARNING: leaked hash_seq_search scan for hash table 0x7fd2ca878f20 Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [Proposal] Add foreign-server health checks infrastructure
On 2022/02/22 11:53, kuroda.hay...@fujitsu.com wrote: How do you think? Thanks for updating the patches! I will read them. cfbot is reporting that the 0002 patch fails to be applied cleanly. Could you update the patch? http://cfbot.cputube.org/patch_37_3388.log Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: postgres_fdw: using TABLESAMPLE to collect remote sample
On 2022/02/18 22:28, Tomas Vondra wrote: Hi, here's a slightly updated version of the patch series. Thanks for updating the patches! The 0001 part adds tracking of server_version_num, so that it's possible to enable other features depending on it. Like configure_remote_session() does, can't we use PQserverVersion() instead of implementing new function GetServerVersion()? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Add DBState to pg_control_system function
On 2022/02/01 13:22, Bharath Rupireddy wrote: Hi, I think emitting DBState (staring up, shut down, shut down in recovery, shutting down, in crash recovery, in archive recovery, in production) via the pg_control_system function would help know the database state, especially during PITR/archive recovery. During archive recovery, the server can open up for read-only connections even before the archive recovery finishes. Having, pg_control_system emit database state would help the users/service layers know it and so they can take some actions based on it. Attaching a patch herewith. Thoughts? This information seems not so helpful because only "in production" and "archive recovery" can be reported during normal running and recovery, respectively. No? In the state other than them, we cannot connect to the server and execute pg_control_system(). Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: RFC: Logging plan of the running query
On 2022/02/09 0:12, torikoshia wrote: BTW, since the above example results in calling ExecutorRun() only once, the output didn't differ even after ActiveQueryDesc is reset to save_ActiveQueryDesc. The below definition of test() worked as expected. create or replace function test () returns int as $$ begin perform 1; perform 1/0; exception when others then return 30; end; $$ language plpgsql; So in this case ActiveQueryDesc set by ExecutorRun() called only once is still valid even when AbortSubTransaction() is called. That is, that ActiveQueryDesc should NOT be reset to save_ActiveQueryDesc in this case, should it? OTOH, in your example, ActiveQueryDesc set by the second call to ExecutorRun() should be reset in AbortSubTransaction(). Then ActiveQueryDesc set by the first call should be valid. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
On 2022/02/14 14:40, Kyotaro Horiguchi wrote: For backbranches, the attached for pg14 does part of the full patch. Thanks for updating the patch! Of the following, I think we should do (a) and (b) to make future backpatchings easier. a) Use RedoRecPtr and PriorRedoPtr after they are assigned. b) Move assignment to PriorRedoPtr into the ControlFileLock section. I failed to understand how (a) and (b) can make the backpatching easier. How easy to backpatch seems the same whether we apply (a) and (b) or not... c) Skip udpate of minRecoveryPoint only when the checkpoint gets old. Yes. d) Skip call to UpdateCheckPointDistanceEstimate() when RedoRecPtr <= PriorRedoPtr. But "RedoRecPtr <= PriorRedoPtr" will never happen, will it? Because a restartpoint is skipped at the beginning of CreateRestartPoint() in that case. If this understanding is right, the check of "RedoRecPtr <= PriorRedoPtr" is not necessary before calling UpdateCheckPointDistanceEstimate(). + ControlFile->minRecoveryPoint = InvalidXLogRecPtr; + ControlFile->minRecoveryPointTLI = 0; Don't we need to update LocalMinRecoveryPoint and LocalMinRecoveryPointTLI after this? Maybe it's not necessary, but ISTM that it's safer and better to always update them whether the state is DB_IN_ARCHIVE_RECOVERY or not. if (flags & CHECKPOINT_IS_SHUTDOWN) ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY; Same as above. IMO it's safer and better to always update the state (whether the state is DB_IN_ARCHIVE_RECOVERY or not) if CHECKPOINT_IS_SHUTDOWN flag is passed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit
On 2022/02/21 14:45, Etsuro Fujita wrote: On Fri, Feb 18, 2022 at 1:46 AM Fujii Masao wrote: I reviewed 0001 patch. It looks good to me except the following minor things. If these are addressed, I think that the 001 patch can be marked as ready for committer. OK +* Also determine to commit (sub)transactions opened on the remote server +* in parallel at (sub)transaction end. Like the comment "Determine whether to keep the connection ...", "determine to commit" should be "determine whether to commit"? Agreed. I’ll change it as such. Thanks! If that's updated, IMO it's ok to commit the 0001 patch. After the commit, I will review 0002 and 0003 patches. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: [Proposal] Add foreign-server health checks infrastructure
On 2022/02/17 19:35, kuroda.hay...@fujitsu.com wrote: Dear Horiguchi-san, I think we just don't need to add the special timeout kind to the core. postgres_fdw can use USER_TIMEOUT and it would be suffiction to keep running health checking regardless of transaction state then fire query cancel if disconnection happens. As I said in the previous main, possible extra query cancel woud be safe. Sounds reasonable to me. I finally figured out that you mentioned about user-defined timeout system. Firstly - before posting to hackers - I designed like that, but I was afraid of an overhead that many FDW registers timeout and call setitimer() many times. Is it too overcautious? Isn't it a very special case where many FDWs use their own user timeouts? Could you tell me the assumption that you're thinking, especially how many FDWs are working? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Fix CheckIndexCompatible comment
On 2022/02/07 19:14, Yugo NAGATA wrote: Agreed. I updated the patch to add a comment about 'oldId'. Thanks for updating the patch! I slightly modified the patch and pushed it. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
