[COMMITTERS] pgsql: Fix error message from wal_level value renaming

[Peter Eisentraut]

Fix error message from wal_level value renaming

found by Ian Barwick

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/4dcd4da98c786c48b0dbf129c8f7ea592c34a185

Modified Files
--
src/backend/replication/slot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Michael Paquier]

On Mon, Apr 4, 2016 at 4:50 PM, Andres Freund  wrote:
> On 2016-04-04 08:44:47 +0100, Simon Riggs wrote:
>> That patch does exactly the same thing as the patch you prefer, just
>> does it differently;
>
> No, it doesn't; as explained above.

FWIW, I vote also for reverting this patch. This has been committed
without any real discussions..
-- 
Michael


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Disallow newlines in parameter values to be set in ALTER SYSTEM.

[Tom Lane]

Disallow newlines in parameter values to be set in ALTER SYSTEM.

As noted by Julian Schauder in bug #14063, the configuration-file parser
doesn't support embedded newlines in string literals.  While there might
someday be a good reason to remove that restriction, there doesn't seem
to be one right now.  However, ALTER SYSTEM SET could accept strings
containing newlines, since many of the variable-specific value-checking
routines would just see a newline as whitespace.  This led to writing a
postgresql.auto.conf file that was broken and had to be removed manually.

Pending a reason to work harder, just throw an error if someone tries this.

In passing, fix several places in the ALTER SYSTEM logic that failed to
provide an errcode() for an ereport(), and thus would falsely log the
failure as an internal XX000 error.

Back-patch to 9.4 where ALTER SYSTEM was introduced.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/99f3b5613bd1f145b5dbbe86000337bbe37fb094

Modified Files
--
src/backend/utils/misc/guc.c | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Disallow newlines in parameter values to be set in ALTER SYSTEM.

[Tom Lane]

Disallow newlines in parameter values to be set in ALTER SYSTEM.

As noted by Julian Schauder in bug #14063, the configuration-file parser
doesn't support embedded newlines in string literals.  While there might
someday be a good reason to remove that restriction, there doesn't seem
to be one right now.  However, ALTER SYSTEM SET could accept strings
containing newlines, since many of the variable-specific value-checking
routines would just see a newline as whitespace.  This led to writing a
postgresql.auto.conf file that was broken and had to be removed manually.

Pending a reason to work harder, just throw an error if someone tries this.

In passing, fix several places in the ALTER SYSTEM logic that failed to
provide an errcode() for an ereport(), and thus would falsely log the
failure as an internal XX000 error.

Back-patch to 9.4 where ALTER SYSTEM was introduced.

Branch
--
REL9_4_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/28148e25823a42877b2f2ee0961afcb8b5243a72

Modified Files
--
src/backend/utils/misc/guc.c | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Disallow newlines in parameter values to be set in ALTER SYSTEM.

[Tom Lane]

Disallow newlines in parameter values to be set in ALTER SYSTEM.

As noted by Julian Schauder in bug #14063, the configuration-file parser
doesn't support embedded newlines in string literals.  While there might
someday be a good reason to remove that restriction, there doesn't seem
to be one right now.  However, ALTER SYSTEM SET could accept strings
containing newlines, since many of the variable-specific value-checking
routines would just see a newline as whitespace.  This led to writing a
postgresql.auto.conf file that was broken and had to be removed manually.

Pending a reason to work harder, just throw an error if someone tries this.

In passing, fix several places in the ALTER SYSTEM logic that failed to
provide an errcode() for an ereport(), and thus would falsely log the
failure as an internal XX000 error.

Back-patch to 9.4 where ALTER SYSTEM was introduced.

Branch
--
REL9_5_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/f3d17491c49362b78da0c1a5b0691821dcc8c435

Modified Files
--
src/backend/utils/misc/guc.c | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Display WAL pointer in rm_redo error callback

[Alvaro Herrera]

Display WAL pointer in rm_redo error callback

This makes it easier to identify the source of a recovery problem
in case of a bug or data corruption.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/890614d2b35bd20468352043870edc7f24a7b8ec

Modified Files
--
src/backend/access/transam/xlog.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Partially revert commit 3d3bf62f30200500637b24fdb7b992a99f9704c3

[Tom Lane]

Partially revert commit 3d3bf62f30200500637b24fdb7b992a99f9704c3.

On reflection, the pre-existing logic in ANALYZE is specifically meant to
compare the frequency of a candidate MCV against the estimated frequency of
a random distinct value across the whole table.  The change to compare it
against the average frequency of values actually seen in the sample doesn't
seem very principled, and if anything it would make us less likely not more
likely to consider a value an MCV.  So revert that, but keep the aspect of
considering only nonnull values, which definitely is correct.

In passing, rename the local variables in these stanzas to
"ndistinct_table", to avoid confusion with the "ndistinct" that appears at
an outer scope in compute_scalar_stats.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/391159e03a8b69dd04a1432ceb800c7c4c3d608c

Modified Files
--
src/backend/commands/analyze.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Add a few comments about ANALYZE's strategy for collecting MCVs.

[Tom Lane]

Add a few comments about ANALYZE's strategy for collecting MCVs.

Alex Shulgin complained that the underlying strategy wasn't all that
apparent, particularly not the fact that we intentionally have two
code paths depending on whether we think the column has a limited set
of possible values or not.  Try to make it clearer.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/3c69b33f459f62fe6db66c386ef12620ea697f74

Modified Files
--
src/backend/commands/analyze.c | 32 ++--
1 file changed, 30 insertions(+), 2 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: XLogReader general code cleanup

[Alvaro Herrera]

Alvaro Herrera wrote:
> Peter Eisentraut wrote:
> > On 03/30/2016 05:57 PM, Alvaro Herrera wrote:
> > >XLogReader general code cleanup
> > 
> > This is causing compiler warnings on 32-bit environments(?):
> 
> Will fix.

Fix pushed.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Silence compiler warning

[Alvaro Herrera]

Silence compiler warning

Reported by Peter Eisentraut to occur on 32bit systems

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/c9ff752a854b687fc0a05fd4aba1066028ec5495

Modified Files
--
src/backend/access/transam/xlogreader.c | 8 
1 file changed, 4 insertions(+), 4 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Add a \gexec command to psql for evaluation of computed queries.

[Tom Lane]

Add a \gexec command to psql for evaluation of computed queries.

\gexec executes the just-entered query, like \g, but instead of printing
the results it takes each field as a SQL command to send to the server.
Computing a series of queries to be executed is a fairly common thing,
but up to now you always had to resort to kluges like writing the queries
to a file and then inputting the file.  Now it can be done with no
intermediate step.

The implementation is fairly straightforward except for its interaction
with FETCH_COUNT.  ExecQueryUsingCursor isn't capable of being called
recursively, and even if it were, its need to create a transaction
block interferes unpleasantly with the desired behavior of \gexec after
a failure of a generated query (i.e., that it can continue).  Therefore,
disable use of ExecQueryUsingCursor when doing the master \gexec query.
We can still apply it to individual generated queries, however, and there
might be some value in doing so.

While testing this feature's interaction with single-step mode, I (tgl) was
led to conclude that SendQuery needs to recognize SIGINT (cancel_pressed)
as a negative response to the single-step prompt.  Perhaps that's a
back-patchable bug fix, but for now I just included it here.

Corey Huinker, reviewed by Jim Nasby, Daniel Vérité, and myself

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/2bbe9112aec60abc2d3b4c39e75d0cbdcaaa45e1

Modified Files
--
doc/src/sgml/ref/psql-ref.sgml | 43 ++
src/bin/psql/command.c |  7 +++
src/bin/psql/common.c  | 90 --
src/bin/psql/help.c|  3 +-
src/bin/psql/settings.h|  1 +
src/bin/psql/tab-complete.c|  2 +-
src/test/regress/expected/psql.out | 45 +++
src/test/regress/sql/psql.sql  | 22 ++
8 files changed, 207 insertions(+), 6 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Introduce a LOG_SERVER_ONLY ereport level, which is never sent t

[Tom Lane]

Introduce a LOG_SERVER_ONLY ereport level, which is never sent to client.

This elevel is useful for logging audit messages and similar information
that should not be passed to the client.  It's equivalent to LOG in terms
of decisions about logging priority in the postmaster log, but messages
with this elevel will never be sent to the client.

In the current implementation, it's just an alias for the longstanding
COMMERROR elevel (or more accurately, we've made COMMERROR an alias for
this).  At some point it might be interesting to allow a LOG_ONLY flag to
be attached to any elevel, but that would be considerably more complicated,
and it's not clear there's enough use-cases to justify the extra work.
For now, let's just take the easy 90% solution.

David Steele, reviewed by Fabien Coelho, Petr Jelínek, and myself

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/66229ac0040cf1e0f5b9d72271aa9feaf3b3a37e

Modified Files
--
src/backend/utils/error/elog.c | 10 +-
src/include/utils/elog.h   | 10 ++
2 files changed, 11 insertions(+), 9 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix latent portability issue in pgwin32_dispatch_queued_signals(

[Tom Lane]

Fix latent portability issue in pgwin32_dispatch_queued_signals().

The first iteration of the signal-checking loop would compute sigmask(0)
which expands to 1<<(-1) which is undefined behavior according to the
C standard.  The lack of field reports of trouble suggest that it
evaluates to 0 on all existing Windows compilers, but that's hardly
something to rely on.  Since signal 0 isn't a queueable signal anyway,
we can just make the loop iterate from 1 instead, and save a few cycles
as well as avoiding the undefined behavior.

In passing, avoid evaluating the volatile expression UNBLOCKED_SIGNAL_QUEUE
twice in a row; there's no reason to waste cycles like that.

Noted by Aleksander Alekseev, though this isn't his proposed fix.
Back-patch to all supported branches.

Branch
--
REL9_4_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/7901a7f31fe82bbab8ad701fe04188b985c8f1ec

Modified Files
--
src/backend/port/win32/signal.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix latent portability issue in pgwin32_dispatch_queued_signals(

[Tom Lane]

Fix latent portability issue in pgwin32_dispatch_queued_signals().

The first iteration of the signal-checking loop would compute sigmask(0)
which expands to 1<<(-1) which is undefined behavior according to the
C standard.  The lack of field reports of trouble suggest that it
evaluates to 0 on all existing Windows compilers, but that's hardly
something to rely on.  Since signal 0 isn't a queueable signal anyway,
we can just make the loop iterate from 1 instead, and save a few cycles
as well as avoiding the undefined behavior.

In passing, avoid evaluating the volatile expression UNBLOCKED_SIGNAL_QUEUE
twice in a row; there's no reason to waste cycles like that.

Noted by Aleksander Alekseev, though this isn't his proposed fix.
Back-patch to all supported branches.

Branch
--
REL9_1_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/b1b6aa88b146e0e7ca9b2eb5911d1f56064ccabb

Modified Files
--
src/backend/port/win32/signal.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix latent portability issue in pgwin32_dispatch_queued_signals(

[Tom Lane]

Fix latent portability issue in pgwin32_dispatch_queued_signals().

The first iteration of the signal-checking loop would compute sigmask(0)
which expands to 1<<(-1) which is undefined behavior according to the
C standard.  The lack of field reports of trouble suggest that it
evaluates to 0 on all existing Windows compilers, but that's hardly
something to rely on.  Since signal 0 isn't a queueable signal anyway,
we can just make the loop iterate from 1 instead, and save a few cycles
as well as avoiding the undefined behavior.

In passing, avoid evaluating the volatile expression UNBLOCKED_SIGNAL_QUEUE
twice in a row; there's no reason to waste cycles like that.

Noted by Aleksander Alekseev, though this isn't his proposed fix.
Back-patch to all supported branches.

Branch
--
REL9_2_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/5496c75de7140e0fedf2fba70bd2df08743d6a64

Modified Files
--
src/backend/port/win32/signal.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix latent portability issue in pgwin32_dispatch_queued_signals(

[Tom Lane]

Fix latent portability issue in pgwin32_dispatch_queued_signals().

The first iteration of the signal-checking loop would compute sigmask(0)
which expands to 1<<(-1) which is undefined behavior according to the
C standard.  The lack of field reports of trouble suggest that it
evaluates to 0 on all existing Windows compilers, but that's hardly
something to rely on.  Since signal 0 isn't a queueable signal anyway,
we can just make the loop iterate from 1 instead, and save a few cycles
as well as avoiding the undefined behavior.

In passing, avoid evaluating the volatile expression UNBLOCKED_SIGNAL_QUEUE
twice in a row; there's no reason to waste cycles like that.

Noted by Aleksander Alekseev, though this isn't his proposed fix.
Back-patch to all supported branches.

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/58666ed28ab59a2686ee08bc648b4e9959aacfce

Modified Files
--
src/backend/port/win32/signal.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix latent portability issue in pgwin32_dispatch_queued_signals(

[Tom Lane]

Fix latent portability issue in pgwin32_dispatch_queued_signals().

The first iteration of the signal-checking loop would compute sigmask(0)
which expands to 1<<(-1) which is undefined behavior according to the
C standard.  The lack of field reports of trouble suggest that it
evaluates to 0 on all existing Windows compilers, but that's hardly
something to rely on.  Since signal 0 isn't a queueable signal anyway,
we can just make the loop iterate from 1 instead, and save a few cycles
as well as avoiding the undefined behavior.

In passing, avoid evaluating the volatile expression UNBLOCKED_SIGNAL_QUEUE
twice in a row; there's no reason to waste cycles like that.

Noted by Aleksander Alekseev, though this isn't his proposed fix.
Back-patch to all supported branches.

Branch
--
REL9_5_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/3644ac04c1ea27e596b77b7a02609b50b4742403

Modified Files
--
src/backend/port/win32/signal.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix latent portability issue in pgwin32_dispatch_queued_signals(

[Tom Lane]

Fix latent portability issue in pgwin32_dispatch_queued_signals().

The first iteration of the signal-checking loop would compute sigmask(0)
which expands to 1<<(-1) which is undefined behavior according to the
C standard.  The lack of field reports of trouble suggest that it
evaluates to 0 on all existing Windows compilers, but that's hardly
something to rely on.  Since signal 0 isn't a queueable signal anyway,
we can just make the loop iterate from 1 instead, and save a few cycles
as well as avoiding the undefined behavior.

In passing, avoid evaluating the volatile expression UNBLOCKED_SIGNAL_QUEUE
twice in a row; there's no reason to waste cycles like that.

Noted by Aleksander Alekseev, though this isn't his proposed fix.
Back-patch to all supported branches.

Branch
--
REL9_3_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/43b73d1a40463ff1fe7dee4d61a46a158629ab2b

Modified Files
--
src/backend/port/win32/signal.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix typo

[Teodor Sigaev]

Fix typo

Michael Paquier

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/eb7308d29875df773b5b52b06ed3d8b60f1b8242

Modified Files
--
contrib/bloom/blutils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: fix typo

[Teodor Sigaev]

fix typo

Andreas Ulbrich

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/9b27aebe7124210c1b0dbacac657edfefa16a006

Modified Files
--
doc/src/sgml/generic-wal.sgml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Improve estimate of distinct values in estimate_num_groups().

[Dean Rasheed]

Improve estimate of distinct values in estimate_num_groups().

When adjusting the estimate for the number of distinct values from a
rel in a grouped query to take into account the selectivity of the
rel's restrictions, use a formula that is less likely to produce
under-estimates.

The old formula simply multiplied the number of distinct values in the
rel by the restriction selectivity, which would be correct if the
restrictions were fully correlated with the grouping expressions, but
can produce significant under-estimates in cases where they are not
well correlated.

The new formula is based on the random selection probability, and so
assumes that the restrictions are not correlated with the grouping
expressions. This is guaranteed to produce larger estimates, and of
course risks over-estimating in cases where the restrictions are
correlated, but that has less severe consequences than
under-estimating, which might lead to a HashAgg that consumes an
excessive amount of memory.

This could possibly be improved upon in the future by identifying
correlated restrictions and using a hybrid of the old and new
formulae.

Author: Tomas Vondra, with some hacking be me
Reviewed-by: Mark Dilger, Alexander Korotkov, Dean Rasheed and Tom Lane
Discussion: 
http://www.postgresql.org/message-id/flat/56cd0381.5060...@2ndquadrant.com

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/84f9a35e398f863c62440d3f82fc57b4fedc5d08

Modified Files
--
src/backend/utils/adt/selfuncs.c| 64 +++--
src/test/regress/expected/subselect.out | 25 ++---
2 files changed, 64 insertions(+), 25 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Andres Freund]

On 2016-04-04 08:44:47 +0100, Simon Riggs wrote:
> On 4 April 2016 at 08:22, Andres Freund  wrote:
> > No, because in the alternative proposal we determine that the system
> > indeed has been idle since the last time a WAL record was logged.

> Why would that change anything? If something aborts without writing an
> abort record, the system can easily be idle afterwards. So an idle system
> means nothing. Your objection applies to *all* cases, not just for this
> patch and if we revert, we block all cases.

The previous suggestion only considered a system idle if there's been a
previous snapshot logged (addressing the issue at hand) and no further
WAL record, but a select few excepted, have been generated since the
last logged WAL record.

Thus e.g. such an abort would only be ignored if it never persisted it
xid anywhere (as that'd be WAL logging), in which case there cannot be
an effect for HS.

>
> I haven't ignored any messages, regrettably I read them all. I've committed
> a much simpler patch, which is what committers do.

Unless it's completely obviously an improvement, most also send a
revised version of the patch for review/comments.


> That patch does exactly the same thing as the patch you prefer, just
> does it differently;

No, it doesn't; as explained above.


Greetings,

Andres Freund


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Simon Riggs]

On 4 April 2016 at 08:22, Andres Freund  wrote:

> On 2016-04-04 08:08:32 +0100, Simon Riggs wrote:
> > For that matter, it's also important for hot standby; to deal with
> > > FATALed transactions which didn't write an abort record. It's kinda
> > > important to call StandbyReleaseOldLocks for those.  And if a standby
> is
> > > in STANDBY_SNAPSHOT_READY it's also important to see this kind of
> > > record.
> > >
> >
> > Those objections apply to all solutions to the problem so far
> > proposed,
>
> No, because in the alternative proposal we determine that the system
> indeed has been idle since the last time a WAL record was logged.


Why would that change anything? If something aborts without writing an
abort record, the system can easily be idle afterwards. So an idle system
means nothing. Your objection applies to *all* cases, not just for this
patch and if we revert, we block all cases.


> And
> only a few select records are exempt from being considered idle; every
> other type of record causes the system to be considered non-idle.


> likely any solution. My understanding was that those issues are considered
> > the lesser of the two evils. I'm happy to revert this patch, as long as
> we
> > agree that it also blocks all further "bug fixes" so far proposed based
> > upon those arguments.
>
> Yes, please do.
>
> > > Additionally, we're now logging more WAL if archive timeout isn't
> > > configured, which doesn't strike me as a good idea.
>
> (to clarify, I'm comparing a configured XLogArchiveTimeout to none, not
> the before/after patch state)
>
> > That's not true either...
>
> +   if (running->xcnt == 0 &&
> +   nlocks == 0 &&
> +   XLogArchiveTimeout > 0 &&
> +   !last_snapshot_overflowed)
> +   {
> +   LWLockRelease(XidGenLock);
> +   return InvalidXLogRecPtr;
> +   }
>
> How can XLogArchiveTimeout not imply a behavioural difference here?


The bug appears when we have archive_timeout set; the bug fix applies in
that case also.


> > Thanks for your comments, but you seem to be misreading the patch with
> > respect to the if clause and new brackets.
>
> I indeed missed the outer if; which addresses the logical issue. So the
> problem there isn't fixed at all.
>
> You ignored a good number of messages and just committed a patch with a
> different approach without any further discussion. So it shouldn't be
> particularly surprising that people aren't willing to look super careful
> at what you did.
>

I haven't ignored any messages, regrettably I read them all. I've committed
a much simpler patch, which is what committers do.  That patch does exactly
the same thing as the patch you prefer, just does it differently; I'm sorry
if that annoys you. If you have time for debate, please read the patch and
lets have an even-handed and rational debate. If you don't have time to
review the patch, lets skip the debate.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Andres Freund]

On 2016-04-04 08:08:32 +0100, Simon Riggs wrote:
> For that matter, it's also important for hot standby; to deal with
> > FATALed transactions which didn't write an abort record. It's kinda
> > important to call StandbyReleaseOldLocks for those.  And if a standby is
> > in STANDBY_SNAPSHOT_READY it's also important to see this kind of
> > record.
> >
> 
> Those objections apply to all solutions to the problem so far
> proposed,

No, because in the alternative proposal we determine that the system
indeed has been idle since the last time a WAL record was logged. And
only a few select records are exempt from being considered idle; every
other type of record causes the system to be considered non-idle.


> likely any solution. My understanding was that those issues are considered
> the lesser of the two evils. I'm happy to revert this patch, as long as we
> agree that it also blocks all further "bug fixes" so far proposed based
> upon those arguments.

Yes, please do.

> > Additionally, we're now logging more WAL if archive timeout isn't
> > configured, which doesn't strike me as a good idea.

(to clarify, I'm comparing a configured XLogArchiveTimeout to none, not
the before/after patch state)

> That's not true either...

+   if (running->xcnt == 0 &&
+   nlocks == 0 &&
+   XLogArchiveTimeout > 0 &&
+   !last_snapshot_overflowed)
+   {
+   LWLockRelease(XidGenLock);
+   return InvalidXLogRecPtr;
+   }

How can XLogArchiveTimeout not imply a behavioural difference here?


> Thanks for your comments, but you seem to be misreading the patch with
> respect to the if clause and new brackets.

I indeed missed the outer if; which addresses the logical issue. So the
problem there isn't fixed at all.

You ignored a good number of messages and just committed a patch with a
different approach without any further discussion. So it shouldn't be
particularly surprising that people aren't willing to look super careful
at what you did.


Greetings,


Andres Freund


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Simon Riggs]

On 4 April 2016 at 07:49, Andres Freund  wrote:


> > It doesn't?
>

Nope, clearly in the code 2 lines above.

For that matter, it's also important for hot standby; to deal with
> FATALed transactions which didn't write an abort record. It's kinda
> important to call StandbyReleaseOldLocks for those.  And if a standby is
> in STANDBY_SNAPSHOT_READY it's also important to see this kind of
> record.
>

Those objections apply to all solutions to the problem so far proposed,
likely any solution. My understanding was that those issues are considered
the lesser of the two evils. I'm happy to revert this patch, as long as we
agree that it also blocks all further "bug fixes" so far proposed based
upon those arguments.


> Additionally, we're now logging more WAL if archive timeout isn't
> configured, which doesn't strike me as a good idea.
>

That's not true either...

Thanks for your comments, but you seem to be misreading the patch with
respect to the if clause and new brackets.

I suggest we give this some hours of thought before discussing it further.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Andres Freund]

On 2016-04-04 08:42:02 +0200, Andres Freund wrote:
> On 2016-04-04 07:31:46 +0100, Simon Riggs wrote:
> > On 4 April 2016 at 07:24, Andres Freund  wrote:
> > 
> > > On 2016-04-04 06:19:04 +, Simon Riggs wrote:
> > > > Avoid archiving XLOG_RUNNING_XACTS on idle server
> > > >
> > > > If archive_timeout > 0 we should avoid logging XLOG_RUNNING_XACTS if
> > > idle.
> > > >
> > > > Bug 13685 reported by Laurence Rowe, investigated in detail by Michael
> > > Paquier,
> > > > though this is not his proposed fix.
> > > > 20151016203031.3019.72...@wrigleys.postgresql.org
> > > >
> > > > Simple non-invasive patch to allow later backpatch to 9.4 and 9.5
> > >
> > > Uh. This is wrong.
> > 
> > 
> > For one it breaks cleanup with logical decoding which
> > > does *NEED* to know that nothing is happening. Although only once, not
> > > repeatedly.
> > >
> > 
> > If the patch did that, I agree it would be wrong. It doesn't,
> > deliberately.
> 
> It doesn't?
> 
> +   if (running->xcnt == 0 &&
> +   nlocks == 0 &&
> +   XLogArchiveTimeout > 0 &&
> +   !last_snapshot_overflowed)
> +   {
> +   LWLockRelease(XidGenLock);
> +   return InvalidXLogRecPtr;
> +   }
> 
> Overflowed snapshots aren't a problem for logical decoding. But we need
> to know that no old transactions are running occasionally.

For that matter, it's also important for hot standby; to deal with
FATALed transactions which didn't write an abort record. It's kinda
important to call StandbyReleaseOldLocks for those.  And if a standby is
in STANDBY_SNAPSHOT_READY it's also important to see this kind of
record.

Additionally, we're now logging more WAL if archive timeout isn't
configured, which doesn't strike me as a good idea.

Andres


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Andres Freund]

On 2016-04-04 07:31:46 +0100, Simon Riggs wrote:
> On 4 April 2016 at 07:24, Andres Freund  wrote:
> 
> > On 2016-04-04 06:19:04 +, Simon Riggs wrote:
> > > Avoid archiving XLOG_RUNNING_XACTS on idle server
> > >
> > > If archive_timeout > 0 we should avoid logging XLOG_RUNNING_XACTS if
> > idle.
> > >
> > > Bug 13685 reported by Laurence Rowe, investigated in detail by Michael
> > Paquier,
> > > though this is not his proposed fix.
> > > 20151016203031.3019.72...@wrigleys.postgresql.org
> > >
> > > Simple non-invasive patch to allow later backpatch to 9.4 and 9.5
> >
> > Uh. This is wrong.
> 
> 
> For one it breaks cleanup with logical decoding which
> > does *NEED* to know that nothing is happening. Although only once, not
> > repeatedly.
> >
> 
> If the patch did that, I agree it would be wrong. It doesn't,
> deliberately.

It doesn't?

+   if (running->xcnt == 0 &&
+   nlocks == 0 &&
+   XLogArchiveTimeout > 0 &&
+   !last_snapshot_overflowed)
+   {
+   LWLockRelease(XidGenLock);
+   return InvalidXLogRecPtr;
+   }

Overflowed snapshots aren't a problem for logical decoding. But we need
to know that no old transactions are running occasionally.

Andres


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Simon Riggs]

On 4 April 2016 at 07:24, Andres Freund  wrote:

> On 2016-04-04 06:19:04 +, Simon Riggs wrote:
> > Avoid archiving XLOG_RUNNING_XACTS on idle server
> >
> > If archive_timeout > 0 we should avoid logging XLOG_RUNNING_XACTS if
> idle.
> >
> > Bug 13685 reported by Laurence Rowe, investigated in detail by Michael
> Paquier,
> > though this is not his proposed fix.
> > 20151016203031.3019.72...@wrigleys.postgresql.org
> >
> > Simple non-invasive patch to allow later backpatch to 9.4 and 9.5
>
> Uh. This is wrong.


For one it breaks cleanup with logical decoding which
> does *NEED* to know that nothing is happening. Although only once, not
> repeatedly.
>

If the patch did that, I agree it would be wrong. It doesn't, deliberately.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Michael Paquier]

On Mon, Apr 4, 2016 at 3:24 PM, Andres Freund  wrote:
> On 2016-04-04 06:19:04 +, Simon Riggs wrote:
>> Avoid archiving XLOG_RUNNING_XACTS on idle server
>>
>> If archive_timeout > 0 we should avoid logging XLOG_RUNNING_XACTS if idle.
>>
>> Bug 13685 reported by Laurence Rowe, investigated in detail by Michael 
>> Paquier,
>> though this is not his proposed fix.
>> 20151016203031.3019.72...@wrigleys.postgresql.org
>>
>> Simple non-invasive patch to allow later backpatch to 9.4 and 9.5

Well...

> Uh. This is wrong. For one it breaks cleanup with logical decoding which
> does *NEED* to know that nothing is happening. Although only once, not 
> repeatedly.

On top of that, it does not interact with the snapshots logged by
other backends. We may want something in shared memory instead to
control the whole facility. Nor does this commit fix the checkpoint
skip logic.
-- 
Michael


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Andres Freund]

On 2016-04-04 06:19:04 +, Simon Riggs wrote:
> Avoid archiving XLOG_RUNNING_XACTS on idle server
> 
> If archive_timeout > 0 we should avoid logging XLOG_RUNNING_XACTS if idle.
> 
> Bug 13685 reported by Laurence Rowe, investigated in detail by Michael 
> Paquier,
> though this is not his proposed fix.
> 20151016203031.3019.72...@wrigleys.postgresql.org
> 
> Simple non-invasive patch to allow later backpatch to 9.4 and 9.5

Uh. This is wrong. For one it breaks cleanup with logical decoding which
does *NEED* to know that nothing is happening. Although only once, not 
repeatedly.

Andres


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Avoid archiving XLOG_RUNNING_XACTS on idle server

[Simon Riggs]

Avoid archiving XLOG_RUNNING_XACTS on idle server

If archive_timeout > 0 we should avoid logging XLOG_RUNNING_XACTS if idle.

Bug 13685 reported by Laurence Rowe, investigated in detail by Michael Paquier,
though this is not his proposed fix.
20151016203031.3019.72...@wrigleys.postgresql.org

Simple non-invasive patch to allow later backpatch to 9.4 and 9.5

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/bf08f2292ffca14fd133aa0901d1563b6ecd6894

Modified Files
--
src/backend/postmaster/bgwriter.c |  5 -
src/backend/storage/ipc/standby.c | 21 +
2 files changed, 25 insertions(+), 1 deletion(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers