Re: pg_upgrade fails with non-standard ACL
Tested this patch by running several upgrades from different major versions and different setups. ACL, that are impossible to apply, are detected and reported, so it looks good for me.
history file on replica and double switchover
Hello! I`ve noticed, that when running switchover replica to master and back to replica, new history file is streamed to replica, but not archived, which is not great, because it breaks PITR if archiving is running on replica. The fix looks trivial. Bash script to reproduce the problem and patch are attached. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index 7c11e1ab44..63a3d0c1e6 100644 --- a/src/backend/replication/walreceiver.c +++ b/src/backend/replication/walreceiver.c @@ -761,6 +761,10 @@ WalRcvFetchTimeLineHistoryFiles(TimeLineID first, TimeLineID last) */ writeTimeLineHistoryFile(tli, content, len); + /* Mark history file as ready for archiving */ + if (XLogArchiveMode != ARCHIVE_MODE_OFF) + XLogArchiveNotify(fname); + pfree(fname); pfree(content); } double_switchover.sh Description: application/shellscript
Re: Reopen logfile on SIGHUP
On 04/11/2018 12:00 AM, Tom Lane wrote: Alexander Kuzmenkov writes: Syslogger does already rotate logs properly on SIGHUP under some conditions, so we can just change this to unconditional rotation. Probably some people wouldn't want their logs to be rotated on SIGHUP, so we could also add a GUC to control this. Please see the attached patch. I don't believe this meets the "not break other use-cases" requirement. Point 1: I do not like a solution that presumes that some background daemon is going to SIGHUP the postmaster whenever it feels like it. That will break scenarios in which the DBA is in the midst of a set of related configuration changes (either ALTER SYSTEM commands or manual postgresql.conf edits) and doesn't want those changes applied till she's done. So we need a mechanism that's narrowly targeted to reopening the logfile, without SIGHUP'ing the entire database. If logging collector can reopen file on SIGUSR1, then maybe there should be logging_collector.pid file in PGDATA, so external rotation tools can get it without much trouble? Point 2: Depending on how you've got the log filenames configured, setting rotation_requested may result in a change in log filename, which will be the wrong thing in some use-cases, particularly that of an external logrotate daemon that only wishes you'd close and reopen your file descriptor. This is a pre-existing issue with the SIGUSR1 code path, which I think hasn't come up only because hardly anybody is using that. If we're going to make it mainstream, we need to think harder about how that ought to work. External tools usually rely on logfile name staying the same. PGDG distribution do it that way for sure. Anastasia's original patch avoided the point-2 pitfall, but didn't do anything about point 1. BTW, another thing that needs to be considered is the interaction with rotation_disabled. Right now we automatically drop that on SIGHUP, but I'm unclear on whether it should be different for logrotate requests. regards, tom lane -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [PROPOSAL] Drop orphan temp tables in single-mode
On 03/07/2019 06:49 PM, Robert Haas wrote: On Thu, Mar 7, 2019 at 10:24 AM Tom Lane wrote: So if we think we can invent a "MAGICALLY FIX MY DATABASE" command, let's do that. But please let's not turn a well defined command like VACUUM into something that you don't quite know what it will do. I am on the fence about that. I see your point, but on the other hand, autovacuum drops temp tables all the time in multi-user mode and I think it's pretty clear that, with the possible exception of you, users find that an improvement. So it could be argued that we're merely proposing to make the single-user mode behavior of vacuum consistent with the behavior people are already expecting it to do. The underlying and slightly more general problem here is that users find it really hard to know what to do when vacuum fails to advance relfrozenxid. Of course, temp tables are only one reason why that can happen: logical decoding slots and prepared transactions are others, and I don't think we can automatically drop that stuff because somebody may still be expecting them to accomplish whatever their intended purpose is. The difference with temp tables is that users imagine -- quite naturally I think -- that they are in fact temporary, and that they will in fact go away when the session ends. The user would tend to view their continued existence as an unwanted implementation artifact, not something that they should be responsible for removing. I`m no hacker, but I would like to express my humble opinion on the matter. 1. Proposed patch is fairly conservative, to be on fully consistent with autovacuum behaivour VACUUM should be able to drop orphaned temp table even in mult-user mode. 2. There is indeed a problem of expected behavior from user perspective. Every PostgreSQL user knows that if you hit wraparound, you go single-mode, run VACUUM and the problem goes away. Exactly because of this I`ve got involved with this problem: https://www.postgresql.org/message-id/0c7c2f84-74f5-2cd9-767e-9b2566065d71%40postgrespro.ru Poor guy repeatedly run VACUUM after VACUUM and had no clue what to do. He even considered to just restore from backup and be done with it. It took some time to figure out a true culprit, and time = money. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: autovacuum: change priority of the vacuumed tables
On 02/15/2018 09:28 AM, Masahiko Sawada wrote: Hi, On Thu, Feb 8, 2018 at 11:01 PM, Ildus Kurbangaliev wrote: Hi, Attached patch adds 'autovacuum_table_priority' to the current list of automatic vacuuming settings. It's used in sorting of vacuumed tables in autovacuum worker before actual vacuum. The idea is to give possibility to the users to prioritize their tables in autovacuum process. Hmm, I couldn't understand the benefit of this patch. Would you elaborate it a little more? Multiple autovacuum worker can work on one database. So even if a table that you want to vacuum first is the back of the list and there other worker would pick up it. If the vacuuming the table gets delayed due to some big tables are in front of that table I think you can deal with it by increasing the number of autovacuum workers. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center Database can contain thousands of tables and often updates/deletes concentrate mostly in only a handful of tables. Going through thousands of less bloated tables can take ages. Currently autovacuum know nothing about prioritizing it`s work with respect to user`s understanding of his data and application. Also It`s would be great to sort tables according to dead/live tuple ratio and relfrozenxid. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
[proposal] recovery_target "latest"
Hello, hackers!
I`d like to propose a new argument for recovery_target parameter, which
will stand to recovering until all available WAL segments are applied.
Current PostgreSQL recovery default behavior(when no recovery target is
provided) does exactly that, but there are several shortcomings:
- without explicit recovery target standing for default behavior,
recovery_target_action is not coming to action at the end of recovery
- with PG12 changes, the life of all backup tools became very hard,
because now recovery parameters can be set outside of single config
file(recovery.conf), so it is impossible to ensure, that default
recovery behavior, desired in some cases, will not be silently
overwritten by some recovery parameter forgotten by user.
Proposed path is very simple and solves the aforementioned problems by
introducing new argument "latest" for recovery_target parameter.
Old recovery behavior is still available if no recovery target is
provided. I`m not sure, whether it should it be left as it is now, or not.
Another open question is what to do with recovery_target_inclusive if
recovery_target = "latest" is used.
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0191ec84b1..49675c38da 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3366,21 +3366,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
- recovery_target = 'immediate'
+ recovery_target (string)
recovery_target configuration parameter
-This parameter specifies that recovery should end as soon as a
-consistent state is reached, i.e. as early as possible. When restoring
-from an online backup, this means the point where taking the backup
-ended.
-
-
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
+This parameter determines how far recovery should proceed. The value
+immediate means that recovery should end as soon as a consistent
+state is reached, i.e. as early as possible. When restoring from an online
+backup, this means the point where taking the backup ended.
+The second possible value latest means that recovery
+should proceed to the end of the available WAL log.
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 2e3cc51006..faab0b2b4c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6345,6 +6345,9 @@ StartupXLOG(void)
else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
ereport(LOG,
(errmsg("starting point-in-time recovery to earliest consistent point")));
+ else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+ ereport(LOG,
+ (errmsg("starting point-in-time recovery until all available WAL is applied")));
else
ereport(LOG,
(errmsg("starting archive recovery")));
@@ -7257,6 +7260,13 @@ StartupXLOG(void)
* end of main redo apply loop
*/
+ /*
+ * If all avaialble WAL is replayed,
+ * then RECOVERY_TARGET_LATEST is satisfied
+ */
+ if (recoveryTarget == RECOVERY_TARGET_LATEST)
+reachedStopPoint = true;
+
if (reachedStopPoint)
{
if (!reachedConsistency)
@@ -7459,6 +7469,8 @@ StartupXLOG(void)
recoveryStopName);
else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
snprintf(reason, sizeof(reason), "reached consistency");
+ else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+ snprintf(reason, sizeof(reason), "applied all available WAL");
else
snprintf(reason, sizeof(reason), "no recovery target specified");
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 31a5ef0474..b652304683 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3503,7 +3503,10 @@ static struct config_string ConfigureNamesString[] =
{
{"recovery_target", PGC_POSTMASTER, WAL_RECOVERY_TARGET,
- gettext_noop("Set to \"immediate\" to end recovery as soon as a consistent state is reached."),
+ gettext_noop("Set to \"immediate\" to end recovery as "
+ "soon as a consistent state is reached or "
+ " to \"latest\" to end recovery after "
+ "replaying all available WAL in the archive."),
NULL
},
&recovery_target_string,
@@ -11527,9 +11530,10 @@ error_multiple_recovery_targets(void)
static bool
check_recovery_target(char **newval, void **extra, GucSource source)
{
- if (strcmp(*newval, "immediate") != 0 && strcmp(*newva
Re: [proposal] recovery_target "latest"
Thank you for you interest in this topic! On 11/5/19 10:41 AM, Kyotaro Horiguchi wrote: Hello. At Mon, 4 Nov 2019 16:03:38 +0300, Grigory Smolkin wrote in Hello, hackers! I`d like to propose a new argument for recovery_target parameter, which will stand to recovering until all available WAL segments are applied. Current PostgreSQL recovery default behavior(when no recovery target is provided) does exactly that, but there are several shortcomings: - without explicit recovery target standing for default behavior, recovery_target_action is not coming to action at the end of recovery - with PG12 changes, the life of all backup tools became very hard, because now recovery parameters can be set outside of single config file(recovery.conf), so it is impossible to ensure, that default recovery behavior, desired in some cases, will not be silently overwritten by some recovery parameter forgotten by user. Proposed path is very simple and solves the aforementioned problems by introducing new argument "latest" for recovery_target parameter. Does the tool remove or rename recovery.conf to cancel the settings? And do you intend that the new option is used to override settings by appending it at the end of postgresql.conf? If so, the commit f2cbffc7a6 seems to break the assumption. PG12 rejects to start if it finds two different kinds of recovery target settings. Yes, previously it was possible to remove/rename old recovery.conf, but not anymore. My assumption is exactly that PG should reject to start because of multiple recovery targets. Failing to start is infinitely better that recovering to the wrong recovery target. Old recovery behavior is still available if no recovery target is provided. I`m not sure, whether it should it be left as it is now, or not. Another open question is what to do with recovery_target_inclusive if recovery_target = "latest" is used. Anyway inclusiveness doesn't affect to "immediate". If we had the "latest" option, it would behave the same way. Right, thank you. regards. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [proposal] recovery_target "latest"
Attached new version of a patch with TAP test.
On 11/5/19 11:51 AM, Grigory Smolkin wrote:
Thank you for you interest in this topic!
On 11/5/19 10:41 AM, Kyotaro Horiguchi wrote:
Hello.
At Mon, 4 Nov 2019 16:03:38 +0300, Grigory Smolkin
wrote in
Hello, hackers!
I`d like to propose a new argument for recovery_target parameter,
which will stand to recovering until all available WAL segments are
applied.
Current PostgreSQL recovery default behavior(when no recovery target
is provided) does exactly that, but there are several shortcomings:
- without explicit recovery target standing for default behavior,
recovery_target_action is not coming to action at the end of recovery
- with PG12 changes, the life of all backup tools became very hard,
because now recovery parameters can be set outside of single config
file(recovery.conf), so it is impossible to ensure, that default
recovery behavior, desired in some cases, will not be silently
overwritten by some recovery parameter forgotten by user.
Proposed path is very simple and solves the aforementioned problems by
introducing new argument "latest" for recovery_target parameter.
Does the tool remove or rename recovery.conf to cancel the settings?
And do you intend that the new option is used to override settings by
appending it at the end of postgresql.conf? If so, the commit
f2cbffc7a6 seems to break the assumption. PG12 rejects to start if it
finds two different kinds of recovery target settings.
Yes, previously it was possible to remove/rename old recovery.conf,
but not anymore.
My assumption is exactly that PG should reject to start because of
multiple recovery targets.
Failing to start is infinitely better that recovering to the wrong
recovery target.
Old recovery behavior is still available if no recovery target is
provided. I`m not sure, whether it should it be left as it is now, or
not.
Another open question is what to do with recovery_target_inclusive if
recovery_target = "latest" is used.
Anyway inclusiveness doesn't affect to "immediate". If we had the
"latest" option, it would behave the same way.
Right, thank you.
regards.
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 619ac8c50c..f0fa0b09c8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3316,21 +3316,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
- recovery_target = 'immediate'
+ recovery_target (string)
recovery_target configuration parameter
-This parameter specifies that recovery should end as soon as a
-consistent state is reached, i.e. as early as possible. When restoring
-from an online backup, this means the point where taking the backup
-ended.
-
-
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
+This parameter determines how far recovery should proceed. The value
+immediate means that recovery should end as soon as a consistent
+state is reached, i.e. as early as possible. When restoring from an online
+backup, this means the point where taking the backup ended.
+The second possible value latest means that recovery
+should proceed to the end of the available WAL log.
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 77ad765989..707ebe68a1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6350,6 +6350,9 @@ StartupXLOG(void)
else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
ereport(LOG,
(errmsg("starting point-in-time recovery to earliest consistent point")));
+ else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+ ereport(LOG,
+ (errmsg("starting point-in-time recovery until all available WAL is applied")));
else
ereport(LOG,
(errmsg("starting archive recovery")));
@@ -7261,6 +7264,13 @@ StartupXLOG(void)
* end of main redo apply loop
*/
+ /*
+ * If all avaialble WAL is replayed,
+ * then RECOVERY_TARGET_LATEST is satisfied
+ */
+ if (recoveryTarget == RECOVERY_TARGET_LATEST)
+reachedStopPoint = true;
+
if (reachedStopPoint)
{
if (!reachedConsistency)
@@ -7462,6 +7472,8 @@ StartupXLOG(void)
recoveryStopName);
else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
snprintf(reason, sizeof(reason), "reached consistency");
+ else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+ snprintf(reason, sizeof(reason), "applied all available WAL");
else
snprintf(reason, sizeof(reason), "no recovery target specified");
Re: [proposal] recovery_target "latest"
On 11/6/19 10:39 AM, Peter Eisentraut wrote: This seems to also be related to this discussion: <https://www.postgresql.org/message-id/flat/993736dd3f1713ec1f63fc3b65383...@lako.no> Yes, in a way. Strengthening current lax recovery behavior is a very good idea. I like this idea. I don't like the name "latest". What does that mean? Other documentation talks about the "end of the archive". What does that mean? It means until restore_command errors. Let's think of a name that reflects that better. Maybe "all_archive" or something like that. As with "immediate", "latest" reflects the latest possible state this PostgreSQL instance can achieve when using PITR. I think it is simple and easy to understand for an end user, which sees PITR as a way to go from one state to another. In my experience, at least, which is, of course, subjective. But if we want an argument name to be technically accurate, then, I think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL" is a way to go. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [proposal] recovery_target "latest"
On 11/6/19 12:56 PM, Fujii Masao wrote: On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin wrote: On 11/6/19 10:39 AM, Peter Eisentraut wrote: This seems to also be related to this discussion: <https://www.postgresql.org/message-id/flat/993736dd3f1713ec1f63fc3b65383...@lako.no> Yes, in a way. Strengthening current lax recovery behavior is a very good idea. I like this idea. I don't like the name "latest". What does that mean? Other documentation talks about the "end of the archive". What does that mean? It means until restore_command errors. Let's think of a name that reflects that better. Maybe "all_archive" or something like that. As with "immediate", "latest" reflects the latest possible state this PostgreSQL instance can achieve when using PITR. I think it is simple and easy to understand for an end user, which sees PITR as a way to go from one state to another. In my experience, at least, which is, of course, subjective. But if we want an argument name to be technically accurate, then, I think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL" is a way to go. What happens if this parameter is set to latest in the standby mode? Or the combination of those settings should be prohibited? Currently it will behave just like regular standby, so I think, to avoid confusion and keep things simple, the combination of them should be prohibited. Thank you for pointing this out, I will work on it. The other way around, as I see it, is to define RECOVERY_TARGET_LATEST as something more complex, for example, the latest possible endptr in latest WAL segment. But it is tricky, because WAL archive may keeps growing as recovery is progressing or, in case of standby, master keeps sending more and more WAL. Regards, -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [proposal] recovery_target "latest"
On 11/6/19 1:55 PM, Grigory Smolkin wrote:
On 11/6/19 12:56 PM, Fujii Masao wrote:
On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin
wrote:
On 11/6/19 10:39 AM, Peter Eisentraut wrote:
This seems to also be related to this discussion:
<https://www.postgresql.org/message-id/flat/993736dd3f1713ec1f63fc3b65383...@lako.no>
Yes, in a way. Strengthening current lax recovery behavior is a very
good idea.
I like this idea.
I don't like the name "latest". What does that mean? Other
documentation talks about the "end of the archive". What does that
mean? It means until restore_command errors. Let's think of a name
that reflects that better. Maybe "all_archive" or something like
that.
As with "immediate", "latest" reflects the latest possible state this
PostgreSQL instance can achieve when using PITR. I think it is simple
and easy to understand for an end user, which sees PITR as a way to go
from one state to another. In my experience, at least, which is, of
course, subjective.
But if we want an argument name to be technically accurate, then, I
think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL" is
a way to go.
What happens if this parameter is set to latest in the standby mode?
Or the combination of those settings should be prohibited?
Currently it will behave just like regular standby, so I think, to
avoid confusion and keep things simple, the combination of them should
be prohibited.
Thank you for pointing this out, I will work on it.
Attached new patch revision, now it is impossible to use recovery_target
'latest' in standby mode.
TAP test is updated to reflect this behavior.
The other way around, as I see it, is to define RECOVERY_TARGET_LATEST
as something more complex, for example, the latest possible endptr in
latest WAL segment. But it is tricky, because WAL archive may keeps
growing as recovery is progressing or, in case of standby, master
keeps sending more and more WAL.
Regards,
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 619ac8c50c..f0fa0b09c8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3316,21 +3316,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
- recovery_target = 'immediate'
+ recovery_target (string)
recovery_target configuration parameter
-This parameter specifies that recovery should end as soon as a
-consistent state is reached, i.e. as early as possible. When restoring
-from an online backup, this means the point where taking the backup
-ended.
-
-
-Technically, this is a string parameter, but 'immediate'
-is currently the only allowed value.
+This parameter determines how far recovery should proceed. The value
+immediate means that recovery should end as soon as a consistent
+state is reached, i.e. as early as possible. When restoring from an online
+backup, this means the point where taking the backup ended.
+The second possible value latest means that recovery
+should proceed to the end of the available WAL log.
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 77ad765989..6001b0aa34 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5401,6 +5401,15 @@ validateRecoveryParameters(void)
errmsg("must specify restore_command when standby mode is not enabled")));
}
+ /* Combining standby mode with recovery to the end of WAL is
+ * impossible, because there is no end of WAL in this case.
+ */
+ if (StandbyModeRequested && recoveryTarget == RECOVERY_TARGET_LATEST)
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("recovery to the end of WAL is not possible because standby mode is enabled")));
+
+
/*
* Override any inconsistent requests. Note that this is a change of
* behaviour in 9.5; prior to this we simply ignored a request to pause if
@@ -6350,6 +6359,9 @@ StartupXLOG(void)
else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
ereport(LOG,
(errmsg("starting point-in-time recovery to earliest consistent point")));
+ else if (recoveryTarget == RECOVERY_TARGET_LATEST)
+ ereport(LOG,
+ (errmsg("starting point-in-time recovery until all available WAL is applied")));
else
ereport(LOG,
(errmsg("starting archive recovery")));
@@ -7261,6 +7273,13 @@ StartupXLOG(void)
* end of main redo apply loop
*/
+ /*
+ * If all avaialble WAL is replayed,
+ * then RECOVERY_TAR
Re: [proposal] recovery_target "latest"
On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote: At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin wrote in On 11/6/19 1:55 PM, Grigory Smolkin wrote: On 11/6/19 12:56 PM, Fujii Masao wrote: On Wed, Nov 6, 2019 at 6:33 PM Grigory Smolkin wrote: On 11/6/19 10:39 AM, Peter Eisentraut wrote: This seems to also be related to this discussion: <https://www.postgresql.org/message-id/flat/993736dd3f1713ec1f63fc3b65383...@lako.no> Yes, in a way. Strengthening current lax recovery behavior is a very good idea. I like this idea. I don't like the name "latest". What does that mean? Other documentation talks about the "end of the archive". What does that mean? It means until restore_command errors. Let's think of a name that reflects that better. Maybe "all_archive" or something like that. As with "immediate", "latest" reflects the latest possible state this PostgreSQL instance can achieve when using PITR. I think it is simple and easy to understand for an end user, which sees PITR as a way to go from one state to another. In my experience, at least, which is, of course, subjective. But if we want an argument name to be technically accurate, then, I think, something like "end-of-available-WAL"/"all-WAL", "end-of-WAL" is a way to go. What happens if this parameter is set to latest in the standby mode? Or the combination of those settings should be prohibited? Currently it will behave just like regular standby, so I think, to avoid confusion and keep things simple, the combination of them should be prohibited. Thank you for pointing this out, I will work on it. Attached new patch revision, now it is impossible to use recovery_target 'latest' in standby mode. TAP test is updated to reflect this behavior. In the first place, latest (or anything it could be named as) is defined as the explit label for the default behavior. Thus the latest should work as if nothing is set to recovery_target* following the definition. That might seems somewhat strange but I think at least it is harmless. Well, it was more about getting default behavior by using some explicit recovery_target, not the other way around. Because it will break some 3rd party backup and replication applications, that may rely on old behavior of ignoring recovery_target_action when no recovery_target is provided. But you think that it is worth pursuing, I can do that. recovery_target=immediate + r_t_action=shutdown for a standby works as commanded. Do we need to inhibit that, too? Why something, that work as expected, should be inhibited? The other way around, as I see it, is to define RECOVERY_TARGET_LATEST as something more complex, for example, the latest possible endptr in latest WAL segment. But it is tricky, because WAL archive may keeps growing as recovery is progressing or, in case of standby, master keeps sending more and more WAL. regards. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [proposal] recovery_target "latest"
On 11/7/19 12:56 PM, Kyotaro Horiguchi wrote: At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin wrote in On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote: At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin wrote in On 11/6/19 1:55 PM, Grigory Smolkin wrote: On 11/6/19 12:56 PM, Fujii Masao wrote: What happens if this parameter is set to latest in the standby mode? Or the combination of those settings should be prohibited? Currently it will behave just like regular standby, so I think, to avoid confusion and keep things simple, the combination of them should be prohibited. Thank you for pointing this out, I will work on it. Attached new patch revision, now it is impossible to use recovery_target 'latest' in standby mode. TAP test is updated to reflect this behavior. In the first place, latest (or anything it could be named as) is defined as the explit label for the default behavior. Thus the latest should work as if nothing is set to recovery_target* following the definition. That might seems somewhat strange but I think at least it is harmless. Well, it was more about getting default behavior by using some explicit recovery_target, not the other way around. Because it will break some 3rd party backup and replication applications, that may rely on old behavior of ignoring recovery_target_action when no recovery_target is provided. But you think that it is worth pursuing, I can do that. Ah. Sorry for the misleading statement. What I had in my mind was somewhat the mixture of them. I thought that recovery_target ='' behaves the same way as now, r_t_action is ignored. And 'latest' just makes recovery_target_action work as the current non-empty recovery_target's does. But I'm not confident that it is a good design. recovery_target=immediate + r_t_action=shutdown for a standby works as commanded. Do we need to inhibit that, too? Why something, that work as expected, should be inhibited? To make sure, I don't think we should do that. I meant by the above that standby mode is already accepting recovery_target_action so inhibiting that only for 'latest' is not orthogonal and could be more confusing for users, and complicatig the code. So my opinion is we shouldn't inhibit 'latest' unless r_t_action harms. I gave it some thought and now think that prohibiting recovery_target 'latest' and standby was a bad idea. All recovery_targets follow the same pattern of usage, so recovery_target "latest" also must be capable of working in standby mode. All recovery_targets have a clear deterministic 'target' where recovery should end. In case of recovery_target "latest" this target is the end of available WAL, therefore the end of available WAL must be more clearly defined. I will work on it. Thank you for a feedback. regards. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [proposal] recovery_target "latest"
On 11/7/19 4:36 PM, Grigory Smolkin wrote: On 11/7/19 12:56 PM, Kyotaro Horiguchi wrote: At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin wrote in On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote: At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin wrote in On 11/6/19 1:55 PM, Grigory Smolkin wrote: On 11/6/19 12:56 PM, Fujii Masao wrote: What happens if this parameter is set to latest in the standby mode? Or the combination of those settings should be prohibited? Currently it will behave just like regular standby, so I think, to avoid confusion and keep things simple, the combination of them should be prohibited. Thank you for pointing this out, I will work on it. Attached new patch revision, now it is impossible to use recovery_target 'latest' in standby mode. TAP test is updated to reflect this behavior. In the first place, latest (or anything it could be named as) is defined as the explit label for the default behavior. Thus the latest should work as if nothing is set to recovery_target* following the definition. That might seems somewhat strange but I think at least it is harmless. Well, it was more about getting default behavior by using some explicit recovery_target, not the other way around. Because it will break some 3rd party backup and replication applications, that may rely on old behavior of ignoring recovery_target_action when no recovery_target is provided. But you think that it is worth pursuing, I can do that. Ah. Sorry for the misleading statement. What I had in my mind was somewhat the mixture of them. I thought that recovery_target ='' behaves the same way as now, r_t_action is ignored. And 'latest' just makes recovery_target_action work as the current non-empty recovery_target's does. But I'm not confident that it is a good design. recovery_target=immediate + r_t_action=shutdown for a standby works as commanded. Do we need to inhibit that, too? Why something, that work as expected, should be inhibited? To make sure, I don't think we should do that. I meant by the above that standby mode is already accepting recovery_target_action so inhibiting that only for 'latest' is not orthogonal and could be more confusing for users, and complicatig the code. So my opinion is we shouldn't inhibit 'latest' unless r_t_action harms. I gave it some thought and now think that prohibiting recovery_target 'latest' and standby was a bad idea. All recovery_targets follow the same pattern of usage, so recovery_target "latest" also must be capable of working in standby mode. All recovery_targets have a clear deterministic 'target' where recovery should end. In case of recovery_target "latest" this target is the end of available WAL, therefore the end of available WAL must be more clearly defined. I will work on it. Thank you for a feedback. Attached new patch revision, now end of available WAL is defined as the fact of missing required WAL. In case of standby, the end of WAL is defined as 2 consecutive switches of WAL source, that didn`t provided requested record. In case of streaming standby, each switch of WAL source is forced after 3 failed attempts to get new data from walreceiver. All constants are taken off the top of my head and serves as proof of concept. TAP test is updated. regards. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 619ac8c50c..f0fa0b09c8 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -3316,21 +3316,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows - recovery_target = 'immediate' + recovery_target (string) recovery_target configuration parameter -This parameter specifies that recovery should end as soon as a -consistent state is reached, i.e. as early as possible. When restoring -from an online backup, this means the point where taking the backup -ended. - - -Technically, this is a string parameter, but 'immediate' -is currently the only allowed value. +This parameter determines how far recovery should proceed. The value +immediate means that recovery should end as soon as a consistent +state is reached, i.e. as early as possible. When restoring from an online +backup, this means the point where taking the backup ended. +The second possible value latest means that recovery +should proceed to the end of the available WAL log. diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 77ad765989..74b1a6696d 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/a
Re: [proposal] recovery_target "latest"
On 11/8/19 7:00 AM, Grigory Smolkin wrote: On 11/7/19 4:36 PM, Grigory Smolkin wrote: On 11/7/19 12:56 PM, Kyotaro Horiguchi wrote: At Thu, 7 Nov 2019 12:22:28 +0300, Grigory Smolkin wrote in On 11/7/19 8:36 AM, Kyotaro Horiguchi wrote: At Thu, 7 Nov 2019 02:28:39 +0300, Grigory Smolkin wrote in On 11/6/19 1:55 PM, Grigory Smolkin wrote: On 11/6/19 12:56 PM, Fujii Masao wrote: What happens if this parameter is set to latest in the standby mode? Or the combination of those settings should be prohibited? Currently it will behave just like regular standby, so I think, to avoid confusion and keep things simple, the combination of them should be prohibited. Thank you for pointing this out, I will work on it. Attached new patch revision, now it is impossible to use recovery_target 'latest' in standby mode. TAP test is updated to reflect this behavior. In the first place, latest (or anything it could be named as) is defined as the explit label for the default behavior. Thus the latest should work as if nothing is set to recovery_target* following the definition. That might seems somewhat strange but I think at least it is harmless. Well, it was more about getting default behavior by using some explicit recovery_target, not the other way around. Because it will break some 3rd party backup and replication applications, that may rely on old behavior of ignoring recovery_target_action when no recovery_target is provided. But you think that it is worth pursuing, I can do that. Ah. Sorry for the misleading statement. What I had in my mind was somewhat the mixture of them. I thought that recovery_target ='' behaves the same way as now, r_t_action is ignored. And 'latest' just makes recovery_target_action work as the current non-empty recovery_target's does. But I'm not confident that it is a good design. recovery_target=immediate + r_t_action=shutdown for a standby works as commanded. Do we need to inhibit that, too? Why something, that work as expected, should be inhibited? To make sure, I don't think we should do that. I meant by the above that standby mode is already accepting recovery_target_action so inhibiting that only for 'latest' is not orthogonal and could be more confusing for users, and complicatig the code. So my opinion is we shouldn't inhibit 'latest' unless r_t_action harms. I gave it some thought and now think that prohibiting recovery_target 'latest' and standby was a bad idea. All recovery_targets follow the same pattern of usage, so recovery_target "latest" also must be capable of working in standby mode. All recovery_targets have a clear deterministic 'target' where recovery should end. In case of recovery_target "latest" this target is the end of available WAL, therefore the end of available WAL must be more clearly defined. I will work on it. Thank you for a feedback. Attached new patch revision, now end of available WAL is defined as the fact of missing required WAL. In case of standby, the end of WAL is defined as 2 consecutive switches of WAL source, that didn`t provided requested record. In case of streaming standby, each switch of WAL source is forced after 3 failed attempts to get new data from walreceiver. All constants are taken off the top of my head and serves as proof of concept. TAP test is updated. Attached new revision, it contains some minor refactoring. While working on it, I stumbled upon something strange: why DisownLatch(&XLogCtl->recoveryWakeupLatch) is called before ReadRecord(xlogreader, LastRec, PANIC, false) ? Isn`t this latch may be accessed in WaitForWALToBecomeAvailable() if streaming standby gets promoted? regards. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 0191ec84b1..49675c38da 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -3366,21 +3366,19 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows - recovery_target = 'immediate' + recovery_target (string) recovery_target configuration parameter -This parameter specifies that recovery should end as soon as a -consistent state is reached, i.e. as early as possible. When restoring -from an online backup, this means the point where taking the backup -ended. - - -Technically, this is a string parameter, but 'immediate' -is currently the only allowed value. +This parameter determines how far recovery should proceed. The value +immediate means that recovery should end as soon as a consistent +state is reached, i.e. as early as possible. When restoring from an onlin
Re: pg_upgrade fails with non-standard ACL
HelLo!
On 11/9/19 5:26 AM, Michael Paquier wrote:
On Fri, Nov 08, 2019 at 06:03:06PM +0900, Michael Paquier wrote:
I have begun looking at this one.
Another question I have: do we need to care more about other extra
ACLs applied to other object types? For example a subset of columns
on a table with a column being renamed could be an issue. Procedure
renamed in core are not that common still we did it.
I think that all objects must be supported.
User application may depend on them, so silently casting them to the
void during upgrade may ruin someones day.
But also I think, that this work should be done piecemeal, to make
testing and reviewing easier, and functions are a good candidate for a
first go.
I think that we should keep the core part of the fix more simple by
just warning about missing function signatures in the target cluster
when pg_upgrade --check is used.
I think that warning without any concrete details on functions involved
is confusing.
So I think that it would be better
for now to get to a point where we can warn about the function
signatures involved in a given database, without the generation of the
script with those REVOKE queries. Or would folks prefer keep that in
the first version? My take would be to handle both separately, and to
warn about everything so as there is no need to do pg_upgrade --check
more than once.
I would prefer to warn about every function (so he can more quickly
assess the situation) AND generate script. It is good to save some user
time, because he is going to need that script anyway.
I`ve tested the master patch:
1. upgrade from 9.5 and lower is broken:
[gsmol@deck upgrade_workdir]$ /home/gsmol/task/13_devel/bin/pg_upgrade
-b /home/gsmol/task/9.5.19/bin/ -B /home/gsmol/task/13_devel/bin/ -d
/home/gsmol/task/9.5.19/data -D /tmp/upgrade
Performing Consistency Checks
-
Checking cluster versions ok
SQL command failed
select proname::text || '(' ||
pg_get_function_arguments(pg_proc.oid)::text || ')' as funsig, array
(SELECT unnest(pg_proc.proacl) EXCEPT SELECT
unnest(pg_init_privs.initprivs)) from pg_proc join pg_init_privs on
pg_proc.oid = pg_init_privs.objoid where pg_init_privs.classoid =
'pg_proc'::regclass::oid and pg_init_privs.privtype = 'i' and
pg_proc.proisagg = false and pg_proc.proacl != pg_init_privs.initprivs;
ERROR: relation "pg_init_privs" does not exist
LINE 1: ...nnest(pg_init_privs.initprivs)) from pg_proc join pg_init_pr...
^
Failure, exiting
2. I think that message "Your installation contains non-default
privileges for system procedures for which the API has changed." must
contain versions:
"Your PostgreSQL 9.5 installation contains non-default privileges for
system procedures for which the API has changed in PostgreSQL 12."
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Re: pg_upgrade fails with non-standard ACL
Hello! On 11/29/19 11:07 AM, Artur Zakirov wrote: If Anastasia doesn't mind I'd like to send new version of the patch. On 2019/11/28 12:29, Artur Zakirov wrote: On 2019/11/27 13:22, Michael Paquier wrote: Yeah, the actual take is if we want to make the frontend code more complicated with a large set of SQL queries to check that each object ACL is modified, which adds an additional maintenance cost in pg_upgrade. Or if we want to keep the frontend simple and have more backend facility to ease cross-catalog lookups for ACLs. Perhaps we could also live with just checking after the ACLs of functions in the short term and perhaps it covers most of the cases users would care about.. That's tricky to conclude about and I am not sure which path is better in the long-term, but at least it's worth discussing all possible candidates IMO so as we make sure to not miss anything. I checked what objects changed their signatures between master and 9.6. I just ran pg_describe_object() for grantable object types, saved the output into a file and diffed the outputs. It seems that only functions and table columns changed their signatures. A list of functions is big and here the list of columns: table pg_attrdef column adsrc table pg_class column relhasoids table pg_class column relhaspkey table pg_constraint column consrc table pg_proc column proisagg table pg_proc column proiswindow table pg_proc column protransform As a result I think in pg_upgrade we could just check functions and columns signatures. It might simplify the patch. And if something changes in a future we could fix pg_upgrade too. New version of the patch differs from the previous: - it doesn't generate script to revoke conflicting permissions (but the patch can be fixed easily) - generates file incompatible_objects_for_acl.txt to report which objects changed their signatures - uses pg_shdepend and pg_depend catalogs to get objects with custom privileges - uses pg_describe_object() to find objects with different signatures Currently relations, attributes, languages, functions and procedures are scanned. According to the documentation foreign databases, foreign-data wrappers, foreign servers, schemas and tablespaces also support ACLs. But some of them doesn't have entries initialized during initdb, others like schemas and tablespaces didn't change their names. So the patch doesn't scan such objects. Grigory it would be great if you'll try the patch. I tested it but I could miss something. I`ve tested the patch on upgrade from 9.5 to master and it works now, thank you. But I think that 'incompatible_objects_for_acl.txt' is not a very informative way of reporting to user the source of the problem. There is no mentions of rolenames that holds permissions, that prevents the upgrade, and even if they were, it is still up to user to conjure an script to revoke all those grants, which is not a very user-friendly. I think it should generate 'catalog_procedures.sql' script as in previous version with all REVOKE statements, that are required to execute for pg_upgrade to work. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [proposal] recovery_target "latest"
On 11/21/19 1:46 PM, Peter Eisentraut wrote: On 2019-11-08 05:00, Grigory Smolkin wrote: Attached new patch revision, now end of available WAL is defined as the fact of missing required WAL. In case of standby, the end of WAL is defined as 2 consecutive switches of WAL source, that didn`t provided requested record. In case of streaming standby, each switch of WAL source is forced after 3 failed attempts to get new data from walreceiver. All constants are taken off the top of my head and serves as proof of concept. Well, this is now changing the meaning of the patch quite a bit. I'm on board with making the existing default behavior explicit. (This is similar to how we added recovery_target_timeline = 'current' in PG12.) Still not a fan of the name yet, but that's trivial. If, however, you want to change the default behavior or introduce a new behavior, as you are suggesting here, that should be a separate discussion. No, default behavior is not to be changed. As I previously mentioned, it may break the backward compatibility for 3rd party backup and replication applications. At Fri, 8 Nov 2019 16:08:47 +0300, Grigory Smolkin wrote in On 11/8/19 7:00 AM, Grigory Smolkin wrote: On 11/7/19 4:36 PM, Grigory Smolkin wrote: I gave it some thought and now think that prohibiting recovery_target 'latest' and standby was a bad idea. All recovery_targets follow the same pattern of usage, so recovery_target "latest" also must be capable of working in standby mode. All recovery_targets have a clear deterministic 'target' where recovery should end. In case of recovery_target "latest" this target is the end of available WAL, therefore the end of available WAL must be more clearly defined. I will work on it. Thank you for a feedback. Attached new patch revision, now end of available WAL is defined as the fact of missing required WAL. In case of standby, the end of WAL is defined as 2 consecutive switches of WAL source, that didn`t provided requested record. In case of streaming standby, each switch of WAL source is forced after 3 failed attempts to get new data from walreceiver. All constants are taken off the top of my head and serves as proof of concept. TAP test is updated. Attached new revision, it contains some minor refactoring. Thanks for the new patch. I found that it needs more than I thought, but it seems a bit too complicated and less stable. As the patch does, WaitForWALToBecomeAvailable needs to exit when avaiable sources are exhausted. However, we don't need to count failures to do that. It is enough that the function have two more exit point. When streaming timeout fires, and when we found that streaming is not set up when archive/wal failed. In my opinion, it is better that we have less dependency to global variables in such deep levels in a call hierachy. Such nformation can be stored in XLogPageReadPrivate. Many thanks! It looks much better that my approach with global variables. I`ve tested it and have some thoughts/concerns: 1. Recovery should report the exact reason why it has been forced to stop. In case of recovering to the end of WAL, standby promotion request received during recovery could be mistaken for reaching the end of WAL and reported as such. To avoid this, I think that reachedEndOfWal variable should be introduced. In attached patch it is added as a global variable, but maybe something more clever may be devised. I was not sure that reachedEndOfWal could be placed in XLogPageReadPrivate. Because we need to access it at the higher level than ReadRecord(), and I was under impression placing it in XLogPageReadPrivate could violate abstraction level of XLogPageReadPrivate. 2. During the testing, I`ve stumbled upon assertion failure in case of recovering in standby mode to the the end of WAL coupled with recovery_target_action as "promote", caused by the WAL source in state machine not been changed after reaching the recovery target (script to reproduce is attached): 2019-12-07 13:45:54.468 MSK [23067] LOG: starting PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1), 64-bit 2019-12-07 13:45:54.468 MSK [23067] LOG: listening on IPv4 address "127.0.0.1", port 15433 2019-12-07 13:45:54.470 MSK [23067] LOG: listening on Unix socket "/tmp/.s.PGSQL.15433" 2019-12-07 13:45:54.475 MSK [23068] LOG: database system was interrupted; last known up at 2019-12-07 13:45:49 MSK cp: cannot stat '/home/gsmol/task/13_devel/archive/0002.history': No such file or directory 2019-12-07 13:45:54.602 MSK [23068] LOG: entering standby mode 2019-12-07 13:45:54.614 MSK [23068] LOG: restored log file "00010002" from archive 2019-12-07 13:45:54.679 MSK [23068] LOG: redo starts at 0/228 2019-12-07 13:45:54.682 MSK [23068] LOG: consistent recovery state reached at 0/200
Re: Function to track shmem reinit time
It can be used to accurately calculate server uptime, since you can`t rely on pg_postmaster_start_time() in this. On 02/28/2018 03:11 PM, Anastasia Lubennikova wrote: Attached patch introduces a new function pg_shmem_init_time(), which returns the time shared memory was last (re)initialized. It is created for use by monitoring tools to track backend crashes. Currently, if the 'restart_after_crash' option is on, postgres will just restart. And the only way to know that it happened is to regularly parse logfile or monitor it, catching restart messages. This approach is really inconvenient for users, who have gigabytes of logs. This new function can be periodiacally called by a monitoring agent, and, if /shmem_init_time/ doesn't match /pg_postmaster_start_time,/ we know that server crashed-restarted, and also know the exact time, when. Also, working on this patch, I noticed a bit of dead code and some discordant comments in postmaster.c. I see no reason to leave it as is. So there is a small remove_dead_shmem_reinit_code_v0.patch. -- Anastasia Lubennikova Postgres Professional:http://www.postgrespro.com The Russian Postgres Company -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Reopen logfile on SIGHUP
If there is already SIGUSR1 for logfile reopening then shouldn`t postmaster watch for it? Postmaster PID is easy to obtain. On 02/28/2018 01:32 AM, Greg Stark wrote: On 27 February 2018 at 14:41, Anastasia Lubennikova wrote: Small patch in the attachment implements logfile reopeninig on SIGHUP. It only affects the file accessed by logging collector, which name you can check with pg_current_logfile(). HUP will cause Postgres to reload its config files. That seems like a fine time to reopen the log files as well but it would be nice if there was also some way to get it to *just* do that and not reload the config files. I wonder if it would be easiest to just have the syslogger watch for some other signal as well (I'm guessing the the syslogger doesn't use relcache invalidations so it could reuse USR1 for example). That would be a bit inconvenient as the admins would have to find the syslogger and deliver the signal directly, rather than through the postmaster but it would be pretty easy for them. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Changing the autovacuum launcher scheduling; oldest table first algorithm
On 02/28/2018 12:04 PM, Masahiko Sawada wrote: Hi, I've created the new thread for the changing AV launcher scheduling. The problem of AV launcher scheduling is described on [1] but I summarize it here. If there is even one database that is at risk of wraparound, currently AV launcher selects the database having the oldest datfrozenxid until all tables in that database have been vacuumed. This leads that tables on other database can bloat and not be frozen because other database are not selected by AV launcher. It makes things worse if the database has a large table that takes a long time to be vacuumed. Attached patch changes the AV launcher scheduling algorithm based on the proposed idea by Robert so that it selects a database first that has the oldest table when the database is at risk of wraparound. For detail of the algorithm please refer to [2]. In this algorithm, the running AV workers advertise the next datfrozenxid on the shared memory that we will have. That way, AV launcher can select a database that has the oldest table in the database cluster. However, this algorithm doesn't support the case where the age of next datfrozenxid gets greater than autovacum_vacuum_max_age. This case can happen if users set autovacvuum_vacuum_max_age to small value and vacuuming the whole database takes a long time. But since it's not a common case and it doesn't degrade than current behavior even if this happened, I think it's not a big problem. Feedback is very welcome. [1] https://www.postgresql.org/message-id/0A3221C70F24FB45833433255569204D1F8A4DC6%40G01JPEXMBYT05 [2] https://www.postgresql.org/message-id/CA%2BTgmobT3m%3D%2BdU5HF3VGVqiZ2O%2Bv6P5wN1Gj%2BPrq%2Bhj7dAm9AQ%40mail.gmail.com Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center Hello! I`ve tried to compile your patch on Fedora24 with gcc 6.3.1 20161221: gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -g3 -O0 -I../../../src/include -D_GNU_SOURCE -c -o autovacuum.o autovacuum.c In file included from ../../../src/include/postgres.h:46:0, from autovacuum.c:62: autovacuum.c: In function ‘get_next_xid_bounds’: autovacuum.c:1979:9: warning: implicit declaration of function ‘TransactionIdIsVaild’ [-Wimplicit-function-declaration] Assert(TransactionIdIsVaild(frozenxid)); ^ ../../../src/include/c.h:713:7: note: in definition of macro ‘Trap’ if (condition) \ ^ autovacuum.c:1979:2: note: in expansion of macro ‘Assert’ Assert(TransactionIdIsVaild(frozenxid)); ^~ autovacuum.c:1980:28: error: ‘minmutli’ undeclared (first use in this function) Assert(MultiXactIdIsValid(minmutli)); ^ ../../../src/include/c.h:713:7: note: in definition of macro ‘Trap’ if (condition) \ ^ autovacuum.c:1980:2: note: in expansion of macro ‘Assert’ Assert(MultiXactIdIsValid(minmutli)); ^~ autovacuum.c:1980:9: note: in expansion of macro ‘MultiXactIdIsValid’ Assert(MultiXactIdIsValid(minmutli)); ^~ autovacuum.c:1980:28: note: each undeclared identifier is reported only once for each function it appears in Assert(MultiXactIdIsValid(minmutli)); ^ ../../../src/include/c.h:713:7: note: in definition of macro ‘Trap’ if (condition) \ ^ autovacuum.c:1980:2: note: in expansion of macro ‘Assert’ Assert(MultiXactIdIsValid(minmutli)); ^~ autovacuum.c:1980:9: note: in expansion of macro ‘MultiXactIdIsValid’ Assert(MultiXactIdIsValid(minmutli)); ^~ : recipe for target 'autovacuum.o' failed make[3]: *** [autovacuum.o] Error 1 -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Function to track shmem reinit time
8 07:02 PM, Tomas Vondra wrote: Actually, after looking at the code a bit, I think that test would not really work anyway, because those two timestamps come from two separate GetCurrentTimestamp calls, so you get this: test=# select pg_shmem_init_time(), pg_postmaster_start_time(); pg_shmem_init_time | pg_postmaster_start_time ---+--- 2018-03-04 17:10:59.017058+01 | 2018-03-04 17:10:59.022071+01 (1 row) test=# select pg_shmem_init_time() = pg_postmaster_start_time(); ?column? -- f (1 row) So there would have to be some sort of "fuzz" parameter when comparing the values, etc. The difference measured in milliseconds is expected. After all, shared memory is initialized after postmaster start. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
archive recovery fetching wrong segments
Hello, hackers!
I`m investigating a complains from our clients about archive recovery
speed been very slow, and I`ve noticed a really strange and, I think, a
very dangerous recovery behavior.
When running multi-timeline archive recovery, for every requested segno
startup process iterates through every timeline in restore target
timeline history, starting from highest timeline and ending in current,
and tries to fetch the segno in question from this timeline.
Consider the following example.
Timelines:
ARCHIVE INSTANCE 'node'
TLI Parent TLI Switchpoint Min Segno Max
Segno N segments Size Zratio N backups Status
3 2 0/AEFFEDE0 000300AE
000300D5 40 41MB 15.47 0 OK
2 1 0/A08768D0 000200A0
000200AE 15 14MB 17.24 0 OK
1 0 0/0 00010001
000100BB 187 159MB 18.77 1 OK
Backup:
Instance Version ID Recovery Time Mode WAL Mode
TLI Time Data WAL Zratio Start LSN Stop LSN Status
node 11 Q8C8IH 2020-04-06 02:13:31+03 FULL ARCHIVE 1/0
3s 23MB 16MB 1.00 0/228 0/3B8 OK
So when we are trying to restore this backup, located on Timeline 1, to
the restore target on Timeline 3, we are getting this in the PostgreSQL
log:
2020-04-05 23:24:36 GMT [28508]: [5-1] LOG: restored log file
"0003.history" from archive
INFO: PID [28511]: pg_probackup archive-get WAL file:
00030002, remote: none, threads: 1/1, batch: 20
ERROR: PID [28511]: pg_probackup archive-get failed to deliver WAL file
00030002, prefetched: 0/20, time elapsed: 0ms
INFO: PID [28512]: pg_probackup archive-get WAL file:
00020002, remote: none, threads: 1/1, batch: 20
ERROR: PID [28512]: pg_probackup archive-get failed to deliver WAL file
00020002, prefetched: 0/20, time elapsed: 0ms
INFO: PID [28513]: pg_probackup archive-get WAL file:
00010002, remote: none, threads: 1/1, batch: 20
INFO: PID [28513]: pg_probackup archive-get copied WAL file
00010002
2020-04-05 23:24:36 GMT [28508]: [6-1] LOG: restored log file
"00010002" from archive
...
Before requesting 00010002 recovery tries to fetch
00030002 and 00020002 and that goes for
every segment, restored from the archive.
This tremendously slows down recovery speed, especially if archive is
located on remote machine with high latency network.
And it also may lead to feeding recovery with wrong WAL segment, located
on the next timeline.
Is there a reason behind this behavior?
Also I`ve attached a patch, which fixed this issue for me, but I`m not
sure, that chosen approach is sound and didn`t break something.
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f6cd4fde2b..b4ddb246c2 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3714,12 +3714,30 @@ XLogFileReadAnyTLI(XLogSegNo segno, int emode, int source)
foreach(cell, tles)
{
TimeLineID tli = ((TimeLineHistoryEntry *) lfirst(cell))->tli;
+ XLogRecPtr switchpoint = ((TimeLineHistoryEntry *) lfirst(cell))->begin;
if (tli < curFileTLI)
break;/* don't bother looking at too-old TLIs */
if (source == XLOG_FROM_ANY || source == XLOG_FROM_ARCHIVE)
{
+ /* If current timeline is from the future ... */
+ if (tli > curFileTLI && switchpoint != InvalidXLogRecPtr)
+ {
+XLogSegNo switchSegno;
+
+XLByteToSeg(switchpoint, switchSegno, wal_segment_size);
+
+/* ... and requested segno is not the segment with switchpoint, then
+ * skip current timeline */
+if (segno < switchSegno)
+{
+ elog(WARNING, "Skipping timeline: %u, switch segno: %u, current segno: %u", tli, switchSegno, segno);
+ continue;
+}
+ }
+
+
fd = XLogFileRead(segno, emode, tli,
XLOG_FROM_ARCHIVE, true);
if (fd != -1)
Re: archive recovery fetching wrong segments
On 4/6/20 9:17 PM, David Steele wrote: Hi Grigory, Hello! On 4/5/20 8:02 PM, Grigory Smolkin wrote: Hello, hackers! I`m investigating a complains from our clients about archive recovery speed been very slow, and I`ve noticed a really strange and, I think, a very dangerous recovery behavior. When running multi-timeline archive recovery, for every requested segno startup process iterates through every timeline in restore target timeline history, starting from highest timeline and ending in current, and tries to fetch the segno in question from this timeline. Is there a reason behind this behavior? Also I`ve attached a patch, which fixed this issue for me, but I`m not sure, that chosen approach is sound and didn`t break something. This sure looks like [1] which has a completed patch nearly ready to commit. Can you confirm and see if the proposed patch looks good? Well I`ve been testing it all day and so far nothing is broken. But this foreach(xlog.c:3777) loop looks very strange to me, it is not robust, we are blindly going over timelines and feeding recovery some files, hoping they are the right ones. I think we can do better, because: 1. we know whether or not we are running multi-timeline recovery 2. we know next timeline ID and can calculate switchpoint segment 3. make an informed decision about from what timeline we must requesting files now. I will work on it. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: archive recovery fetching wrong segments
On 4/6/20 10:51 PM, David Steele wrote: On 4/6/20 3:23 PM, Grigory Smolkin wrote: On 4/6/20 9:17 PM, David Steele wrote: Hi Grigory, Hello! On 4/5/20 8:02 PM, Grigory Smolkin wrote: Hello, hackers! I`m investigating a complains from our clients about archive recovery speed been very slow, and I`ve noticed a really strange and, I think, a very dangerous recovery behavior. When running multi-timeline archive recovery, for every requested segno startup process iterates through every timeline in restore target timeline history, starting from highest timeline and ending in current, and tries to fetch the segno in question from this timeline. Is there a reason behind this behavior? Also I`ve attached a patch, which fixed this issue for me, but I`m not sure, that chosen approach is sound and didn`t break something. This sure looks like [1] which has a completed patch nearly ready to commit. Can you confirm and see if the proposed patch looks good? Well I`ve been testing it all day and so far nothing is broken. Perhaps I wasn't clear. There is a patch in this thread: https://www.postgresql.org/message-id/flat/792ea085-95c4-bca0-ae82-47fdc80e146d%40oss.nttdata.com#800f005e01af6cb3bfcd70c53007a2db which seems to address the same issue and is ready to be committed. I'd suggest you have a look at that patch and see if it fixes your issue. Ops, I`ve missed it. Thank you, I will look into it. Regards, -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Postgres is not able to handle more than 4k tables!?
On 7/8/20 11:41 PM, Konstantin Knizhnik wrote: So looks like NUM_LOCK_PARTITIONS and MAXNUMMESSAGES constants have to be replaced with GUCs. To avoid division, we can specify log2 of this values, so shift can be used instead. And MAX_SIMUL_LWLOCKS should be defined as NUM_LOCK_PARTITIONS + NUM_INDIVIDUAL_LWLOCKS + NAMED_LWLOCK_RESERVE. Because I was involved in this particular case and don`t want it to became a habit, I`m volunteering to test whatever patch this discussion will produce. -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [PATCH] xlogreader: do not read a file block twice
Hm, looks like it could speed up PostgreSQL recovery, but is it safe? On 02/11/2019 07:25 PM, Arthur Zakirov wrote: Hello hackers, Grigory noticed that one of our utilities has very slow performance when xlogreader reads zlib archives. We found out that xlogreader sometimes reads a WAL file block twice. zlib has slow performance when you read an archive not in sequential order. I think reading a block twice in same position isn't sequential, because gzread() moves current position forward and next call gzseek() to the same position moves it back. It seems that the attached patch solves the issue. I think when reqLen == state->readLen the requested block already is in the xlogreader's buffer. What do you think? -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: readdir is incorrectly implemented at Windows
Originally bug was reported by Yuri Kurenkov:
https://github.com/postgrespro/pg_probackup/issues/48
As pg_probackup rely on readdir() for listing files to backup, wrong
permissions could lead to a broken backup.
On 02/25/2019 06:38 PM, Konstantin Knizhnik wrote:
Hi hackers,
Small issue with readir implementation for Windows.
Right now it returns ENOENT in case of any error returned by
FindFirstFile.
So all places in Postgres where opendir/listdir are used will assume
that directory is empty and
do nothing without reporting any error.
It is not so good if directory is actually not empty but there are not
enough permissions for accessing the directory and FindFirstFile
returns ERROR_ACCESS_DENIED:
struct dirent *
readdir(DIR *d)
{
WIN32_FIND_DATA fd;
if (d->handle == INVALID_HANDLE_VALUE)
{
d->handle = FindFirstFile(d->dirname, &fd);
if (d->handle == INVALID_HANDLE_VALUE)
{
errno = ENOENT;
return NULL;
}
}
Attached please find small patch fixing the problem.
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Another fun fact about temp tables and wraparound
Hello, hackers!
Recently I was investigating the case of 'stuck in wraparaound' problem.
PostgreSQL instance(9.6.9) in question reached
'million-before-wraparound' threshold and switched to read-only mode.
Running vacuum in single-mode gives not results, datfrozenxid was not
advancing:
backend> vacuum freeze;
2018-07-13 16:43:58 MSK [3666-3] WARNING: database "database_name" must
be vacuumed within 991565 transactions
2018-07-13 16:43:58 MSK [3666-4] HINT: To avoid a database shutdown,
execute a database-wide VACUUM in that database.
You might also need to commit or roll back old prepared
transactions.
backend>
pg_prepared_xacts was empty.
After some poking around it became clear that some old temp table was
holding the oldest relfrozenxid!
vacuum during get_rel_oids() ignored temp table but didn`t when it comes
to calculating oldest relfrozenxid.
Dropping all temp schemas helped
Crude way to reproduce:
postgres=# create temp table t1();
gdb: set ShmemVariableCache->nextXid = ShmemVariableCache->xidStopLimit
+ 100
pg_ctl stop -D PGDATA
with open('path_to_clog_file', 'w') as f:
x = 0
while x < 20:
f.write(chr(1))
x = x + 1
postgres --single -D $PGDATA
PostgreSQL stand-alone backend 9.6.9
backend> vacuum freeze;
WARNING: database "postgres" must be vacuumed within 47 transactions
HINT: To avoid a database shutdown, execute a database-wide VACUUM in
that database.
You might also need to commit or roll back old prepared
transactions.
backend>
backend>
backend> vacuum freeze;
backend>
I think the root case of all temp table problems is that they are
present in catalog. I think they should not be present in catalog.
And vacuum probably should ignore them during datfrozenxid calculation.
In single mode at least. Or just drop them in single mode.
And it would be good to have advice 'drop temp schemas' in HINT message.
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Re: history file on replica and double switchover
Fujii Masao, David Zhang, Anastasia Lubennikova, many thanks to you for
efforts with this patch!
Can I mark it as ready for committer?
On 9/25/20 10:07 AM, Fujii Masao wrote:
On 2020/09/25 13:00, David Zhang wrote:
On 2020-09-24 5:00 p.m., Fujii Masao wrote:
On 2020/09/25 8:15, David Zhang wrote:
Hi,
My understanding is that the "archiver" won't even start if
"archive_mode = on" has been set on a "replica". Therefore, either
(XLogArchiveMode == ARCHIVE_MODE_ALWAYS) or (XLogArchiveMode !=
ARCHIVE_MODE_OFF) will produce the same result.
Yes, the archiver isn't invoked in the standby when archive_mode=on.
But, with the original patch, walreceiver creates .ready archive
status file
even when archive_mode=on and no archiver is running. This causes
the history file to be archived after the standby is promoted and
the archiver is invoked.
With my patch, walreceiver creates .ready archive status for the
history file
only when archive_mode=always, like it does for the streamed WAL files.
This is the difference between those two patches. To prevent the
streamed
timeline history file from being archived, IMO we should use the
condition
archive_mode==always in the walreceiver.
+1
Please see how the "archiver" is started in
src/backend/postmaster/postmaster.c
5227 /*
5228 * Start the archiver if we're responsible for
(re-)archiving received
5229 * files.
5230 */
5231 Assert(PgArchPID == 0);
5232 if (XLogArchivingAlways())
5233 PgArchPID = pgarch_start();
I did run the nice script "double_switchover.sh" using either
"always" or "on" on patch v1 and v2. They all generate the same
results below. In other words, whether history file
(0003.history) is archived or not depends on "archive_mode"
settings.
echo "archive_mode = always" >> ${PGDATA_NODE2}/postgresql.auto.conf
$ ls -l archive
-rw--- 1 david david 16777216 Sep 24 14:40
00010002
... ...
-rw--- 1 david david 16777216 Sep 24 14:40
0002000A
-rw--- 1 david david 41 Sep 24 14:40 0002.history
-rw--- 1 david david 83 Sep 24 14:40 0003.history
echo "archive_mode = on" >> ${PGDATA_NODE2}/postgresql.auto.conf
$ ls -l archive
-rw--- 1 david david 16777216 Sep 24 14:47
00010002
... ...
-rw--- 1 david david 16777216 Sep 24 14:47
0002000A
-rw--- 1 david david 41 Sep 24 14:47 0002.history
Personally, I prefer patch v2 since it allies to the statement
here,
https://www.postgresql.org/docs/13/warm-standby.html#CONTINUOUS-ARCHIVING-IN-STANDBY
"If archive_mode is set to on, the archiver is not enabled during
recovery or standby mode. If the standby server is promoted, it
will start archiving after the promotion, but will not archive any
WAL it did not generate itself."
By the way, I think the last part of the sentence should be changed
to something like below:
"but will not archive any WAL, which was not generated by itself."
I'm not sure if this change is an improvement or not. But if we apply
the patch I proposed, maybe we should mention also history file here.
For example, "but will not archive any WAL or timeline history files
that it did not generate itself".
make sense for me
So I included this change in the patch. Patch attached.
Regards,
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Re: libpq compression
Hello! I have noticed that psql --help lack -Z|--compression option. Also it would be nice to have option like --compression-level in psql and pgbench. On 03/30/2018 03:53 PM, Konstantin Knizhnik wrote: Hi hackers, One of our customers was managed to improve speed about 10 times by using SSL compression for the system where client and servers are located in different geographical regions and query results are very large because of JSON columns. Them actually do not need encryption, just compression. I expect that it is not the only case where compression of libpq protocol can be useful. Please notice that Postgres replication is also using libpq protocol. Taken in account that vulnerability was found in SSL compression and so SSLComppression is considered to be deprecated and insecure (http://www.postgresql-archive.org/disable-SSL-compression-td6010072.html), it will be nice to have some alternative mechanism of reducing libpq traffic. I have implemented some prototype implementation of it (patch is attached). To use zstd compression, Postgres should be configured with --with-zstd. Otherwise compression will use zlib unless it is disabled by --without-zlib option. I have added compression=on/off parameter to connection string and -Z option to psql and pgbench utilities. Below are some results: Compression ratio (raw->compressed): libz (level=1) libzstd (level=1) pgbench -i -s 10 16997209->2536330 16997209->268077 pgbench -t 10 -S 6289036->1523862 6600338<-900293 6288933->1777400 6600338<-1000318 There is no mistyping: libzstd compress COPY data about 10 times better than libz, with wonderful compression ratio 63. Influence on execution time is minimal (I have tested local configuration when client and server are at the same host): no compression libz (level=1) libzstd (level=1) pgbench -i -s 10 1.552 1.572 1.611 pgbench -t 10 -S 4.482 4.926 4.877 -- Konstantin Knizhnik Postgres Professional:http://www.postgrespro.com The Russian Postgres Company -- Grigory Smolkin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
