Re: [HACKERS] pg_dump broken for non-super user

[Rushabh Lathia]

On Tue, May 3, 2016 at 8:34 PM, Stephen Frost  wrote:

> * Rushabh Lathia (rushabh.lat...@gmail.com) wrote:
> > With commit a9f0e8e5a2e779a888988cb64479a6723f668c84, now pg_dump, use a
> > bitmap
> > to represent what to include. With this commit if non-super user is
> unable
> > to perform the dump.
> [...]
> > pg_dump: [archiver (db)] query was: LOCK TABLE pg_catalog.pg_authid IN
> > ACCESS SHARE MODE
>
> This does get addressed, in a way, in one of the patches that I've
> currently got pending for pg_dump.  That particular change is actually
> one for performance and therefore it's only going to help in cases where
> none of the catalog tables have had their ACLs changed.
>
> If the ACL has changed for any catalog table then pg_dump will still try
> to LOCK the table, because we're going to dump out information about
> that table (the ACLs on it).  I'm not sure if that's really an issue or
> not..  Generally, if you're using pg_dump as a non-superuser, I'd expect
> you to be limiting the tables you're dumping to ones you have access to
> normally (using -n).
>

If its limitation that if someone is using pg_dump as a non-superuser, it
should be limiting the tables using -n, then better we document that. But
I don't think this is valid limitation. Comments ?


>
> > getTables() take read-lock target tables to make sure they aren't DROPPED
> > or altered in schema before we get around to dumping them. Here it having
> > below condition to take  a lock:
> >
> > if (tblinfo[i].dobj.dump && tblinfo[i].relkind ==
> RELKIND_RELATION)
> >
> > which need to replace with:
> >
> > if ((tblinfo[i].dobj.dump & DUMP_COMPONENT_DEFINITION) &&
> > tblinfo[i].relkind == RELKIND_RELATION)
> >
> > PFA patch to fix the issue.
>
> I don't think we want to limit the cases where we take a lock to just
> when we're dumping out the table definition..  Consider what happens if
> someone drops the table before we get to dumping out the data in the
> table, or gathering the ACLs on it (which happens much, much later).
>

Right, condition should check for (DUMP_COMPONENT_DEFINITION ||
DUMP_COMPONENT_DATA).

I am confused, in getTables() we executing LOCK TABLE, which holds the
share lock on that table till we get around to dumping them ( and that
include
dumping data or gathering the ACLs).  isn't that right ?



>
> Thanks!
>
> Stephen
>



-- 
Rushabh Lathia

Re: [HACKERS] Make PG's "NOT NULL"s and attnotnull ("is_nullable") conform to SQL-2011

[Vitaly Burovoy]

On 5/3/16, Tom Lane  wrote:
> Vitaly Burovoy  writes:
>> On 4/27/16, Alvaro Herrera  wrote:
>>> Point 2 is where things differ from what I remember; my (possibly
>>> flawed) understanding was that there's no difference between those
>>> things.  Many (maybe all) of the things from this point on are probably
>>> fallout from that one change.
>
>> It is just mentioning that CHECK constraints have influence on
>> nullability characteristic, but it differs from NNC.
>> NNC creates CHECK constraint, but not vice versa. You can create
>> several CHECK "col IS NOT NULL" constraints, but only one NNC (several
>> ones by inheritance only?). And DROP NOT NULL should drop only those
>> CHECK that is linked with NNC (and inherited), but no more (full
>> explanation is in my initial letter).
>
> This seems to me to be a most curious reading of the standard.
> SQL:2011 11.4  syntax rule 17a says
>
>If a  is specified that contains
>the  NOT NULL, then it is equivalent to the
>following :
>
>   CND CHECK ( C IS NOT NULL ) CA
>
> As a rule, when the SQL spec says "equivalent", they do not mean "it's
> sort of like this", they mean the effects are indistinguishable.  In
> particular, I see nothing whatsoever saying that you're not allowed to
> write more than one per column.

1. SQL:2011 4.13 :

 — If C is a column of a base table, then an indication of whether it is
 defined as NOT NULL and, if so, the constraint name of the associated 
table
 constraint definition.
 NOTE 41 — This indication and the associated constraint name 
exist for
 definitional purposes only and are not exposed through the 
COLUMNS view
 in the Information Schema.

There is only "constraint name", not "constraint names".

2. SQL:2011 11.15   General Rule 1:

... If the column descriptor of C does not contain an indication that
C is defined as NOT NULL, then:

And there is no rule 2. I.e. if the column is already set as NOT NULL
you can't specify it as NOT NULL again.

3. SQL:2011 11.15   General Rule 1.d:

 The following  is executed without further
Access Rule checking:
 ALTER TABLE TN ADD CONSTRAINT IDCN CHECK ( CN IS NOT NULL )


> So I don't like the proposal to add an attnotnullid column to
> pg_attribute.

Why and where to place it?

> What we'd talked about earlier was converting attnotnull
> into, effectively, a hint flag saying that there's at least one NOT NULL
> constraint attached to the column.  That still seems like a good approach
> to me.

Ok. But not only NOT NULL constraint, but also non-deferrable PK,
CHECK, domains, may be the strictest FK.

> When we're actually ready to throw an error for a null value,
> we could root through the table's constraint list for a not-null
> constraint name to report.

attnotnullid is not for reporting, it is for DROP NOT NULL and
recreating "CREATE TABLE" statements via pg_dump.

>  It doesn't matter which one we select, because
> constraint application order has never been promised to be deterministic;
> and a few extra cycles at that point don't seem like a big problem to me.
>
>   regards, tom lane

-- 
Best regards,
Vitaly Burovoy


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

Re: [HACKERS] Make PG's "NOT NULL"s and attnotnull ("is_nullable") conform to SQL-2011

[David G. Johnston]

On Monday, February 8, 2016, Vitaly Burovoy 
wrote:

>
> 12. At the same time in (subcl. 4.13) mentioned there can be "at least
> one NNC" (may be via inheritance?).
>
>
This is a bit hard to reason about given that our implementation of
inheritance is non-standard.

Are we close to the standard semantics with regard to this particular
dynamic?

David J.

Re: [HACKERS] Apparent race condition in standby promotion

[Noah Misch]

On Mon, Apr 25, 2016 at 02:09:27PM -0400, Tom Lane wrote:
> I'm looking at buildfarm member tern's recent failure:
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tern&dt=2016-04-25%2001%3A08%3A08

> # Running: pg_ctl -D 
> /home/nm/farm/gcc32/HEAD/pgsql.build/src/bin/pg_rewind/tmp_check/data_standby_fTzy/pgdata
>  -l 
> /home/nm/farm/gcc32/HEAD/pgsql.build/src/bin/pg_rewind/tmp_check/log/001_basic_standby.log
>  promote
> server promoting
> # 
> Timed out while waiting for promotion of standby at RewindTest.pm line 166.
> 
> But look at what's in the standby's log:
> 
> LOG:  database system was interrupted; last known up at 2016-04-25 01:58:39 
> UTC
> LOG:  entering standby mode
> LOG:  redo starts at 0/228
> LOG:  consistent recovery state reached at 0/2000C00
> LOG:  database system is ready to accept read only connections
> LOG:  started streaming WAL from primary at 0/300 on timeline 1
> LOG:  statement: SELECT NOT pg_is_in_recovery()
> ... 88 occurrences of that removed ...
> LOG:  statement: SELECT NOT pg_is_in_recovery()
> LOG:  received promote request
> FATAL:  terminating walreceiver process due to administrator command
> 
> The standby server didn't notice the promote request until AFTER
> the perl script had probed 90 times, once a second, for promotion
> to finish.  What's up with that?

This resembles symptoms I studied last September.

> The other theory is that the startup process received the SIGUSR2
> but failed to act on it for a long time.  It checks for that only
> in CheckForStandbyTrigger(), and the calls to that function are
> in assorted rather unprincipled places, which makes it hard to
> convince oneself that such a call will always happen soon.

Right.  In every case, I caught the startup process taking too long in a
single XLOG_DBASE_CREATE replay.  I don't suspect a bug; that is a costly
record to replay, especially given the machine's slow filesystem metadata
operations.  I could reproduce such symptoms reliably, on any system, with the
attached demonstration patch.  While we could hack things until the message
prints earlier, that won't make promotion finish earlier.

> We've seen previous examples of the same thing, eg
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer&dt=2016-02-09%2019%3A16%3A09
> Noah raised the test's timeout from 30 seconds to 90 in response
> to that failure, but it looks to me like this was a mis-diagnosis
> of the problem.

I can understand you guessing the 2016-02-09 failure inspired that change, but
it was either a coincidence or merely reminded me to commit an already-written
change.  I had performed burn-in runs, with timeouts raised to 900s, totalling
25177 executions of src/bin/pg_rewind/t/002_databases.pl.  The longest promote
duration among those runs was 43s, so I doubled that quantity and rounded up.

For the 2016-04-25 failure, note this bit of 001_basic_master.log:

LOG:  statement: CHECKPOINT
LOG:  statement: SELECT pg_current_xlog_location() = write_location FROM 
pg_stat_replication WHERE application_name = 'rewind_standby';
... 39 duplicate lines removed ...
LOG:  statement: SELECT pg_current_xlog_location() = write_location FROM 
pg_stat_replication WHERE application_name = 'rewind_standby';
LOG:  received immediate shutdown request

That's 41s just to copy WAL.  The slowest of those 002_databases.pl runs used
7s, and the worst I've seen in 2579 runs of 001_basic.pl is 22s.  The failed
run likely coincided with exceptional system load.

If this happens again anytime soon, I should triple (or so) that 90s timeout.
diff --git a/src/backend/commands/dbcommands.c 
b/src/backend/commands/dbcommands.c
index 6cbe65e..8135a64 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -44,6 +44,7 @@
 #include "commands/defrem.h"
 #include "commands/seclabel.h"
 #include "commands/tablespace.h"
+#include "libpq/pqsignal.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -2085,6 +2086,11 @@ dbase_redo(XLogReaderState *record)
 * We don't need to copy subdirectories
 */
copydir(src_path, dst_path, false);
+
+   /* Pause to tickle timeouts sensitive to recovery duration. */
+   PG_SETMASK(&BlockSig);
+   pg_usleep(91 * 100L);
+   PG_SETMASK(&UnBlockSig);
}
else if (info == XLOG_DBASE_DROP)
{

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

Re: [HACKERS] text search changes vs. binary upgrade

[Noah Misch]

On Tue, May 03, 2016 at 11:13:54PM -0400, Tom Lane wrote:
> Noah Misch  writes:
> > Commit bb14050 said:
> > - change order for tsquery, so, users, who has a btree index over 
> > tsquery,
> >   should reindex it
> 
> We undid that in 1ec4c7c05, no?

Ah, looks that way.

> > Commit 61d66c4 may or may not warrant pg_upgrade treatment:
> > Fix support of digits in email/hostnames.
> 
> The general theory about changes in text search parser and dictionary
> behavior has always been that a reindex is not required, because that does
> not invalidate the derived data in the same sort of way that changing,
> say, btree sort order of a datatype would.  At worst, searches for the
> specifically affected words might fail to find relevant entries because
> to_tsvector now produces a different list of lexemes than before (and
> those new lexemes are not in the index, the old ones are).  If the
> affected set of words is sufficiently large and relevant to her use-case,
> a user might judge that rebuilding derived tsvector data is worth her
> trouble.  But I am dubious that pg_upgrade should issue guidance
> unconditionally telling people to do it.  Most people probably aren't
> going to have any noticeable amount of data that's affected by this change.
> 
> If we did worry about this for 61d66c4, then for example the unaccent
> changes would also be problematic, and probably the ispell changes too.
> I'm inclined to just group all those things in the release notes and
> provide text counseling users to think about how much those changes affect
> their full-text data and whether rebuilding derived tsvectors would be
> worth it.

Fair.


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

Re: [HACKERS] Make PG's "NOT NULL"s and attnotnull ("is_nullable") conform to SQL-2011

[David G. Johnston]

Quick flyby here...

On Tuesday, May 3, 2016, Tom Lane  wrote:

> Vitaly Burovoy > writes:
> > On 4/27/16, Alvaro Herrera >
> wrote:
> >> Point 2 is where things differ from what I remember; my (possibly
> >> flawed) understanding was that there's no difference between those
> >> things.  Many (maybe all) of the things from this point on are probably
> >> fallout from that one change.
>
> > It is just mentioning that CHECK constraints have influence on
> > nullability characteristic, but it differs from NNC.
> > NNC creates CHECK constraint, but not vice versa. You can create
> > several CHECK "col IS NOT NULL" constraints, but only one NNC (several
> > ones by inheritance only?). And DROP NOT NULL should drop only those
> > CHECK that is linked with NNC (and inherited), but no more (full
> > explanation is in my initial letter).


Either it's one, or it's not...


> This seems to me to be a most curious reading of the standard.
> SQL:2011 11.4  syntax rule 17a says
>
>  If a  is specified that contains
>  the  NOT NULL, then it is equivalent to the
>  following :
>
> CND CHECK ( C IS NOT NULL ) CA
>
> As a rule, when the SQL spec says "equivalent", they do not mean "it's
> sort of like this", they mean the effects are indistinguishable.  In
> particular, I see nothing whatsoever saying that you're not allowed to
> write more than one per column.


Does it define how DROP NOT NULL is supposed to behave?

I agree that the behavior of a column NNC is identical to a similar
constraint defined on the table: but if drop not null doesn't impact table
constraints then the concept of perfect equality is already lost.


> So I don't like the proposal to add an attnotnullid column to
> pg_attribute.  What we'd talked about earlier was converting attnotnull
> into, effectively, a hint flag saying that there's at least one NOT NULL
> constraint attached to the column.


>
Have we considered making it a table constraint and giving it a name?  We
already handle that case without difficulty.

Not looking for a detailed explanation.

David J.

Re: [HACKERS] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Fabien COELHO]

Hello Andres,


An enum doesn't have a benefit for a bitmask imo - you can't "legally"
use it as a type for functions accepting the bitmask.


I do not understand. I suggested to use enum to enumerate the bitmask
constants, ISTM that it does not preclude to use it as a bitmask as you do,
it is just a replacement of the #define? The type constraint on the enum
does not disallow bitmasking values, I checked with both gcc & clang.


There's not really a point in using an enum if you use neither the type 
(which you shouldn't because if you or the bitmask value you have types 
outside the range of the enum), nor the autogenerated numbers.


I do not think that there is such a constraint in C, you can use the enum 
bitfield type, so you should. You can say in the type name that it is a 
bitmask to make it clearer:


  typedef enum {
EXTENSION_XXX = ...;
  } extension_behavior_bitfield;


Anyway seems fairly unimportant.


Sure, it is just cosmetic. Now the type is already an enum and you can 
keep it that way, ISTM that it is cleaner to avoid defines, so I think you 
should.



Then should it be _OPEN_INACTIVE[TED] or _OPEN_TRUNCATED rather than
_OPEN_DELETED, which is contradictory?


Well, TRUNCATED doesn't entirely work, there's I think some cases where
this currently also applies to deleted relations. I kind of like the
unintuitive sounding name, because it's really a dangerous options (any
further mdnblocks will be wrong).


Hmmm.

My issue is with the semantics of the name which implies how it can be 
understand by someone reading the code. The interface deals with files, so 
implicitely DELETED refers to files, and AFAICS no file was deleted. Maybe 
rows in the relation where deleted somehow, or entries in an index, but 
that has no sense at the API level. So I think you should not use DELETED.


I can see why a vaguely oxymoronic name is amusing, but I do not think 
that it conveys any idea of "dangerous" as you suggest.


Now the important think is probably not that the code is the cleanest 
possible one, but that it fixes the issue, so you should probably commit 
something.


--
Fabien.


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

Re: [HACKERS] Make PG's "NOT NULL"s and attnotnull ("is_nullable") conform to SQL-2011

[Tom Lane]

Vitaly Burovoy  writes:
> On 4/27/16, Alvaro Herrera  wrote:
>> Point 2 is where things differ from what I remember; my (possibly
>> flawed) understanding was that there's no difference between those
>> things.  Many (maybe all) of the things from this point on are probably
>> fallout from that one change.

> It is just mentioning that CHECK constraints have influence on
> nullability characteristic, but it differs from NNC.
> NNC creates CHECK constraint, but not vice versa. You can create
> several CHECK "col IS NOT NULL" constraints, but only one NNC (several
> ones by inheritance only?). And DROP NOT NULL should drop only those
> CHECK that is linked with NNC (and inherited), but no more (full
> explanation is in my initial letter).

This seems to me to be a most curious reading of the standard.
SQL:2011 11.4  syntax rule 17a says

 If a  is specified that contains
 the  NOT NULL, then it is equivalent to the
 following :

CND CHECK ( C IS NOT NULL ) CA

As a rule, when the SQL spec says "equivalent", they do not mean "it's
sort of like this", they mean the effects are indistinguishable.  In
particular, I see nothing whatsoever saying that you're not allowed to
write more than one per column.

So I don't like the proposal to add an attnotnullid column to
pg_attribute.  What we'd talked about earlier was converting attnotnull
into, effectively, a hint flag saying that there's at least one NOT NULL
constraint attached to the column.  That still seems like a good approach
to me.  When we're actually ready to throw an error for a null value,
we could root through the table's constraint list for a not-null
constraint name to report.  It doesn't matter which one we select, because
constraint application order has never been promised to be deterministic;
and a few extra cycles at that point don't seem like a big problem to me.

regards, tom lane


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

Re: [HACKERS] what to revert

[Euler Taveira]

On 03-05-2016 20:25, Craig Ringer wrote:
> On 4 May 2016 at 01:12, Peter Geoghegan  > wrote:
> 
> On Tue, May 3, 2016 at 9:58 AM, Alvaro Herrera
> mailto:alvhe...@2ndquadrant.com>> wrote:
> > As its committer, I tend to agree about reverting that feature.  Craig
> > was just posting some more patches, and I have the pg_recvlogical
> > changes here (--endpos) which after some testing are not quite looking
> > ready to go -- plus we still have to write the actual Perl test scripts
> > that would use it.  Taken together, this is now looking to me a bit
> > rushed, so I prefer to cut my losses here and revert the patch so that
> > we can revisit it for 9.7.
> 
> I think it's a positive development that we can take this attitude to
> reverting patches. It should not be seen as a big personal failure,
> because it isn't. Stigmatizing reverts incentivizes behavior that
> leads to bad outcomes.
> 
> 
> Indeed. Being scared to revert also makes the barrier to committing
> something and getting it into more hands, for longer, under a variety of
> different conditions higher. Not a ton of help with this particular
> feature but quite important with others.
> 
> While I'm personally disappointed by this outcome, I also can't really
> disagree with it. The whole area is a bit of a mess and hard to work on,
> and as demonstrated my understanding of it when I wrote the original
> patch was incomplete. We could commit the rewritten function, but ...
> rewriting a feature just before beta1 probably says it's just not baked
> yet, right?
> 
You said that in another thread...

> The upside of all this is that now I have a decent picture of how it
> should all look and how timeline following, failover capability etc can
> be introduced in a staged way. I'd also like to get rid of the use of
> various globals to pass timeline information "around" the walsender and
> share more of the logic between the code paths.
> 
Question is: is the actual code so useless that it can't even be a 1.0
release? A lot of (complex) features were introduced with the notion
that will be improved in the next version. However, if the code is buggy
or there are serious API problems, revert them.


-- 
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


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

Re: [HACKERS] what to revert

[Tom Lane]

Amit Kapila  writes:
> Yes, that would be a way forward for 9.6 if we are not able to close
> blocking open items before beta1.  However, I think it would be bad if we
> miss some of the below listed important features like snapshot_too_old or
> atomic pin/unpin for 9.6.  Can we consider to postpone beta1, so that the
> patch authors get time to resolve blocking issues?

This was already considered and rejected by the release team.  Most of
the patches in question went in very close to the feature freeze deadline
(all but one, in fact, in the last week) and there is every reason to
suspect that they were rushed rather than really being ready to commit.
We should not allow an entire year's worth of work to slide in the
possibly-vain hope that these few patches can get fixed up if they're
given more time.

The bigger picture here is that we'd all like to get back to development
sooner rather than later.  The longer it takes to stabilize 9.6, the
longer it will be before the tree reopens for new development.  That
consideration should make us very willing to revert problematic changes
and let their authors try again in the next cycle.

regards, tom lane


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

Re: [HACKERS] Make PG's "NOT NULL"s and attnotnull ("is_nullable") conform to SQL-2011

[Vitaly Burovoy]

I'm sorry for the late answer.

On 4/27/16, Alvaro Herrera  wrote:
> Vitaly Burovoy wrote:
>
> Hi,
>
>> But before starting working on it I had a look at the SQL-2011
>> standard (ISO/IEC 9075-2)[3] and found that:
>>
>> 1. A name for a "NOT NULL" constraint  can be given by a table
>> definition (subcl. 11.4, "Format"->"column constraint definition").
>> 2. The standard splits NNC and CHECK constraints (subcl. 11.4,
>> "Format"-> "column constraint")
>
> Point 2 is where things differ from what I remember; my (possibly
> flawed) understanding was that there's no difference between those
> things.  Many (maybe all) of the things from this point on are probably
> fallout from that one change.

It is just mentioning that CHECK constraints have influence on
nullability characteristic, but it differs from NNC.
NNC creates CHECK constraint, but not vice versa. You can create
several CHECK "col IS NOT NULL" constraints, but only one NNC (several
ones by inheritance only?). And DROP NOT NULL should drop only those
CHECK that is linked with NNC (and inherited), but no more (full
explanation is in my initial letter).

>> III. "pg_attribute" table should have an "attnotnullid oid" as an
>> indicator of "NOT NULL" (p.4) and points to a CHECK constraint; It is
>> in addition to a "Nullability characteristic" "attnotnull" (p.3).
>> IV. "pg_constraint" should have a column "connotnullkey int2[]" as a
>> "list of the nullable columns" which references to
>> "pg_attribute.attnum" for fast checking whether a column is still
>> nullable after deleting/updating constraints or not. Array is
>> necessary for cases like "CHECK ((col1 IS NOT NULL) AND (col2 IS NOT
>> NULL))" and for nondeferrable PKs.
>
> I think these points warrant some more consideration. I don't like the
> idea that pg_attribute and pg_constraint are both getting considerably
> bloated to support this.

Ok, I'm ready for a discussion.

Two additional columns are necessary: one for pointing to an
underlying CHECK constraint (or boolean column whether current CHECK
is NNC or not) and second for fast computation of "attnotnull" (which
means "nullable characteristic") and ability to skip check if
"attnotnull" is set but not triggered (I think it'll improve
performance for inherited tables).

I think placing the first column (attnotnullid) to pg_attribute is
better because you can't have more than one value in it.

The second is obviously should be placed in pg_constraint.

>> P.S.:
>> Since the SQL standard defines that "col NOT NULL" as an equivalent to
>> "CHECK (col IS NOT NULL)" (p.8) what to do with that behavior:
>>
>> postgres=# create type t as (x int);
>> CREATE TYPE
>> postgres=# SELECT v, v IS NOT NULL AS should_be_in_table FROM
>> (VALUES('(1)'::t),('()'),(NULL)) AS x(v);
>>   v  | should_be_in_table
>> -+
>>  (1) | t
>>  ()  | f
>>  | f
>> (3 rows)
>>
>> "attnotnull" in such case is stricter, like "CHECK (col IS DISTINCT FROM
>> NULL)".
>>
>> Should such values (with NULL in each attribute of a composite type)
>> violate NOT NULL constraints?
>
> I wonder if the standard has a concept of null composite values.  If
> not, then there is no difference between IS NOT NULL and IS DISTINCT
> FROM NULL, which explains why they define NNC in terms of the former.

Yes, it has. The PG's composite type is "Row types" (subcl.4.8) in the standard.

The standard also differentiates IS [NOT] NULL and IS [NOT] DISTINCT FROM:

>>> Subcl. 8.8 :
>>> ...
>>> 1) Let R be the  and let V be the value of R.
>>> 2) Case:
>>>  a) If V is the null value, then “R IS NULL” is True and
>>>   the value of “R IS NOT NULL” is False.
>>>  b) Otherwise:
>>>   i) The value of “R IS NULL” is
>>>Case:
>>>1) If the value of every field of V is the null value, then True.
>>>2) Otherwise, False.
>>> ...
>>>
>>> Subcl. 8.15 
>>> ...
>>> 1) Let V1 be the value of  and let V2 be the value 
>>> of .
>>> ...
>>>  b) If V1 is the null value and V2 is not the null value, or if V1 is not 
>>> the null value and V2 is the null
>>> value, then the result is True.
>>> ...

In subcl.8.8 "each column" is mentioned, in 8.15 if one of value is
the null value and the other is not then nothing more is checked and
True is returned.

> I think your email was too hard to read because of excessive density,
> which would explain the complete lack of response.

Hmm. I decided it was "silently approved". =)

> I haven't had the chance to work on this topic again, but I encourage you to,
> if you have the resources.

Thank you, I think I'll find a time for it no earlier than the summer.

> (TBH I haven't had the chance to study your proposed design in detail, 
> either).

I hope somebody find a time to study it before someone sends a proposal.

-- 
Best regards,
Vitaly Burovoy


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

Re: [HACKERS] what to revert

[Amit Kapila]

On Tue, May 3, 2016 at 9:28 PM, Robert Haas  wrote:
>
> On Tue, May 3, 2016 at 11:36 AM, Tom Lane  wrote:
> >> There are a lot more than 2 patchsets that are busted at the moment,
> >> unfortunately, but I assume you are referring to "snapshot too old"
> >> and "Use Foreign Key relationships to infer multi-column join
> >> selectivity".
> >
> > Yeah, those are the ones I'm thinking of.  I've not heard serious
> > proposals to revert any others, have you?
>
> Here's a list of what I think is currently broken in 9.6 that we might
> conceivably fix by reverting patches:
>

Yes, that would be a way forward for 9.6 if we are not able to close
blocking open items before beta1.  However, I think it would be bad if we
miss some of the below listed important features like snapshot_too_old or
atomic pin/unpin for 9.6.  Can we consider to postpone beta1, so that the
patch authors get time to resolve blocking issues?  I think there could be
a strong argument that it is just a waste of time if the situation doesn't
improve much even after delay, but it seems we can rely on people involved
in those patch sets to make a progress.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Amit Kapila]

On Tue, May 3, 2016 at 9:47 PM, Kevin Grittner  wrote:
>
> On Tue, May 3, 2016 at 11:09 AM, Robert Haas 
wrote:
> > On Tue, May 3, 2016 at 11:46 AM, Kevin Grittner 
wrote:
> >>> Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
> >>> you?
> >>
> >> Yes, I see three ways, the most obvious of which is what Amit
> >> suggested -- don't do early vacuum on a table which has a hash index.
> >
> > What do you mean by "early VACUUM"?
>
> Both vacuuming and hot-pruning adjust xmin based on calling
> TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
> Relation relation).  I'm talking about having that function, if all
> other conditions for the override pass, checking for a hash index,
> too.
>
> > Amit suggested disabling
> > HOT-pruning, but HOT-pruning happens completely outside of VACUUM.  It
> > also happens inside VACUUM, so if we disabled HOT pruning, how could
> > we VACUUM at all?  Sorry, I am confused.
>
> I guess we were both talking a bit loosely since (as I mentioned
> above) the function that adjusts the xmin is called for a vacuum or
> pruning.  He mentioned one and I mentioned the other, but it's all
> controlled by TransactionIdLimitedForOldSnapshots().
>

Yes, I think we are saying the same thing here.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Rename max_parallel_degree?

[David Rowley]

On 4 May 2016 at 15:12, Amit Kapila  wrote:
> On Mon, May 2, 2016 at 11:47 PM, Robert Haas  wrote:
>>
>> On Tue, Apr 26, 2016 at 11:49 AM, Robert Haas 
>> wrote:
>> > On Tue, Apr 26, 2016 at 11:44 AM, Tom Lane  wrote:
>> >> Robert Haas  writes:
>>
>> To summarize the positions as I understand them:
>>
>> Magnus seems OK with the way things are.
>> Peter wants to change either the fact that it is 0-based or the fact
>> that it is called degree, but is OK with either.
>> Tom doesn't like "degree" and also thinks anything called degree
>> should 1-based, but it sounds like he's more interested in changing
>> the first thing than the second one
>> Bruce and JD seemed to like degree -> workers.
>> JD also suggested another option that nobody else endorsed.
>> Alvaro suggested another option that nobody else endorsed.
>>
>> Does anyone else want to vote?
>>
>
> I think the way it is currently in HEAD seems easy to correlate how the
> feature works, but may be it appears to me that way because I am involved
> from long time with this implementation.   I also think one can easily
> confused among max_parallel_workers and max_worker_processes, so if we want
> to change, my vote goes with changing the default of max_parallel_degree to
> 1 (as suggested by Peter G.).

I'd like to put my +1 against making the current GUCs with their
current names 1-based, rather than 0-based. Doing anything else like
giving them new names seems like reinventing the wheel.

My reasoning is that the only gripe that I understand against the
current names is that the "degree" term appears not to be aligned with
what other databases do.

I think that actual rows / (degree+1) might get confusing for people,
such as in the following EXPLAIN ANALYZE output.

 Workers Launched: 2
 ->  Partial Aggregate  (cost=96331.21..96331.22 rows=1
width=8) (actual time=584.297..584.297 rows=1 loops=3)
   ->  Parallel Seq Scan on a  (cost=0.00..85914.57
rows=4166657 width=0) (actual time=1.566..347.091 rows=333
loops=3)

The above would make more sense with max_parallel_degree=3.

I also think that the parallel query, at its best will have the
workers working hard for their tuples. In such cases the main process
will be helping out much more, and the more it helps the more a
1-based degree makes sense. Also I can't stretch my imagination enough
to imagine how any other database can handle worker tuples any
differently than us... Surely their main process/thread must marshal
worker's tuples the same as what we do? And if they use a 1-based
degree for that, then surely we can too.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] text search changes vs. binary upgrade

[Tom Lane]

Noah Misch  writes:
> Commit bb14050 said:
> - change order for tsquery, so, users, who has a btree index over tsquery,
>   should reindex it

We undid that in 1ec4c7c05, no?  (Even if we didn't, the usefulness
of a btree index on tsquery seems negligibly small.)

> Commit 61d66c4 may or may not warrant pg_upgrade treatment:
> Fix support of digits in email/hostnames.

The general theory about changes in text search parser and dictionary
behavior has always been that a reindex is not required, because that does
not invalidate the derived data in the same sort of way that changing,
say, btree sort order of a datatype would.  At worst, searches for the
specifically affected words might fail to find relevant entries because
to_tsvector now produces a different list of lexemes than before (and
those new lexemes are not in the index, the old ones are).  If the
affected set of words is sufficiently large and relevant to her use-case,
a user might judge that rebuilding derived tsvector data is worth her
trouble.  But I am dubious that pg_upgrade should issue guidance
unconditionally telling people to do it.  Most people probably aren't
going to have any noticeable amount of data that's affected by this change.

If we did worry about this for 61d66c4, then for example the unaccent
changes would also be problematic, and probably the ispell changes too.
I'm inclined to just group all those things in the release notes and
provide text counseling users to think about how much those changes affect
their full-text data and whether rebuilding derived tsvectors would be
worth it.

regards, tom lane


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

Re: [HACKERS] Rename max_parallel_degree?

[Amit Kapila]

On Mon, May 2, 2016 at 11:47 PM, Robert Haas  wrote:
>
> On Tue, Apr 26, 2016 at 11:49 AM, Robert Haas 
wrote:
> > On Tue, Apr 26, 2016 at 11:44 AM, Tom Lane  wrote:
> >> Robert Haas  writes:
>
> To summarize the positions as I understand them:
>
> Magnus seems OK with the way things are.
> Peter wants to change either the fact that it is 0-based or the fact
> that it is called degree, but is OK with either.
> Tom doesn't like "degree" and also thinks anything called degree
> should 1-based, but it sounds like he's more interested in changing
> the first thing than the second one
> Bruce and JD seemed to like degree -> workers.
> JD also suggested another option that nobody else endorsed.
> Alvaro suggested another option that nobody else endorsed.
>
> Does anyone else want to vote?
>

I think the way it is currently in HEAD seems easy to correlate how the
feature works, but may be it appears to me that way because I am involved
from long time with this implementation.   I also think one can easily
confused among max_parallel_workers and max_worker_processes, so if we want
to change, my vote goes with changing the default of max_parallel_degree to
1 (as suggested by Peter G.).


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

[HACKERS] modifying WaitEventSets (was: Performance degradation in commit ac1d794)

[Robert Haas]

On Sun, Mar 20, 2016 at 8:31 AM, Thomas Munro
 wrote:
> One thing that I want to do but can't with this interface is remove an
> fd from the set.  I can AddWaitEventToSet returning a position, and I
> can ModifyWait to provide new event mask by position including zero
> mask, I can't actually remove the fd (for example to avoid future
> error events that can't be masked, or to allow that fd to be closed
> and perhaps allow that fd number to coincidentally be readded later,
> and just generally to free the slot).  There is an underlying way to
> remove an fd from a set with poll (sort of), epoll, kqueue.  (Not sure
> about Windows.  But surely...).  I wonder if there should be
> RemoveWaitEventFromSet(set, position) which recycles event slots,
> sticking them on a freelist (and setting corresponding pollfd structs'
> fd to -1).

In practice, you're not likely to want to completely forget about a
file descriptor and wait for some totally new file descriptor, or at
least not very often.  You're much more likely to have a set of fds
that you care about, but the events that you care about for each one
vary over time, and it may often by the empty set for some of those
fds.  For example, in the case of Append -> {Foreign Scan x many}, you
will have an fd for each foreign scan.  When you dispatch a query to a
particular server, you're now interested in whether that fd is
ready-ready (or, if SSL is involved, maybe sometimes you care about
write-ready instead) until you get back all the query results from
that server.  At that point, you no longer care about events on that
fd until you dispatch the next query - and then you again do.

So what's the best API for that?  One idea is to change
ModifyWaitEvent to accept events = 0 instead of failing an assertion
inside WaitEventAdjustEpoll.  We don't want to wait for EPOLLERR |
EPOLLHUP in that case since we'd have to wait event to return if one
of those things occurred.  It would be easy enough to rejigger that
code so that we pass epoll_ev.events as 0 in that case, but I think it
doesn't help: epoll_ctl(2) says epoll_wait(2) always waits for those
events.  Instead I think we'd have to use EPOLL_CTL_DEL in that case,
which is a problem: when the user next calls ModifyWaitEvent, we would
need to use EPOLL_CTL_ADD rather than EPOLL_CTL_MOD, but how would we
know that?  We'd have to add state for that somewhere.

Alternatively, we could implement RemoveWaitEventFromSet(set,
position), as you propose.  That, too, needs some kind of additional
state, because we've got to track which positions are unused.  I'd be
inclined to guess that a bitmap would be better than a linked list;
for the storage space of one pointer, we could handle all
WaitEventSets with nevents <= 64, which is hard to beat.

Yet another idea is to have a new event WL_SOCKET_ERROR which waits
for only EPOLLERR | EPOLLHUP.  So, if you don't care about the socket
being readable or writeable at the moment, but you still vaguely care
about the file descriptor, you can do ModifyWaitEvent(set, pos,
WL_SOCKET_ERROR, NULL).  That's actually kind of nice, because now you
can still detect error/eof conditions on sockets that you are not
otherwise waiting for, and report the errors promptly.  At the moment,
I'm inclined to think this might be the best option...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

[HACKERS] text search changes vs. binary upgrade

[Noah Misch]

Commit bb14050 said:

- change order for tsquery, so, users, who has a btree index over tsquery,
  should reindex it

The last change of this sort also modified pg_upgrade to issue REINDEX
guidance.  See old_8_3_invalidate_hash_gin_indexes() in the PostgreSQL 9.4
source.  PostgreSQL 9.6 pg_upgrade should do likewise.


Commit 61d66c4 may or may not warrant pg_upgrade treatment:

Fix support of digits in email/hostnames.

When tsearch was implemented I did several mistakes in hostname/email
definition rules:
1) allow underscore in hostname what prohibited by RFC
2) forget to allow leading digits separated by hyphen (like 123-x.com)
   in hostname
3) do no allow underscore/hyphen after leading digits in localpart of email
...

Any index (not just btree) that depends on a text search configuration using
parser pg_catalog.default may need a REINDEX after this change.  (Furthermore,
any constraint having such a dependency would need a recheck.  That use case
may be less important.)  I think the last changes to pg_catalog.default
semantics were 2c265ad (URLs) and 89b0095 (emails), both in 9.0.  For those,
we didn't change pg_upgrade or recommend REINDEX in the release notes.  We
could call that a relevant precedent and, for this 9.6 change, once again take
no particular action.  On the other hand, binary upgrade has matured since its
9.0 birth.  Perhaps standards have risen, and pg_upgrade should issue guidance
to REINDEX affected text search indexes.  (The guidance could mention the kind
of queries that will notice the difference.)  I lean toward having pg_upgrade
address this incompatibility.  Other opinions?


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

Re: [HACKERS] pg_xlog -> pg_xjournal?

[Joshua D. Drake]

On 05/03/2016 06:38 PM, Michael Paquier wrote:

On Mon, May 2, 2016 at 10:29 PM, Robert Haas  wrote:

On Thu, Apr 28, 2016 at 11:46 PM, Craig Ringer  wrote:

I just helped another person yesterday who deleted their pg_xlog.


The biggest reason I've seen pg_xlog get deleted is not because it's
called pg_xlog, but because it's located someplace other than under
the data directory and referenced via a symlink.  Renaming it might
still make it less likely for people to get trigger happy, though.


FWIW, I have seen a couple of times the same pattern as Craig:
"because it contains log in its name implies that removing it causes
no harm".


"You can't fix stupid.", Ron White.

That said, I too have run into the "Oh it said, log... we thought it was 
o.k. to delete."


Sincerely,

JD


--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


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

Re: [HACKERS] pg_xlog -> pg_xjournal?

[Michael Paquier]

On Mon, May 2, 2016 at 10:29 PM, Robert Haas  wrote:
> On Thu, Apr 28, 2016 at 11:46 PM, Craig Ringer  wrote:
>> I just helped another person yesterday who deleted their pg_xlog.
>
> The biggest reason I've seen pg_xlog get deleted is not because it's
> called pg_xlog, but because it's located someplace other than under
> the data directory and referenced via a symlink.  Renaming it might
> still make it less likely for people to get trigger happy, though.

FWIW, I have seen a couple of times the same pattern as Craig:
"because it contains log in its name implies that removing it causes
no harm".
-- 
Michael


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

Re: [HACKERS] psql :: support for \ev viewname and \sv viewname

[Peter Eisentraut]

On 5/3/16 3:10 PM, Dean Rasheed wrote:

On 3 May 2016 at 16:52, Peter Eisentraut
 wrote:

I would change appendReloptionsArrayAH() to a function and keep AH as the
first argument (similar to other functions that take such a handle).


I can understand changing it to a function, but I don't think AH
should be the first argument. All other append*() functions that
append to a buffer have the buffer as the first argument, including
the appendStringLiteralAH() macro on which this is based.


Well, all the functions that take archive handles have that as the first 
argument, so how do we consolidate that?


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)

[David Rowley]

On 4 May 2016 at 09:18, David Rowley  wrote:
> On 4 May 2016 at 02:10, Tomas Vondra  wrote:
>> There are probably a few reasonably simple things we could do - e.g. ignore
>> foreign keys with just a single column, as the primary goal of the patch is
>> improving estimates with multi-column foreign keys. I believe that covers a
>> vast majority of foreign keys in the wild.
>>
>> If that's deemed insufficient, we'll have to resort to a more complex
>> improvement, perhaps something akin to the cache proposed in in the unijoin
>> patch. But if that's required, that's 9.7 material.
>
> I had thought that if we had a hashtable of rel OIDs which belong to
> relations with has_eclass_joins == true, then we could just skip
> foreign keys where the confrelid is not in the hashtable. Perhaps that
> could be optimised a bit more and we could have something akin to what
> predOK is for IndexOptInfo in ForeignKeyOptInfo which just gets set to
> true if the relation referenced by the foreign key is in the
> simple_rel_array. It's quite likely that if many foreign keys were
> used, then the query would have a great number of joins, and planning
> would be slow anyway.

I've spent a few hours looking at this and I've come up with the
attached patch, which flags each ForeignKeyOptInfo to say whether its
possible to be referenced in any join condition, with the logic that
if the referenced relation is in the simple_rte_array, then it could
be referenced.

I ran some of the tests Tomas posted with 1000 FKs and a 4-way join,
with 2 join columns.

Query:
explain analyze
select * from f1
inner join f2 on f1.a = f2.a and f1.b = f2.b
inner join f3 on f1.a = f3.a and f1.b = f3.b
inner join f4 on f1.a = f4.a and f1.b = f4.b;

SET enable_fkey_estimates = on;
duration: 30 s
number of transactions actually processed: 8173
latency average: 3.671 ms
tps = 272.420508 (including connections establishing)
tps = 272.586329 (excluding connections establishing)

SET enable_fkey_estimates = off;
duration: 30 s
number of transactions actually processed: 9153
latency average: 3.278 ms
tps = 305.098852 (including connections establishing)
tps = 305.286072 (excluding connections establishing)

So there's still a 10% planner slowdown for this worst case test, but
it's far less than what it was previously with the same test case.

I just also want to add that the aim of this patch was to fix a very
real world problem which also manifests itself in TPC-H Q9, where the
join to partsupp is drastically underestimated due to the 2 column
join condition, which in our test cases caused the GROUP BY to perform
a Hash Aggregate rather than a Sort/Group Aggregate and since we don't
spill HashAggs to disk, we get OOM for a large scale test on large
scale hardware.

Here's some sample EXPLAIN output from the query in question, which I
think is a smaller scale than the 3TB test where we had issues, but
still demonstrates the issue;

Hash Join  (cost=74686.00..597734.90 rows=2400 width=23) (actual
time=564.038..11645.047 rows=11997996 loops=1)
  Hash Cond: ((lineitem.l_suppkey = partsupp.ps_suppkey) AND
(lineitem.l_partkey = partsupp.ps_partkey))

Here the estimate is off 5000x.

The attached patch is intended to assist discussion at the moment.
Likely some naming could be better, and the code would need to find a
better home.

The patch also fixes the missing IsA OpExpr test.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


mark_useful_foreignkeys.patch
Description: Binary data

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

Re: [HACKERS] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Andres Freund]

Hi,

On 2016-05-03 07:24:46 +0200, Fabien COELHO wrote:
> >>I'm unsure about switching enum to #define, could be an enum still with
> >>explicit values set, something like:
> >
> >An enum doesn't have a benefit for a bitmask imo - you can't "legally"
> >use it as a type for functions accepting the bitmask.
> 
> I do not understand. I suggested to use enum to enumerate the bitmask
> constants, ISTM that it does not preclude to use it as a bitmask as you do,
> it is just a replacement of the #define? The type constraint on the enum
> does not disallow bitmasking values, I checked with both gcc & clang.

There's not really a point in using an enum if you use neither the type
(which you shouldn't because if you or the bitmask value you have types
outside the range of the enum), nor the autogenerated numbers. Anyway
seems fairly unimportant.


> >>I'm fuzzy about the _OPEN_DELETED part because it is an oxymoron. Is it
> >>RECREATE really?
> >
> >No. The relevant explanation is at the top of the file:
> 
> [...]
> 
> >*-- Optionally, any number of inactive segments of size 0 blocks.
> >*Inactive segments are those that once contained data but are currently
> >*not needed because of an mdtruncate() operation.  The reason for leaving
> >*them present at size zero, rather than unlinking them, is that other
> >*backends and/or the checkpointer might be holding open file references 
> >to
> >*such segments.  If the relation expands again after mdtruncate(), such
> >*that a deactivated segment becomes active again, it is important that
> >*such file references still be valid --- else data might get written
> >*out to an unlinked old copy of a segment file that will eventually
> >*disappear.
> 
> Ok.
> 
> Then should it be _OPEN_INACTIVE[TED] or _OPEN_TRUNCATED rather than
> _OPEN_DELETED, which is contradictory?

Well, TRUNCATED doesn't entirely work, there's I think some cases where
this currently also applies to deleted relations. I kind of like the
unintuitive sounding name, because it's really a dangerous options (any
further mdnblocks will be wrong).


Andres


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

Re: [HACKERS] what to revert

[Craig Ringer]

On 4 May 2016 at 01:12, Peter Geoghegan  wrote:

> On Tue, May 3, 2016 at 9:58 AM, Alvaro Herrera 
> wrote:
> > As its committer, I tend to agree about reverting that feature.  Craig
> > was just posting some more patches, and I have the pg_recvlogical
> > changes here (--endpos) which after some testing are not quite looking
> > ready to go -- plus we still have to write the actual Perl test scripts
> > that would use it.  Taken together, this is now looking to me a bit
> > rushed, so I prefer to cut my losses here and revert the patch so that
> > we can revisit it for 9.7.
>
> I think it's a positive development that we can take this attitude to
> reverting patches. It should not be seen as a big personal failure,
> because it isn't. Stigmatizing reverts incentivizes behavior that
> leads to bad outcomes.


Indeed. Being scared to revert also makes the barrier to committing
something and getting it into more hands, for longer, under a variety of
different conditions higher. Not a ton of help with this particular feature
but quite important with others.

While I'm personally disappointed by this outcome, I also can't really
disagree with it. The whole area is a bit of a mess and hard to work on,
and as demonstrated my understanding of it when I wrote the original patch
was incomplete. We could commit the rewritten function, but ... rewriting a
feature just before beta1 probably says it's just not baked yet, right?

The upside of all this is that now I have a decent picture of how it should
all look and how timeline following, failover capability etc can be
introduced in a staged way. I'd also like to get rid of the use of various
globals to pass timeline information "around" the walsender and share more
of the logic between the code paths.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] atomic pin/unpin causing errors

[Andres Freund]

Hi Jeff,

On 2016-04-29 10:38:55 -0700, Jeff Janes wrote:
> I've bisected the errors I was seeing, discussed in
> http://www.postgresql.org/message-id/CAMkU=1xqehc0ok4d+tkjfq1nvuho37pyrkhjp6q8oxifmx7...@mail.gmail.com
> 
> It look like they first appear in:
> 
> commit 48354581a49c30f5757c203415aa8412d85b0f70
> Author: Andres Freund 
> Date:   Sun Apr 10 20:12:32 2016 -0700
> 
> Allow Pin/UnpinBuffer to operate in a lockfree manner.
> 
> 
> I get the errors:
> 
> ERROR:  attempted to delete invisible tuple
> STATEMENT:  update foo set count=count+1,text_array=$1 where text_array @> $2
> 
> And also:
> 
> ERROR:  unexpected chunk number 1 (expected 2) for toast value
> 85223889 in pg_toast_16424
> STATEMENT:  update foo set count=count+1 where text_array @> $1

Hm. I appear to have trouble reproducing this issue (continuing to try)
on master as of 8826d8507.  Is there any chance you could package up a
data directory after the issue hit?


> (This was all run using Teodor's test-enabling patch
> gin_alone_cleanup-4.patch, so as not to change horses in midstream.
> Now that a version of that patch has been committed, I will try to
> repeat this in HEAD)

Any news on that front?

Greetings,

Andres Freund


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

Re: [HACKERS] what to revert

[Andres Freund]

On 2016-05-03 20:57:13 +0200, Tomas Vondra wrote:
> On 05/03/2016 07:41 PM, Andres Freund wrote:
> >On 2016-05-03 11:46:23 -0500, Kevin Grittner wrote:
> >>>was immediately addressed by another round of benchmarks after you
> >>>pointed it out.
> >>
> >>Which showed a 4% maximum hit before moving the test for whether it
> >>was "off" inline.
> >
> >>(I'm not clear from the posted results whether that was before or
> >>after skipping the spinlock when the feature was off.)
> >
> >They're from after the spinlock issue was resolved. Before that the
> >issue was a lot worse (see mail linked two messages upthread).
> >
> >
> >I'm pretty sure that I said that somewhere else at least once: But to
> >be absolutely clear, I'm *not* really concerned with the performance
> >with the feature turned off. I'm concerned about the performance with
> >it turned on.
> 
> If you tell me how to best test it, I do have a 4-socket server sitting idly
> in the corner (well, a corner reachable by SSH). I can get us some numbers,
> but I haven't been following the snapshot_too_old so I'll need some guidance
> on what to test.

I think it'd be cool if you could test the effect of the feature in
read-only (and additionally read-mostly?) workload with various client
counts and snapshot_too_old values. For the latter maybe -1, 0, 10, 60
or such?  I've done so (accidentally comparing 0 and 1 instead of -1 and
1) on a two socket machine in:
www.postgresql.org/message-id/20160413171955.i53me46fqqhdl...@alap3.anarazel.de 

It'd be very interesting to see how big the penalty is on a bigger box.


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

Re: [HACKERS] what to revert

[Andres Freund]

On 2016-05-04 00:01:20 +0300, Ants Aasma wrote:
> On Tue, May 3, 2016 at 9:57 PM, Tomas Vondra
>  wrote:
> > If you tell me how to best test it, I do have a 4-socket server sitting idly
> > in the corner (well, a corner reachable by SSH). I can get us some numbers,
> > but I haven't been following the snapshot_too_old so I'll need some guidance
> > on what to test.
> 
> I worry about two contention points with the current implementation.
> 
> The main one is the locking within MaintainOldSnapshotTimeMapping()
> that gets called every time a snapshot is taken. AFAICS this should
> show up by setting old_snapshot_threshold to any positive value and
> then running a simple within shared buffers scale factor read only
> pgbench at high concurrency (number of CPUs or a small multiple). On a
> single socket system this does not show up.

On a two socket system it does, check the bottom of:
http://archives.postgresql.org/message-id/20160413171955.i53me46fqqhdlztq%40alap3.anarazel.de
Note that this (accidentally) compares thresholds 0 and 10 (instead of
-1 and 10), but that's actually interesting for this question because of
the quick exit in MaintainOldSnapshotData:
/* No further tracking needed for 0 (used for testing). */
if (old_snapshot_threshold == 0)
return;
which happens before the lwlock is taken.


> The second one is probably a bit harder to hit,
> GetOldSnapshotThresholdTimestamp() has a spinlock that gets hit
> everytime a scan sees a page that has been modified after the snapshot
> was taken. A workload that would tickle this is something that uses a
> repeatable read snapshot, builds a non-temporary table and runs
> reporting on it.

I'm not particularly concerned about that kind of issue - we can quite
easily replace that spinlock with 64bit atomic reads/writes for which
I've already proposed a patch. I'd expect that to go into 9.7.

Greetings,

Andres Freund


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

Re: [HACKERS] Timeline following for logical slots

[Alvaro Herrera]

I don't like reverting patches, but this patch is making me more and
more uncomfortable.  We have two open items, one of which requires
writing new test code that doesn't exist yet; and we have the
pg_recvlogical changes that were approved post-feature freeze, but that
I now have second thoughts about pushing right away.
Craig has also commented on some followup patch to change this whole
area in 9.7.

Per
https://www.postgresql.org/message-id/20160503165812.GA29604%40alvherre.pgsql
I think the best course of action is to revert the whole feature and
revisit for 9.7.

Here's a proposed revert patch.  Many minor changes (mostly comment
additions) that were applied as part of this series are kept intact.
The test_slot_timeline test module and corresponding recovery test
script are also reverted.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
commit 72604a6c1c3e7952e9e11a554e40117cb974a463
Author: Alvaro Herrera 
AuthorDate: Tue May 3 16:37:01 2016 -0300
CommitDate: Tue May 3 17:18:17 2016 -0300

Revert timeline following in replication slots

This reverts commits f07d18b6e94d, 82c83b337202, 3a3b309041b0, and
24c5f1a103ce.

This feature has shown enough immaturity that it was deemed better to
rip it out before rushing some more fixes at the last minute.  There are
discussions on larger changes in this area for the next release.

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index c5a964a..c3aecc7 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -118,11 +118,6 @@ XLogReaderAllocate(XLogPageReadCB pagereadfunc, void *private_data)
 		return NULL;
 	}
 
-#ifndef FRONTEND
-	/* Will be loaded on first read */
-	state->timelineHistory = NIL;
-#endif
-
 	return state;
 }
 
@@ -142,10 +137,6 @@ XLogReaderFree(XLogReaderState *state)
 	pfree(state->errormsg_buf);
 	if (state->readRecordBuf)
 		pfree(state->readRecordBuf);
-#ifndef FRONTEND
-	if (state->timelineHistory)
-		list_free_deep(state->timelineHistory);
-#endif
 	pfree(state->readBuf);
 	pfree(state);
 }
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index f6ca2b9..cb4563e 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -19,7 +19,6 @@
 
 #include 
 
-#include "access/timeline.h"
 #include "access/xlog.h"
 #include "access/xlog_internal.h"
 #include "access/xlogutils.h"
@@ -660,7 +659,6 @@ XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
 	/* state maintained across calls */
 	static int	sendFile = -1;
 	static XLogSegNo sendSegNo = 0;
-	static TimeLineID sendTLI = 0;
 	static uint32 sendOff = 0;
 
 	p = buf;
@@ -676,8 +674,7 @@ XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
 		startoff = recptr % XLogSegSize;
 
 		/* Do we need to switch to a different xlog segment? */
-		if (sendFile < 0 || !XLByteInSeg(recptr, sendSegNo) ||
-			sendTLI != tli)
+		if (sendFile < 0 || !XLByteInSeg(recptr, sendSegNo))
 		{
 			char		path[MAXPGPATH];
 
@@ -704,7 +701,6 @@ XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
 	path)));
 			}
 			sendOff = 0;
-			sendTLI = tli;
 		}
 
 		/* Need to seek in the file? */
@@ -753,147 +749,6 @@ XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
 }
 
 /*
- * Determine XLogReaderState->currTLI and ->currTLIValidUntil;
- * XLogReaderState->EndRecPtr, ->currRecPtr and ThisTimeLineID affect the
- * decision.  This may later be used to determine which xlog segment file to
- * open, etc.
- *
- * We switch to an xlog segment from the new timeline eagerly when on a
- * historical timeline, as soon as we reach the start of the xlog segment
- * containing the timeline switch.  The server copied the segment to the new
- * timeline so all the data up to the switch point is the same, but there's no
- * guarantee the old segment will still exist. It may have been deleted or
- * renamed with a .partial suffix so we can't necessarily keep reading from
- * the old TLI even though tliSwitchPoint says it's OK.
- *
- * Because of this, callers MAY NOT assume that currTLI is the timeline that
- * will be in a page's xlp_tli; the page may begin on an older timeline or we
- * might be reading from historical timeline data on a segment that's been
- * copied to a new timeline.
- */
-static void
-XLogReadDetermineTimeline(XLogReaderState *state)
-{
-	/* Read the history on first time through */
-	if (state->timelineHistory == NIL)
-		state->timelineHistory = readTimeLineHistory(ThisTimeLineID);
-
-	/*
-	 * Are we reading the record immediately following the one we read last
-	 * time?  If not, then don't use the cached timeline info.
-	 */
-	if (state->currRecPtr != state->EndRecPtr)
-	{
-		state->currTLI = 0;
-		state->currTLIValidUntil = InvalidXLogRecPtr;

Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)

[David Rowley]

On 4 May 2016 at 02:10, Tomas Vondra  wrote:
> There are probably a few reasonably simple things we could do - e.g. ignore
> foreign keys with just a single column, as the primary goal of the patch is
> improving estimates with multi-column foreign keys. I believe that covers a
> vast majority of foreign keys in the wild.
>
> If that's deemed insufficient, we'll have to resort to a more complex
> improvement, perhaps something akin to the cache proposed in in the unijoin
> patch. But if that's required, that's 9.7 material.

I had thought that if we had a hashtable of rel OIDs which belong to
relations with has_eclass_joins == true, then we could just skip
foreign keys where the confrelid is not in the hashtable. Perhaps that
could be optimised a bit more and we could have something akin to what
predOK is for IndexOptInfo in ForeignKeyOptInfo which just gets set to
true if the relation referenced by the foreign key is in the
simple_rel_array. It's quite likely that if many foreign keys were
used, then the query would have a great number of joins, and planning
would be slow anyway.


-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] what to revert

[Ants Aasma]

On Tue, May 3, 2016 at 9:57 PM, Tomas Vondra
 wrote:
> If you tell me how to best test it, I do have a 4-socket server sitting idly
> in the corner (well, a corner reachable by SSH). I can get us some numbers,
> but I haven't been following the snapshot_too_old so I'll need some guidance
> on what to test.

I worry about two contention points with the current implementation.

The main one is the locking within MaintainOldSnapshotTimeMapping()
that gets called every time a snapshot is taken. AFAICS this should
show up by setting old_snapshot_threshold to any positive value and
then running a simple within shared buffers scale factor read only
pgbench at high concurrency (number of CPUs or a small multiple). On a
single socket system this does not show up.

The second one is probably a bit harder to hit,
GetOldSnapshotThresholdTimestamp() has a spinlock that gets hit
everytime a scan sees a page that has been modified after the snapshot
was taken. A workload that would tickle this is something that uses a
repeatable read snapshot, builds a non-temporary table and runs
reporting on it. Something like this would work:

BEGIN ISOLATION LEVEL REPEATABLE READ;
DROP TABLE IF EXISTS test_:client_id;
CREATE TABLE test_:client_id (x int, filler text);
INSERT INTO test_:client_id  SELECT x, repeat(' ', 1000) AS filler
FROM generate_series(1,1000) x;
SELECT (SELECT COUNT(*) FROM test_:client_id WHERE x != y) FROM
generate_series(1,1000) y;
COMMIT;

With this script running with -c4 on a 4 core workstation I'm seeing
the following kind of contention and a >2x loss in throughput:

+   14.77%  postgres  postgres   [.] GetOldSnapshotThresholdTimestamp
-8.01%  postgres  postgres   [.] s_lock
   - s_lock
  + 88.15% GetOldSnapshotThresholdTimestamp
  + 10.47% TransactionIdLimitedForOldSnapshots
  + 0.71% TestForOldSnapshot_impl
  + 0.57% GetSnapshotCurrentTimestamp

Now this is kind of an extreme example, but I'm willing to bet that on
multi socket hosts similar issues can crop up with common real world
use cases.

Regards,
Ants Aasma


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

Re: [HACKERS] Pg_stop_backup process does not run - Backup Intervals

[David G. Johnston]

On Mon, May 2, 2016 at 12:03 PM, Rodrigo Cavalcante 
wrote:

> Hi,
>
> On alternate days my backup is failing, by the pg_stop_backup process ()
> does not perform or quit.
>
> Version PostgreSQL: 9.1.6
>

​Reporting unusual behavior while running a years-old point release is
unlikely to be productive.  No one wants to debug something that very well
may have been fixed in the meantime.

And, as Robert said, you need to provide considerably more detail than you
have if you wish to get help diagnosing your situation.  And I too would
recommend you not "roll your own" in this area unless you are trying to
learn, at a lower-level, how these things work.

David J.
​

Re: [HACKERS] Pg_stop_backup process does not run - Backup Intervals

[Robert Haas]

On Tue, May 3, 2016 at 4:21 PM, Rodrigo Cavalcante
 wrote:
> The my script works, but after a few days it stops working because the
> process does not end pg_stop_backup.

Well, that shouldn't happen, but without any logs or debugging
information, it's hard to guess why.

> The pg_basebackup already does this substitution?

If you use pg_basebackup, you don't need to run pg_start_backup() or
pg_stop_backup().

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Pg_stop_backup process does not run - Backup Intervals

[Rodrigo Cavalcante]

Hello Robert,

Thanks for the feedback.

The my script works, but after a few days it stops working because the
process does not end pg_stop_backup.

The pg_basebackup already does this substitution?



--
View this message in context: 
http://postgresql.nabble.com/Pg-stop-backup-process-does-not-run-Backup-Intervals-tp5901538p5901757.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

Re: [HACKERS] pg_upgrade and toasted pg_largeobject

[Tom Lane]

Alvaro Herrera  writes:
> Robert Haas wrote:
>> On Tue, May 3, 2016 at 2:09 PM, Alvaro Herrera  
>> wrote:
>>> How about backpatching patch 1 all the way back, and putting the others
>>> in 9.6?

>> Why would we do that?  It seems very odd to back-patch a pure
>> refactoring - isn't that taking a risk for no benefit?

Yeah, I don't see the point of that either.

> My inclination is actually to put the whole series back to 9.2, but if
> we don't want to do that, then backpatching just the first one seems to
> make pg_upgrade more amenable to future bugfixes.

I checked, and found that patch 1 doesn't apply cleanly before 9.5.
I've not looked into exactly why not, but it would possibly take some
work to adapt these patches to older branches.

regards, tom lane


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

Re: [HACKERS] pg_upgrade and toasted pg_largeobject

[Alvaro Herrera]

Robert Haas wrote:
> On Tue, May 3, 2016 at 2:09 PM, Alvaro Herrera  
> wrote:
> > Tom Lane wrote:
> >> Any thoughts what to do with this?  We could decide that it's a bug fix
> >> and backpatch, or decide that it's a new feature and delay till 9.7,
> >> or decide that it's a minor bug fix and add it to 9.6 only.  I kinda lean
> >> towards the last alternative.
> >
> > How about backpatching patch 1 all the way back, and putting the others
> > in 9.6?
> 
> Why would we do that?  It seems very odd to back-patch a pure
> refactoring - isn't that taking a risk for no benefit?

>From Tom's description, what is there works by chance only, and maybe
not even in all cases.  The rest of the patches are to fix one
particular problem, which perhaps is not overly serious, but maybe some
other future problem will be discovered and we will want to have patch 1
installed.

My inclination is actually to put the whole series back to 9.2, but if
we don't want to do that, then backpatching just the first one seems to
make pg_upgrade more amenable to future bugfixes.

(I say "back to 9.2" instead of "back to 9.1" because surely we don't
care all that much about upgrades *to* 9.1, since it's going unsupported
soon.)

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


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

Re: [HACKERS] psql :: support for \ev viewname and \sv viewname

[Dean Rasheed]

On 3 May 2016 at 16:52, Peter Eisentraut
 wrote:
> I would change appendReloptionsArrayAH() to a function and keep AH as the
> first argument (similar to other functions that take such a handle).

I can understand changing it to a function, but I don't think AH
should be the first argument. All other append*() functions that
append to a buffer have the buffer as the first argument, including
the appendStringLiteralAH() macro on which this is based.

Regards,
Dean


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

Re: [HACKERS] what to revert

[Tomas Vondra]

On 05/03/2016 07:41 PM, Andres Freund wrote:

On 2016-05-03 11:46:23 -0500, Kevin Grittner wrote:

was immediately addressed by another round of benchmarks after you
pointed it out.


Which showed a 4% maximum hit before moving the test for whether it
was "off" inline.



(I'm not clear from the posted results whether that was before or
after skipping the spinlock when the feature was off.)


They're from after the spinlock issue was resolved. Before that the
issue was a lot worse (see mail linked two messages upthread).


I'm pretty sure that I said that somewhere else at least once: But to
be absolutely clear, I'm *not* really concerned with the performance
with the feature turned off. I'm concerned about the performance with
it turned on.


If you tell me how to best test it, I do have a 4-socket server sitting 
idly in the corner (well, a corner reachable by SSH). I can get us some 
numbers, but I haven't been following the snapshot_too_old so I'll need 
some guidance on what to test.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] what to revert

[Tomas Vondra]

On 05/03/2016 07:12 PM, Peter Geoghegan wrote:

On Tue, May 3, 2016 at 9:58 AM, Alvaro Herrera  wrote:

As its committer, I tend to agree about reverting that feature.  Craig
was just posting some more patches, and I have the pg_recvlogical
changes here (--endpos) which after some testing are not quite looking
ready to go -- plus we still have to write the actual Perl test scripts
that would use it.  Taken together, this is now looking to me a bit
rushed, so I prefer to cut my losses here and revert the patch so that
we can revisit it for 9.7.


I think it's a positive development that we can take this attitude to
reverting patches. It should not be seen as a big personal failure,
because it isn't. Stigmatizing reverts incentivizes behavior that
leads to bad outcomes.



Absolutely +1

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] pg_upgrade and toasted pg_largeobject

[Robert Haas]

On Tue, May 3, 2016 at 2:09 PM, Alvaro Herrera  wrote:
> Tom Lane wrote:
>> Any thoughts what to do with this?  We could decide that it's a bug fix
>> and backpatch, or decide that it's a new feature and delay till 9.7,
>> or decide that it's a minor bug fix and add it to 9.6 only.  I kinda lean
>> towards the last alternative.
>
> How about backpatching patch 1 all the way back, and putting the others
> in 9.6?

Why would we do that?  It seems very odd to back-patch a pure
refactoring - isn't that taking a risk for no benefit?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Andrew Dunstan]

On 05/03/2016 01:58 PM, Alvaro Herrera wrote:

Andrew Dunstan wrote:


And if this is of any use, here are the dump differences from every live
version to git tip, as of this morning.

Interesting, thanks.  I wonder if some of these diffs could be reduced
further by using pg_dump -Fd instead of a single text dump -- then
internal ordering would not matter, and I see that a large part of these
diffs is where GRANTs appear.  (I don't think it's a problem to use a
newer pg_dump to dump the older databases that don't support -Fd, for
this purpose.)

How would you recommend to run this in the coverage reporting machine?
Currently it's just doing "make check-world".  Could we add a buildfarm
script to run, standalone?




Well, to run it you just run a buildfarm animal, possibly not even 
registered, with the module enabled. The module is actually in every 
buildfarm release, and has been for some time, but it's not enabled. 
Right now even if it runs it doesn't report anything to the server, it 
just outputs success/failure lines to stdout.


cheers

andrew



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

Re: [HACKERS] Accidentally parallel unsafe functions

[Robert Haas]

On Fri, Apr 29, 2016 at 6:06 PM, Andreas Karlsson  wrote:
> I am currently looking into adding the correct parallel options to all
> functions in the extensions and I noticed that some built-in functions seems
> to have been marked as unsafe by accident. The main culprit is
> system_views.sql which redefines these functions and removes the parallel
> safe flag.
>
> I think this counts as a 9.6 bug unlike my work on adding the flags to all
> extensions which is for 9.7.
>
> I have attached a patch which marks them and all conversion functions as
> parallel safe. I also added the flag to ts_debug() when I was already
> editing system_views.sql, feel free to ignore that one if you like.
>
> Affected functions:
>
> - json_populate_record()
> - json_populate_recordset()
> - jsonb_insert()
> - jsonb_set()
> - make_interval()
> - parse_ident()
> - Loads of conversion functions

Committed all of this except for the bit about pg_start_backup, for
which I committed a separate fix.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] pg_upgrade and toasted pg_largeobject

[Alvaro Herrera]

Tom Lane wrote:

> Any thoughts what to do with this?  We could decide that it's a bug fix
> and backpatch, or decide that it's a new feature and delay till 9.7,
> or decide that it's a minor bug fix and add it to 9.6 only.  I kinda lean
> towards the last alternative.

How about backpatching patch 1 all the way back, and putting the others
in 9.6?


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


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

Re: [HACKERS] Timeline following for logical slots

[Alvaro Herrera]

Craig Ringer wrote:
 
> With this patch pg_recvlogical takes a new --endpos LSN argument, and will
> exit if either:
> 
> * it receives an XLogData message with dataStart >= endpos; or
> * it receives a keepalive with walEnd >= endpos
> 
> The latter allows it to work without needing a dummy transaction to make it
> see a data message after endpos. If there's nothing to read on the socket
> until a keepalive we know that the server has nothing to send us, and if
> walend has passed endpos we know nothing can have committed before endpos.

Here's a slightly revised version of this patch, for later
consideration.

Changes:
- added -E as short form of --endpos (consistent with -I as --startpos)
- refactored some repetitive code in two auxilliary functions
- allow --endpos to work with --create-slot.
- revert some unrelated changes, such as message additions in verbose
  mode and changes to existing messages
- documentation reworked.

I didn't spot any bugs in Craig's patch, but it needs more testing.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml
index 9d0b58b..6f23229 100644
--- a/doc/src/sgml/ref/pg_recvlogical.sgml
+++ b/doc/src/sgml/ref/pg_recvlogical.sgml
@@ -155,6 +155,41 @@ PostgreSQL documentation
  
 
  
+  -E lsn
+  --endpos=lsn
+  
+   
+In --start mode, automatically stop replication
+and exit with normal exit status 0 when receiving reaches the
+specified LSN.  If specified when not in --start
+mode, an error is raised.
+   
+
+   
+Note the following points:
+
+  
+  
+   If there's a record with LSN exactly equal to lsn,
+   the record will not be output.  If you want to receive up to and
+   including a given LSN, specify LSN + 1 as the desired stop point.
+  
+ 
+ 
+  
+   The --endpos option is not aware of transaction
+   boundaries and may truncate output partway through a transaction.
+   Any partially output transaction will not be consumed and will be
+   replayed again when the slot is next read from. Individual messages
+   are never truncated.
+  
+ 
+
+   
+  
+ 
+
+ 
   --if-not-exists
   

diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 6d12705..5108222 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -37,6 +37,7 @@ static int	noloop = 0;
 static int	standby_message_timeout = 10 * 1000;		/* 10 sec = default */
 static int	fsync_interval = 10 * 1000; /* 10 sec = default */
 static XLogRecPtr startpos = InvalidXLogRecPtr;
+static XLogRecPtr endpos = InvalidXLogRecPtr;
 static bool do_create_slot = false;
 static bool slot_exists_ok = false;
 static bool do_start_slot = false;
@@ -60,6 +61,9 @@ static XLogRecPtr output_fsync_lsn = InvalidXLogRecPtr;
 static void usage(void);
 static void StreamLogicalLog(void);
 static void disconnect_and_exit(int code);
+static bool flushAndSendFeedback(PGconn *conn, TimestampTz *now);
+static void prepareToTerminate(PGconn *conn, XLogRecPtr endpos,
+   bool keepalive, XLogRecPtr lsn);
 
 static void
 usage(void)
@@ -78,6 +82,7 @@ usage(void)
 			 " time between fsyncs to the output file (default: %d)\n"), (fsync_interval / 1000));
 	printf(_("  --if-not-existsdo not error if slot already exists when creating a slot\n"));
 	printf(_("  -I, --startpos=LSN where in an existing slot should the streaming start\n"));
+	printf(_("  -E, --endpos=LSN   exit upon receiving the specified LSN\n"));
 	printf(_("  -n, --no-loop  do not loop on connection lost\n"));
 	printf(_("  -o, --option=NAME[=VALUE]\n"
 			 " pass option NAME with optional value VALUE to the\n"
@@ -278,6 +283,7 @@ StreamLogicalLog(void)
 		int			bytes_written;
 		int64		now;
 		int			hdr_len;
+		XLogRecPtr	cur_record_lsn = InvalidXLogRecPtr;
 
 		if (copybuf != NULL)
 		{
@@ -451,6 +457,7 @@ StreamLogicalLog(void)
 			int			pos;
 			bool		replyRequested;
 			XLogRecPtr	walEnd;
+			bool		endposReached = false;
 
 			/*
 			 * Parse the keepalive message, enclosed in the CopyData message.
@@ -473,18 +480,32 @@ StreamLogicalLog(void)
 			}
 			replyRequested = copybuf[pos];
 
-			/* If the server requested an immediate reply, send one. */
-			if (replyRequested)
+			if (endpos != InvalidXLogRecPtr && walEnd >= endpos)
 			{
-/* fsync data, so we send a recent flush pointer */
-if (!OutputFsync(now))
-	goto error;
+/*
+ * If there's nothing to read on the socket until a keepalive
+ * we know that the server has nothing to send us; and if
+ * walEnd has passed endpos, we know no

Re: [HACKERS] pg_upgrade and toasted pg_largeobject

[Robert Haas]

On Tue, May 3, 2016 at 1:38 PM, Tom Lane  wrote:
> Any thoughts what to do with this?  We could decide that it's a bug fix
> and backpatch, or decide that it's a new feature and delay till 9.7,
> or decide that it's a minor bug fix and add it to 9.6 only.  I kinda lean
> towards the last alternative.

I'm fine with that.

(But I haven't reviewed the code.)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Tom Lane]

Andres Freund  writes:
> On 2016-05-03 12:07:51 -0400, Tom Lane wrote:
>> I think possibly the easiest fix for this is to have pg_upgrade,
>> instead of RESETting a nonexistent option, RESET something that's
>> still considered to require AccessExclusiveLock.  "user_catalog_table"
>> would work, looks like; though I'd want to annotate its entry in
>> reloptions.c to warn people away from downgrading its lock level.

> Alternatively we could just add a function for adding a toast table -
> that seems less hacky and less likely to be broken in the future.

We used to have an explicit "ALTER TABLE CREATE TOAST TABLE", IIRC,
but we got rid of it.  In any case, forcible creation is not what
we're after here, we just want to create it if needs_toast_table()
thinks we need one.  I'm fine with it being a hack, as long as we
have test coverage so we notice when somebody breaks the hack.

regards, tom lane


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Andres Freund]

On 2016-05-03 13:47:14 -0400, Tom Lane wrote:
> I've been thinking of proposing that it's time (not now, at this point,
> but for 9.7) to rip out libpq's support for V2 protocol as well as
> pg_dump's support for pre-7.4 backends.

+1


> There might be an argument for moving pg_dump's cutoff further than that,
> but going to 7.3 or later is significant because it would allow removal of
> the kluges for schema-less and dependency-less servers.  I suggested 7.4
> because it'd comport with removal of V2 wire protocol support, and because
> 7.4 is also our cutoff for describe support in psql.

I think we can be a lot more aggressive moving the cuttoff for psql than
for pg_dump; but that's more an argument ripping out some old psql code.


> I'm hesitant to move the cutoff really far, because we do still hear from
> people running really old versions, and sooner or later those people will
> want to upgrade.  It'd be good if they could use a modern pg_dump for the
> purpose.

I think we should consider making the cutoff point for pg_dump somewhat
predicatable. E.g. saying that we support 5 more versions than the
actually maintained ones.   The likelihood of breakages seems to
increase a good bit for older versions.


Andres


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Alvaro Herrera]

Andrew Dunstan wrote:

> And if this is of any use, here are the dump differences from every live
> version to git tip, as of this morning.

Interesting, thanks.  I wonder if some of these diffs could be reduced
further by using pg_dump -Fd instead of a single text dump -- then
internal ordering would not matter, and I see that a large part of these
diffs is where GRANTs appear.  (I don't think it's a problem to use a
newer pg_dump to dump the older databases that don't support -Fd, for
this purpose.)

How would you recommend to run this in the coverage reporting machine?
Currently it's just doing "make check-world".  Could we add a buildfarm
script to run, standalone?

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


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2016-05-03 12:07:51 -0400, Tom Lane wrote:
> > I think possibly the easiest fix for this is to have pg_upgrade,
> > instead of RESETting a nonexistent option, RESET something that's
> > still considered to require AccessExclusiveLock.  "user_catalog_table"
> > would work, looks like; though I'd want to annotate its entry in
> > reloptions.c to warn people away from downgrading its lock level.
> 
> Alternatively we could just add a function for adding a toast table -
> that seems less hacky and less likely to be broken in the future.

+1

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Tom Lane]

Stephen Frost  writes:
> One other point is that pg_dump goes quite a bit farther back than just
> what we currently support (or at least, it tries to).  I think that,
> generally, that's a good thing, but it does mean we have a lot of cases
> that don't get tested nearly as much...

Yeah.  I do periodically fire up servers back to 7.0 and see if pg_dump
can dump from them, but I don't pretend that that's a very thorough test.

> I was able to get back to 7.4 up and running without too much trouble,
> but even that doesn't cover all the cases we have in pg_dump.  I'm not
> sure if we want to define a "we will support pg_dump back to X" cut-off
> or if we want to try and get older versions to run on modern systems,
> but it's definitely worth pointing out that we're trying to support much
> farther back than what is currently supported in pg_dump today.

I've been thinking of proposing that it's time (not now, at this point,
but for 9.7) to rip out libpq's support for V2 protocol as well as
pg_dump's support for pre-7.4 backends.  That's a quite significant
amount of what at this point is very poorly tested code.  And I doubt
that it would be productive to try to improve the test coverage rather
than just removing it.

There might be an argument for moving pg_dump's cutoff further than that,
but going to 7.3 or later is significant because it would allow removal of
the kluges for schema-less and dependency-less servers.  I suggested 7.4
because it'd comport with removal of V2 wire protocol support, and because
7.4 is also our cutoff for describe support in psql.

I'm hesitant to move the cutoff really far, because we do still hear from
people running really old versions, and sooner or later those people will
want to upgrade.  It'd be good if they could use a modern pg_dump for the
purpose.

regards, tom lane


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

Re: [HACKERS] what to revert

[Andres Freund]

On 2016-05-03 10:12:51 -0700, Peter Geoghegan wrote:
> On Tue, May 3, 2016 at 9:58 AM, Alvaro Herrera  
> wrote:
> > As its committer, I tend to agree about reverting that feature.  Craig
> > was just posting some more patches, and I have the pg_recvlogical
> > changes here (--endpos) which after some testing are not quite looking
> > ready to go -- plus we still have to write the actual Perl test scripts
> > that would use it.  Taken together, this is now looking to me a bit
> > rushed, so I prefer to cut my losses here and revert the patch so that
> > we can revisit it for 9.7.
> 
> I think it's a positive development that we can take this attitude to
> reverting patches. It should not be seen as a big personal failure,
> because it isn't. Stigmatizing reverts incentivizes behavior that
> leads to bad outcomes.

+1


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Andres Freund]

Hi,

On 2016-05-03 12:07:51 -0400, Tom Lane wrote:
> I think possibly the easiest fix for this is to have pg_upgrade,
> instead of RESETting a nonexistent option, RESET something that's
> still considered to require AccessExclusiveLock.  "user_catalog_table"
> would work, looks like; though I'd want to annotate its entry in
> reloptions.c to warn people away from downgrading its lock level.

Alternatively we could just add a function for adding a toast table -
that seems less hacky and less likely to be broken in the future.

- Andres


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Alvaro Herrera]

Stephen Frost wrote:

> One other point is that pg_dump goes quite a bit farther back than just
> what we currently support (or at least, it tries to).  I think that,
> generally, that's a good thing, but it does mean we have a lot of cases
> that don't get tested nearly as much...
> 
> I was able to get back to 7.4 up and running without too much trouble,
> but even that doesn't cover all the cases we have in pg_dump.  I'm not
> sure if we want to define a "we will support pg_dump back to X" cut-off
> or if we want to try and get older versions to run on modern systems,
> but it's definitely worth pointing out that we're trying to support much
> farther back than what is currently supported in pg_dump today.

Yeah.  Trying to compile old stuff with current tools (Debian jessie):

7.0's configure does not recognize my system:

checking host system type... Invalid configuration `x86_64-unknown-linux-gnu': 
machine `x86_64-unknown' not recognized

7.1's configure fails for accept() detection:

checking types of arguments for accept()... configure: error: could not 
determine argument types

7.2's configure works, but apparently it failed to find flex:

make[3]: Entering directory 
'/home/alvherre/Code/pgsql/source/throwaway/src/backend/bootstrap'
***
ERROR: `flex' is missing on your system. It is needed to create the
file `bootscanner.c'. You can either get flex from a GNU mirror site
or download an official distribution of PostgreSQL, which contains
pre-packaged flex output.
***
Makefile:60: recipe for target 'bootscanner.c' failed
make[3]: *** [bootscanner.c] Error 1


7.3 fails in ecpg preprocessor:

make[4]: Entering directory 
'/home/alvherre/Code/pgsql/source/throwaway/src/interfaces/ecpg/preproc'
make -C ../../../../src/port all
make[5]: Entering directory 
'/home/alvherre/Code/pgsql/source/throwaway/src/port'
make[5]: Nothing to be done for 'all'.
make[5]: Leaving directory '/home/alvherre/Code/pgsql/source/throwaway/src/port'
gcc -O2 -fno-strict-aliasing -Wall -Wmissing-prototypes -Wmissing-declarations 
-Wno-error -I./../include -I. -I../../../../src/include  -DMAJOR_VERSION=2 
-DMINOR_VERSION=10 -DPATCHLEVEL=0 -DINCLUDE_PATH=\"/usr/local/pgsql/include\"   
-c -o preproc.o preproc.c
In file included from preproc.y:5571:0:
pgc.c:170:18: error: conflicting types for ‘yyleng’
 extern yy_size_t yyleng;
  ^
In file included from preproc.y:7:0:
extern.h:32:4: note: previous declaration of ‘yyleng’ was here
yyleng;
^
In file included from preproc.y:5571:0:
pgc.c:304:11: error: conflicting types for ‘yyleng’
 yy_size_t yyleng;
   ^
In file included from preproc.y:7:0:
extern.h:32:4: note: previous declaration of ‘yyleng’ was here
yyleng;
^
In file included from preproc.y:5571:0:
pgc.c:2723:16: warning: ‘input’ defined but not used [-Wunused-function]
 static int input  (void)
^
: recipe for target 'preproc.o' failed
make[4]: *** [preproc.o] Error 1


7.4 seems to work fine.  I suppose it should be fine to remove pg_dump's
support for pre-7.3; people wanting to upgrade from before 7.3 (if any)
could use 9.6's pg_dump as an intermediate jump.

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


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

Re: [HACKERS] what to revert

[Andres Freund]

On 2016-05-03 11:46:23 -0500, Kevin Grittner wrote:
> > was immediately addressed by another round of benchmarks after you
> > pointed it out.
> 
> Which showed a 4% maximum hit before moving the test for whether it
> was "off" inline.

> (I'm not clear from the posted results whether that was before or
> after skipping the spinlock when the feature was off.)

They're from after the spinlock issue was resolved. Before that the
issue was a lot worse (see mail linked two messages upthread).


I'm pretty sure that I said that somewhere else at least once: But to be
absolutely clear, I'm *not* really concerned with the performance with
the feature turned off.  I'm concerned about the performance with it
turned on.


Andres


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Andrew Dunstan]

On 05/03/2016 01:33 PM, Andrew Dunstan wrote:



On 05/03/2016 01:28 PM, Andrew Dunstan wrote:



On 05/03/2016 01:21 PM, Stephen Frost wrote:

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:

Tom Lane wrote:


More generally, though, I wonder how we can have some test coverage
on such cases going forward.  Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the 
latest

stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...




I have an module that does it, although it's not really stable 
enough. But it's a big start.
See 




Incidentally, just as a warning for anyone trying, this uses up a 
quite a lot of disk space.


You would need several GB available.




And if this is of any use, here are the dump differences from every live 
version to git tip, as of this morning.



cheers

andrew



upgrade_dump_diff.tgz
Description: Unix tar archive

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

Re: [HACKERS] pg_upgrade and toasted pg_largeobject

[Tom Lane]

I wrote:
> Alvaro Herrera  writes:
>> I'm happy with the solution that pg_upgrade has a step in the check
>> stage that says "catalog XYZ has a toast table but shouldn't, aborting
>> the upgrade".  (Well, not _happy_, but at least it's a lot easier to
>> diagnose).

> I think though that you're defining the problem too narrowly by putting
> forward a solution that would result in an error message like that.
> If we're going to do anything here at all, I think the code should emit
> a list of the names of relations that it was unable to match up, rather
> than trying (and likely failing) to be smart about why.  To take just
> one reason why, the issue might be that there were too many rels in
> the new installation, not too few.

I took a break from release-note writing and hacked up something for
this; see attached patch series.

pg_upgrade-fix-0001.patch attempts to turn get_rel_infos()'s data
collection query into less of an unmaintainable mess.  In the current
code, the "regular_heap" CTE doesn't fetch heap OIDs: it collects some
index OIDs as well.  The "all_indexes" CTE doesn't fetch all index OIDs:
it only fetches OIDs for toast-table indexes (although this is far from
obvious to the naked eye, since it reads both the regular_heap and
toast_heap CTEs; it's only when you notice that it's subsequently ignoring
tables with zero reltoastrelid, and therefore that having read the
toast_heap CTE was totally useless, that you realize this).  Also it
forgets to check indisready, so it's fortunate that it doesn't fetch
anything but toast-table indexes, which are unlikely to be in !indisready
state.  Only the toast_heap CTE actually does what it says, though rather
inefficiently since it's uselessly looking for toast tables attached to
indexes.  The comments that aren't outright wrong are variously
misleading, repetitive, badly placed, and/or ungrammatical.  I'd like to
commit this even if we don't use the rest, because what's there now is not
a fit basis for further development.

pg_upgrade-fix-0002.patch expands the data collection query to collect
the OID of the table associated with each index, and the OID of the base
table for each toast table.  This isn't needed for pg_upgrade's other
processing but we need it in order to usefully identify mismatched tables.

pg_upgrade-fix-0003.patch actually changes gen_db_file_maps() so that
it prints identifying information about each unmatched relation.

pg_upgrade-fix-0004.patch isn't meant to get committed; it's for testing
the previous patches.  It hacks the pg_upgrade test script to cause
pg_largeobject in the source DB to acquire a toast table, thereby
replicating the situation Alvaro complained of.  With that hack in place,
I get

...
Copying user relation files
No match found in new cluster for old relation with OID 13270 in database 
"postgres": "pg_toast.pg_toast_2613", toast table for 
"pg_catalog.pg_largeobject"
No match found in new cluster for old relation with OID 13272 in database 
"postgres": "pg_toast.pg_toast_2613_index", index on "pg_toast.pg_toast_2613", 
toast table for "pg_catalog.pg_largeobject"

Failed to match up old and new tables in database "postgres"
Failure, exiting
+ rm -rf /tmp/pg_upgrade_check-Lui8zH
make: *** [check] Error 1

which illustrates the output this will produce for a mismatch.  (If anyone
wants to bikeshed the output wording, feel free.)

Any thoughts what to do with this?  We could decide that it's a bug fix
and backpatch, or decide that it's a new feature and delay till 9.7,
or decide that it's a minor bug fix and add it to 9.6 only.  I kinda lean
towards the last alternative.

regards, tom lane

diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index 5d83455..0653ff1 100644
*** a/src/bin/pg_upgrade/info.c
--- b/src/bin/pg_upgrade/info.c
*** get_db_infos(ClusterInfo *cluster)
*** 301,311 
  /*
   * get_rel_infos()
   *
!  * gets the relinfos for all the user tables of the database referred
!  * by "db".
   *
!  * NOTE: we assume that relations/entities with oids greater than
!  * FirstNormalObjectId belongs to the user
   */
  static void
  get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
--- 301,311 
  /*
   * get_rel_infos()
   *
!  * gets the relinfos for all the user tables and indexes of the database
!  * referred to by "dbinfo".
   *
!  * Note: the resulting RelInfo array is assumed to be sorted by OID.
!  * This allows later processing to match up old and new databases efficiently.
   */
  static void
  get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
*** get_rel_infos(ClusterInfo *cluster, DbIn
*** 330,415 
  	char	   *last_namespace = NULL,
  			   *last_tablespace = NULL;
  
  	/*
  	 * pg_largeobject contains user data that does not appear in pg_dump
! 	 * --schema-only output, so we have to copy that system table heap and
! 	 * index.  We could grab the pg_largeobject oids from template1, but it is
! 	 * easy to treat it as a n

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Andrew Dunstan]

On 05/03/2016 01:28 PM, Andrew Dunstan wrote:



On 05/03/2016 01:21 PM, Stephen Frost wrote:

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:

Tom Lane wrote:


More generally, though, I wonder how we can have some test coverage
on such cases going forward.  Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the 
latest

stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...




I have an module that does it, although it's not really stable enough. 
But it's a big start.
See 




Incidentally, just as a warning for anyone trying, this uses up a quite 
a lot of disk space.


You would need several GB available.

cheers

andrew





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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > > Tom Lane wrote:
> > > 
> > > > More generally, though, I wonder how we can have some test coverage
> > > > on such cases going forward.  Is the patch below too ugly to commit
> > > > permanently, and if so, what other idea can you suggest?
> > > 
> > > I suggest a buildfarm animal running a custom buildfarm module that
> > > exercises the pg_upgrade test from every supported version to the latest
> > > stable and to master -- together with your proposed case that leaves a
> > > toastless table around for pg_upgrade to handle.
> > 
> > That would help greatly with pg_dump test coverage as well..  One of the
> > problems of trying to get good LOC coverage of pg_dump is that a *lot*
> > of the code is version-specific...
> 
> If we can put together a script that runs test.sh for various versions
> and then verifies the runs, we could use it in both buildfarm and
> coverage.

One other point is that pg_dump goes quite a bit farther back than just
what we currently support (or at least, it tries to).  I think that,
generally, that's a good thing, but it does mean we have a lot of cases
that don't get tested nearly as much...

I was able to get back to 7.4 up and running without too much trouble,
but even that doesn't cover all the cases we have in pg_dump.  I'm not
sure if we want to define a "we will support pg_dump back to X" cut-off
or if we want to try and get older versions to run on modern systems,
but it's definitely worth pointing out that we're trying to support much
farther back than what is currently supported in pg_dump today.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Andrew Dunstan]

On 05/03/2016 01:21 PM, Stephen Frost wrote:

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:

Tom Lane wrote:


More generally, though, I wonder how we can have some test coverage
on such cases going forward.  Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well..  One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...




I have an module that does it, although it's not really stable enough. 
But it's a big start.
See 



cheers

andrew



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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Tom Lane wrote:
> 
> > More generally, though, I wonder how we can have some test coverage
> > on such cases going forward.  Is the patch below too ugly to commit
> > permanently, and if so, what other idea can you suggest?
> 
> I suggest a buildfarm animal running a custom buildfarm module that
> exercises the pg_upgrade test from every supported version to the latest
> stable and to master -- together with your proposed case that leaves a
> toastless table around for pg_upgrade to handle.

That would help greatly with pg_dump test coverage as well..  One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Alvaro Herrera]

Stephen Frost wrote:
> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > Tom Lane wrote:
> > 
> > > More generally, though, I wonder how we can have some test coverage
> > > on such cases going forward.  Is the patch below too ugly to commit
> > > permanently, and if so, what other idea can you suggest?
> > 
> > I suggest a buildfarm animal running a custom buildfarm module that
> > exercises the pg_upgrade test from every supported version to the latest
> > stable and to master -- together with your proposed case that leaves a
> > toastless table around for pg_upgrade to handle.
> 
> That would help greatly with pg_dump test coverage as well..  One of the
> problems of trying to get good LOC coverage of pg_dump is that a *lot*
> of the code is version-specific...

If we can put together a script that runs test.sh for various versions
and then verifies the runs, we could use it in both buildfarm and
coverage.

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


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

Re: [HACKERS] what to revert

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> Here's a list of what I think is currently broken in 9.6 that we might
> conceivably fix by reverting patches:
[...]
> - Predefined Roles.  Neither you nor I liked Stephen's design.  It
> slowed down pg_dump.  It also broke pg_dump for non-superusers and
> something about bypassrls.  None of these issues have been fixed
> despite considerable time having gone by.

The issues you list are not with predefined roles at all, but with the
changes to dump ACLs defined against objects in pg_catalog.  I've also
got patches to address those issues already developed and (mostly)
posted.  I'll be posting a new set later today which addresses all of
the known issues with dumping catalog ACLs.

There is an ongoing thread where Noah and I have been discussing the
dumping of catalog ACLs issues and the TAP test suite which I've been
developing for pg_dump.  Certainly, anyone is welcome to join in that
discussion.

As mentioned before, the concern raised about predefined roles are the
checks which were added to make them unlike regular roles.  I'll be
posting a patch tomorrow to revert those checks, as I mentioned on
another thread earlier today.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] what to revert

[Peter Geoghegan]

On Tue, May 3, 2016 at 9:58 AM, Alvaro Herrera  wrote:
> As its committer, I tend to agree about reverting that feature.  Craig
> was just posting some more patches, and I have the pg_recvlogical
> changes here (--endpos) which after some testing are not quite looking
> ready to go -- plus we still have to write the actual Perl test scripts
> that would use it.  Taken together, this is now looking to me a bit
> rushed, so I prefer to cut my losses here and revert the patch so that
> we can revisit it for 9.7.

I think it's a positive development that we can take this attitude to
reverting patches. It should not be seen as a big personal failure,
because it isn't. Stigmatizing reverts incentivizes behavior that
leads to bad outcomes.

-- 
Peter Geoghegan


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

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Alvaro Herrera]

Tom Lane wrote:

> More generally, though, I wonder how we can have some test coverage
> on such cases going forward.  Is the patch below too ugly to commit
> permanently, and if so, what other idea can you suggest?

I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.

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


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

Re: [HACKERS] what to revert

[Alvaro Herrera]

Andres Freund wrote:

> I'm *really* doubtful about the logical timeline following patches. I
> think they're, as committed, over-complicated and pretty much unusable.

As its committer, I tend to agree about reverting that feature.  Craig
was just posting some more patches, and I have the pg_recvlogical
changes here (--endpos) which after some testing are not quite looking
ready to go -- plus we still have to write the actual Perl test scripts
that would use it.  Taken together, this is now looking to me a bit
rushed, so I prefer to cut my losses here and revert the patch so that
we can revisit it for 9.7.

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


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

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Tue, May 3, 2016 at 11:48 AM, Robert Haas  wrote:

> OK, I see now: the basic idea here is that we can't prune based on the
> newer XID unless the page LSN is guaranteed to advance whenever data
> is removed.  Currently, we attempt to limit bloat in non-unlogged,
> non-catalog tables.  You're saying we can instead attempt to limit
> bloat only in non-unlogged, non-catalog tables without hash indexes,
> and that will fix this issue.  Am I right?

Right.

I was wondering whether there might be other avenues to the same
end, but that is the most obvious fix.  I'm hesitant to raise the
alternatives because people seem to have entered "panic mode", at
which point alternatives always look scary.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Robert Haas]

On Tue, May 3, 2016 at 12:17 PM, Kevin Grittner  wrote:
> On Tue, May 3, 2016 at 11:09 AM, Robert Haas  wrote:
>> On Tue, May 3, 2016 at 11:46 AM, Kevin Grittner  wrote:
 Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
 you?
>>>
>>> Yes, I see three ways, the most obvious of which is what Amit
>>> suggested -- don't do early vacuum on a table which has a hash index.
>>
>> What do you mean by "early VACUUM"?
>
> Both vacuuming and hot-pruning adjust xmin based on calling
> TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
> Relation relation).  I'm talking about having that function, if all
> other conditions for the override pass, checking for a hash index,
> too.
>
>> Amit suggested disabling
>> HOT-pruning, but HOT-pruning happens completely outside of VACUUM.  It
>> also happens inside VACUUM, so if we disabled HOT pruning, how could
>> we VACUUM at all?  Sorry, I am confused.
>
> I guess we were both talking a bit loosely since (as I mentioned
> above) the function that adjusts the xmin is called for a vacuum or
> pruning.  He mentioned one and I mentioned the other, but it's all
> controlled by TransactionIdLimitedForOldSnapshots().

OK, I see now: the basic idea here is that we can't prune based on the
newer XID unless the page LSN is guaranteed to advance whenever data
is removed.  Currently, we attempt to limit bloat in non-unlogged,
non-catalog tables.  You're saying we can instead attempt to limit
bloat only in non-unlogged, non-catalog tables without hash indexes,
and that will fix this issue.  Am I right?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] what to revert

[Kevin Grittner]

On Tue, May 3, 2016 at 11:22 AM, Andres Freund  wrote:

> The issue with 0 v. -1 (and 0 vs. > 0 makes a big performance
> difference, so it's not that surprising to interpret numbers that way)

... if you fail to read the documentation of the feature or the
code implementing it before testing.

> was immediately addressed by another round of benchmarks after you
> pointed it out.

Which showed a 4% maximum hit before moving the test for whether it
was "off" inline.  (I'm not clear from the posted results whether
that was before or after skipping the spinlock when the feature was
off.)  All tests that I have done and that others have done (some
on big NUMA machines) and shared with me show no regression now.  I
haven't been willing to declare victory on that basis without
hearing back from others who were able to show a regression before.
If there is still a regression found when "off", there is one more
test of old_snapshot_threshold which could easily be shifted
in-line; it just seems unnecessary given the other work done in
that area unless there is evidence that it is really needed.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] what to revert

[Robert Haas]

On Tue, May 3, 2016 at 12:22 PM, Andres Freund  wrote:
>> > but that might be fixed now.
>>
>> Certainly all evidence suggests that, FUD to the contrary.
>
> So it's now FUD to report issues with a patch that obviously hasn't
> received sufficient benchmarking? Give me break.

Yeah, I don't think that's FUD.  Kevin, since your last fix, we don't
have a round of benchmarking on a big machine to show whether that
fixed the issue or not.  I think that to really know whether this is
fixed, somebody would need to compare current master with current
master after reverting snapshot too old on a big machine and see if
there's a difference.  If anyone has done that, they have not posted
the results.  So it's more accurate to say that we just don't know.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] what to revert

[Andres Freund]

On 2016-05-03 11:12:03 -0500, Kevin Grittner wrote:
> On Tue, May 3, 2016 at 10:58 AM, Robert Haas  wrote:
> 
> > - Snapshot Too Old.  Tom, Andres, and Bruce want this reverted.
> > It regresses performance significantly when turned on.
> 
> When turned on, it improves performance in some cases and regresses
> performance in others.  Don't forget it is currently back-patched
> to 9.4 and in use for production by users who could not use
> PostgreSQL without the feature.  PostgreSQL failed their
> performance tests miserably without the feature, and passes with
> it.
> 
> > It originally regressed performance significantly even when
> > turned off,
> 
> Which was wildly exaggerated since most of the benchmarks
> purporting to show that actually had it turned on.  I don't think
> the FUD from that has really evaporated.

Oh, ffs.  The first report of the whole issue was *with default
parameters*:
http://archives.postgresql.org/message-id/CAPpHfdtMONZFOXSsw1HkrD9Eb4ozF8Q8oCqH4tkpH_girJPPuA%40mail.gmail.com

The issue with 0 v. -1 (and 0 vs. > 0 makes a big performance
difference, so it's not that surprising to interpret numbers that way)
was immediately addressed by another round of benchmarks after you
pointed it out.

> > but that might be fixed now.
> 
> Certainly all evidence suggests that, FUD to the contrary.

So it's now FUD to report issues with a patch that obviously hasn't
received sufficient benchmarking? Give me break.


Andres


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

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Tue, May 3, 2016 at 11:09 AM, Robert Haas  wrote:
> On Tue, May 3, 2016 at 11:46 AM, Kevin Grittner  wrote:
>>> Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
>>> you?
>>
>> Yes, I see three ways, the most obvious of which is what Amit
>> suggested -- don't do early vacuum on a table which has a hash index.
>
> What do you mean by "early VACUUM"?

Both vacuuming and hot-pruning adjust xmin based on calling
TransactionIdLimitedForOldSnapshots(TransactionId recentXmin,
Relation relation).  I'm talking about having that function, if all
other conditions for the override pass, checking for a hash index,
too.

> Amit suggested disabling
> HOT-pruning, but HOT-pruning happens completely outside of VACUUM.  It
> also happens inside VACUUM, so if we disabled HOT pruning, how could
> we VACUUM at all?  Sorry, I am confused.

I guess we were both talking a bit loosely since (as I mentioned
above) the function that adjusts the xmin is called for a vacuum or
pruning.  He mentioned one and I mentioned the other, but it's all
controlled by TransactionIdLimitedForOldSnapshots().

> Doesn't this issue also affected indexes on any unlogged table?

That's been covered all along.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Processes and caches in postgresql

[Petr Jelinek]

On 03/05/16 16:35, Craig Ringer wrote:

On 3 May 2016 at 21:37, Merlin Moncure mailto:mmonc...@gmail.com>> wrote:


There is library out there, unfortunately GPL licensed, that attempts
to fully implement posix including fork(): http://midipix.org/.  One
of these days I'd like to have a go at porting postgres to it.


... and here I thought you'd be keen to instead remove all use of
globals and 'static' locals to allow thread-safe state tracking, remove
use of shmem, introduce threaded backends, replace use of signals, and
release ThreadedPostgres.

Sounds fun, right? :p

More seriously, shouldn't Microsoft's new (or at least re-blessed and
re-released with a new paint job) Linux/POSIX support offer us some
options here? I suspect not - they're probably restricted to ELF
binaries that won't be able to link to native Windows DLLs to get
support for things like SSPI auth, native Windows SSL API use, etc. But
it's worth keeping the possibility in mind.



Yes the linux emulation is restricted to it's own world, it can't access 
any windows native binaries.


The other thing is that MS repeatedly said that they plan this as purely 
developer feature and you should not use it for deploying anything.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


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

Re: [HACKERS] what to revert

[Andres Freund]

On 2016-05-03 11:58:34 -0400, Robert Haas wrote:
> - Atomic Pin/Unpin.  There are two different open items related to
> this, both apparently relating to testing done by Jeff Janes.  I am
> not sure whether they are really independent reports of different
> problems or just two reports of the same problem.  But they sound like
> fairly serious breakage.

It's a bit hard to say, given the test take this long to run, but so far
I've a fair amount of doubt that the bug report is actually related to
the atomic pin/unpin.  It appears to me - I'm in the process of trying
to confirm (again long runs) that the pin/unpin patch merely changed the
timing.


> - Checkpoint Sorting and Flushing.  Andres tried to fix the last
> report of problems with this in
> 72a98a639574d2e25ed94652848555900c81a799, but there were almost
> immediately new reports.

Yea, it's an issue with the 72a98a639574d2e25ed94652848555900c81a799,
not checkpoint flushing itself. Turns out that mdsync() *wants/needs* to
access deleted segments. Easily enough fixed. I've posted a patch, which
I plan to commit unless somebody wants to give input on the flag design.


> There are a few other open items, but I would consider reverting the
> antecedent patches as either impractical or disproportionate in those
> cases.  Aside from the two cases you mentioned, I don't think that
> anyone's actually called for these other patches to be reverted, but
> I'm not sure that we shouldn't be considering it.  What do you (and
> others) think?

I'm *really* doubtful about the logical timeline following patches. I
think they're, as committed, over-complicated and pretty much unusable.

Greetings,

Andres Freund


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

Re: [HACKERS] what to revert

[Kevin Grittner]

On Tue, May 3, 2016 at 10:58 AM, Robert Haas  wrote:

> - Snapshot Too Old.  Tom, Andres, and Bruce want this reverted.
> It regresses performance significantly when turned on.

When turned on, it improves performance in some cases and regresses
performance in others.  Don't forget it is currently back-patched
to 9.4 and in use for production by users who could not use
PostgreSQL without the feature.  PostgreSQL failed their
performance tests miserably without the feature, and passes with
it.

> It originally regressed performance significantly even when
> turned off,

Which was wildly exaggerated since most of the benchmarks
purporting to show that actually had it turned on.  I don't think
the FUD from that has really evaporated.

> but that might be fixed now.

Certainly all evidence suggests that, FUD to the contrary.

> Also, it seems to be broken for hash indexes, per Amit Kapila's
> report.

Yeah, with a fairly simple fix suggested immediately by Amit.  I'm
looking into a couple other angles for possible fixes, but
certainly what he suggested could be done before beta1.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Robert Haas]

On Tue, May 3, 2016 at 11:46 AM, Kevin Grittner  wrote:
>> Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
>> you?
>
> Yes, I see three ways, the most obvious of which is what Amit
> suggested -- don't do early vacuum on a table which has a hash index.

What do you mean by "early VACUUM"?  Amit suggested disabling
HOT-pruning, but HOT-pruning happens completely outside of VACUUM.  It
also happens inside VACUUM, so if we disabled HOT pruning, how could
we VACUUM at all?  Sorry, I am confused.

Doesn't this issue also affected indexes on any unlogged table?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

[HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Tom Lane]

There is logic in pg_upgrade plus the backend, mostly added by commit
4c6780fd1, to cope with the corner cases that sometimes arise where the
old and new versions have different ideas about whether a given table
needs a TOAST table.  The more common case is where there's a TOAST table
in the old DB, but (perhaps as a result of having dropped all the wide
columns) the new cluster doesn't think the table definition requires a
TOAST table.  The reverse is also possible, although according to the
existing code comments it can only happen when upgrading from pre-9.1.
The way pg_upgrade handles that case is that after running all the
table creation operations it issues this command:

PQclear(executeQueryOrDie(conn, "ALTER TABLE %s.%s RESET 
(binary_upgrade_dummy_option);",
 quote_identifier(PQgetvalue(res, rowno, i_nspname)),
   quote_identifier(PQgetvalue(res, rowno, i_relname;

which doesn't actually do anything (no such reloption being set) but
nonetheless triggers a call of AlterTableCreateToastTable, which
will cause a toast table to be created if the new server thinks the
table definition requires one.

Or at least, it did until Simon decided that ALTER TABLE RESET
doesn't require AccessExclusiveLock.  Now you get a failure.
I haven't tried to construct a pre-9.1 database that would trigger
this, but you can make it happen by applying the attached patch
to create a toast-table-less table in the regression tests,
and then doing "make check" in src/bin/pg_upgrade.  You get this:

...
Restoring database schemas in the new cluster
ok
Creating newly-required TOAST tablesSQL command failed
ALTER TABLE "public"."i_once_had_a_toast_table" RESET 
(binary_upgrade_dummy_option);
ERROR:  AccessExclusiveLock required to add toast table.

Failure, exiting
+ rm -rf /tmp/pg_upgrade_check-o0CUMm
make: *** [check] Error 1


I think possibly the easiest fix for this is to have pg_upgrade,
instead of RESETting a nonexistent option, RESET something that's
still considered to require AccessExclusiveLock.  "user_catalog_table"
would work, looks like; though I'd want to annotate its entry in
reloptions.c to warn people away from downgrading its lock level.

More generally, though, I wonder how we can have some test coverage
on such cases going forward.  Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?

regards, tom lane

diff --git a/src/test/regress/expected/indirect_toast.out b/src/test/regress/expected/indirect_toast.out
index 4f4bf41..ad7127d 100644
*** a/src/test/regress/expected/indirect_toast.out
--- b/src/test/regress/expected/indirect_toast.out
*** SELECT substring(toasttest::text, 1, 200
*** 149,151 
--- 149,158 
  
  DROP TABLE toasttest;
  DROP FUNCTION update_using_indirect();
+ -- Create a table that has a toast table, then modify it so it appears
+ -- not to have one, and leave it behind after the regression tests end.
+ -- This enables testing of this scenario for pg_upgrade.
+ create table i_once_had_a_toast_table(f1 int, f2 text);
+ insert into i_once_had_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+   where relname = 'i_once_had_a_toast_table';
diff --git a/src/test/regress/sql/indirect_toast.sql b/src/test/regress/sql/indirect_toast.sql
index d502480..cefbd0b 100644
*** a/src/test/regress/sql/indirect_toast.sql
--- b/src/test/regress/sql/indirect_toast.sql
*** SELECT substring(toasttest::text, 1, 200
*** 59,61 
--- 59,69 
  
  DROP TABLE toasttest;
  DROP FUNCTION update_using_indirect();
+ 
+ -- Create a table that has a toast table, then modify it so it appears
+ -- not to have one, and leave it behind after the regression tests end.
+ -- This enables testing of this scenario for pg_upgrade.
+ create table i_once_had_a_toast_table(f1 int, f2 text);
+ insert into i_once_had_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+   where relname = 'i_once_had_a_toast_table';

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

[HACKERS] what to revert

[Robert Haas]

On Tue, May 3, 2016 at 11:36 AM, Tom Lane  wrote:
>> There are a lot more than 2 patchsets that are busted at the moment,
>> unfortunately, but I assume you are referring to "snapshot too old"
>> and "Use Foreign Key relationships to infer multi-column join
>> selectivity".
>
> Yeah, those are the ones I'm thinking of.  I've not heard serious
> proposals to revert any others, have you?

Here's a list of what I think is currently broken in 9.6 that we might
conceivably fix by reverting patches:

- Snapshot Too Old.  Tom, Andres, and Bruce want this reverted.  It
regresses performance significantly when turned on.  It originally
regressed performance significantly even when turned off, but that
might be fixed now.  Also, it seems to be broken for hash indexes, per
Amit Kapila's report.

- Atomic Pin/Unpin.  There are two different open items related to
this, both apparently relating to testing done by Jeff Janes.  I am
not sure whether they are really independent reports of different
problems or just two reports of the same problem.  But they sound like
fairly serious breakage.

- Predefined Roles.  Neither you nor I liked Stephen's design.  It
slowed down pg_dump.  It also broke pg_dump for non-superusers and
something about bypassrls.  None of these issues have been fixed
despite considerable time having gone by.

- Checkpoint Sorting and Flushing.  Andres tried to fix the last
report of problems with this in
72a98a639574d2e25ed94652848555900c81a799, but there were almost
immediately new reports.

- Foreign Keys and Multi-Column Outer Joins.  You called for a revert,
and the author responded with various thoughts and counterproposals.

There are a few other open items, but I would consider reverting the
antecedent patches as either impractical or disproportionate in those
cases.  Aside from the two cases you mentioned, I don't think that
anyone's actually called for these other patches to be reverted, but
I'm not sure that we shouldn't be considering it.  What do you (and
others) think?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] psql :: support for \ev viewname and \sv viewname

[Peter Eisentraut]

On 5/2/16 8:53 AM, Dean Rasheed wrote:

Here are updated patches doing that --- the first moves
fmtReloptionsArray() from pg_dump.c to fe_utils/string_utils.c so that
it can be shared by pg_dump and psql, and renames it to
appendReloptionsArray(). The second patch fixes the actual psql bug.


This looks reasonable.

I would change appendReloptionsArrayAH() to a function and keep AH as 
the first argument (similar to other functions that take such a handle).


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Tue, May 3, 2016 at 10:45 AM, Bruce Momjian  wrote:
> On Mon, May  2, 2016 at 04:02:35PM -0500, Kevin Grittner wrote:
>> On Sun, May 1, 2016 at 1:43 AM, Amit Kapila  wrote:
>> > On Sun, May 1, 2016 at 12:05 PM, Amit Kapila  
>> > wrote:
>> >>
>> >> Currently we do the test for old snapshot (TestForOldSnapshot) for hash
>> >> indexes while scanning them.  Does this test makes any sense for hash
>> >> indexes considering LSN on hash index will always be zero (as hash indexes
>> >> are not WAL-logged)?  It seems to me that PageLSN check in
>> >> TestForOldSnapshot() will always return false which means that the error
>> >> "snapshot too old" won't be generated for hash indexes.
>> >>
>> >> Am I missing something here, if not, then I think we need a way to
>> >> prohibit pruning for hash indexes based on old_snapshot_threshold?
>> >
>> > What I mean to say here is prohibit pruning the relation which has hash
>> > index based on old_snapshot_threshold.
>>
>> Good spot; added to the open issues page.
>
> Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
> you?

Yes, I see three ways, the most obvious of which is what Amit
suggested -- don't do early vacuum on a table which has a hash index.

-- 
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] old_snapshot_threshold's interaction with hash index

[Bruce Momjian]

On Mon, May  2, 2016 at 04:02:35PM -0500, Kevin Grittner wrote:
> On Sun, May 1, 2016 at 1:43 AM, Amit Kapila  wrote:
> > On Sun, May 1, 2016 at 12:05 PM, Amit Kapila  
> > wrote:
> >>
> >> Currently we do the test for old snapshot (TestForOldSnapshot) for hash
> >> indexes while scanning them.  Does this test makes any sense for hash
> >> indexes considering LSN on hash index will always be zero (as hash indexes
> >> are not WAL-logged)?  It seems to me that PageLSN check in
> >> TestForOldSnapshot() will always return false which means that the error
> >> "snapshot too old" won't be generated for hash indexes.
> >>
> >> Am I missing something here, if not, then I think we need a way to
> >> prohibit pruning for hash indexes based on old_snapshot_threshold?
> >
> > What I mean to say here is prohibit pruning the relation which has hash
> > index based on old_snapshot_threshold.
> 
> Good spot; added to the open issues page.

Uh, I have no idea how this would be fixed if the PageLSN is zero.  Do
you?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Andres Freund]

On 2016-05-02 10:32:28 -0400, Robert Haas wrote:
> You are certainly welcome to add a new open item to cover those
> complaints.

Done that.

> But I do not want to blur together the discussion of
> whether the feature is well-designed with the question of whether it
> regresses performance when it is turned off.  Those are severable
> issues, meriting separate discussion (and probably separate threads).

The current thread already contains a lot of information about both, so
separating doesn't seem beneficial.

Greetings,

Andres Freund


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

Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> On Tue, May 3, 2016 at 11:23 AM, Tom Lane  wrote:
>> Well, there are at least two patchsets we're actively discussing
>> reverting, so I think this should wait till those decisions are resolved.

> OK, but that may well mean we don't get this done before beta1, which
> I think is a bummer, but oh well.

pgindent is reliable enough that I'm not really worried about running
it post-beta.  Obviously sooner is better than later, to give authors
of 9.7 patches more time to rebase; but I do not think we should give
ourselves extra work just to do it a little sooner.

> There are a lot more than 2 patchsets that are busted at the moment,
> unfortunately, but I assume you are referring to "snapshot too old"
> and "Use Foreign Key relationships to infer multi-column join
> selectivity".

Yeah, those are the ones I'm thinking of.  I've not heard serious
proposals to revert any others, have you?

regards, tom lane


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

Re: [HACKERS] pgindent fixups

[Robert Haas]

On Tue, May 3, 2016 at 11:23 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> OK, I committed this.  Barring objections, I'll go ahead and pgindent
>> the whole tree tomorrow.  If we're going to revert anything big then
>> we might want to hold off, but otherwise I think its better to get
>> this done sooner rather than later.
>
> Well, there are at least two patchsets we're actively discussing
> reverting, so I think this should wait till those decisions are resolved.

OK, but that may well mean we don't get this done before beta1, which
I think is a bummer, but oh well.

There are a lot more than 2 patchsets that are busted at the moment,
unfortunately, but I assume you are referring to "snapshot too old"
and "Use Foreign Key relationships to infer multi-column join
selectivity".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] pgindent fixups

[Tom Lane]

Robert Haas  writes:
> OK, I committed this.  Barring objections, I'll go ahead and pgindent
> the whole tree tomorrow.  If we're going to revert anything big then
> we might want to hold off, but otherwise I think its better to get
> this done sooner rather than later.

Well, there are at least two patchsets we're actively discussing
reverting, so I think this should wait till those decisions are resolved.

regards, tom lane


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

Re: [HACKERS] Pg_stop_backup process does not run - Backup Intervals

[Robert Haas]

On Mon, May 2, 2016 at 3:03 PM, Rodrigo Cavalcante
 wrote:
> On alternate days my backup is failing, by the pg_stop_backup process ()
> does not perform or quit.
>
> Version PostgreSQL: 9.1.6
>
> The following backup script:
>
> PGDATA=/dados
> SAVE_BASE_DIR=/backup/diario
> backup="'bkpfull'"
> data=$(date +'%d%m%y')
>
> WAL_DIR=/backup/archive
>
> mv $WAL_DIR/* $WAL_DIR/wal_anterior
>
> psql -U postgres -h localhost -c 'select pg_start_backup('$backup',true);'
> template1 && \
>
> tar -czvf $SAVE_BASE_DIR/$data.tar.gz $PGDATA && psql -U postgres -h
> localhost -c 'select pg_stop_backup();' template1

I'm going to refer you to
https://wiki.postgresql.org/wiki/Guide_to_reporting_problems

You aren't being very clear about what actually happens here, and you
haven't mentioned what showed up in the logs, either.  If I had to
guess, I'd guess that if you look at the output of this backup script
on alternate days, you'll find that some step in here is failing on
alternate days, with an error message to tell you which step is
failing and in what way.  That might help you figure out the problem.

Also, consider using one of the various backup toolkits that already
exist instead of writing your own script.  Also, consider using
pg_basebackup.  These tend to be more reliably than home-grown
scripts.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] pg_dump broken for non-super user

[Robert Haas]

On Tue, May 3, 2016 at 10:57 AM, Stephen Frost  wrote:
> * Craig Ringer (cr...@2ndquadrant.com) wrote:
>> On 3 May 2016 at 19:04, Rushabh Lathia  wrote:
>>
>> > With commit a9f0e8e5a2e779a888988cb64479a6723f668c84, now pg_dump, use a
>> > bitmap
>> > to represent what to include. With this commit if non-super user is unable
>> > to perform the dump.
>> >
>>
>> Hm. I think we need a src/bin/pg_dump/t/ test for the TAP suite for this
>> one, because something this fundamental should really have blown the
>> buildfarm up when committed.
>
> I'm in the process of building a pretty comprehensive set, as discussed
> in another thread.

Are you going to review and commit this patch?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas  wrote:
> > On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:
> >> Based on our discussion at PGConf.US and the comments up-thread from
> >> Tom, I'll work up a patch to remove those checks around SET ROLE and
> >> friends which were trying to prevent default roles from possibly being
> >> made to own objects.
> >>
> >> Should the checks, which have been included since nearly the start of
> >> this version of the patch, to prevent users from GRANT'ing other rights
> >> to the default roles remain?  Or should those also be removed?  I
> >> *think* pg_dump/pg_upgrade would be fine with rights being added, and if
> >> we aren't preventing ownership of objects then we aren't going to be
> >> able to remove such roles in any case.
> >
> > It'd be good to test that that works.  If it does, I think we may as
> > well allow it.
> >
> >> Of course, with these default roles, users can't REVOKE the rights which
> >> are granted to them as that happens in C code, outside of the GRANT
> >> system.
> >
> > I think you mean that they can't revoke the special magic rights, but
> > they could revoke any additional privileges which were granted.
> >
> >> Working up a patch to remove these checks should be pretty quickly done
> >> (iirc, I've actually got an independent patch around from when I added
> >> them, just need to find it and then go through the committed patches to
> >> make sure I take care of everything), but would like to make sure that
> >> we're now all on the same page and that *all* of these checks should be
> >> removed, making default roles just exactly like "regular" roles, except
> >> that they're created at initdb time and have "special" rights provided
> >> by C-level code checks.
> >
> > That's what I'm thinking.  I would welcome other views.
> 
> Ping!

Thanks.  I'm planning to post a patch tomorrow to remove these checks.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_dump broken for non-super user

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, May 3, 2016 at 10:57 AM, Stephen Frost  wrote:
> > * Craig Ringer (cr...@2ndquadrant.com) wrote:
> >> On 3 May 2016 at 19:04, Rushabh Lathia  wrote:
> >>
> >> > With commit a9f0e8e5a2e779a888988cb64479a6723f668c84, now pg_dump, use a
> >> > bitmap
> >> > to represent what to include. With this commit if non-super user is 
> >> > unable
> >> > to perform the dump.
> >> >
> >>
> >> Hm. I think we need a src/bin/pg_dump/t/ test for the TAP suite for this
> >> one, because something this fundamental should really have blown the
> >> buildfarm up when committed.
> >
> > I'm in the process of building a pretty comprehensive set, as discussed
> > in another thread.
> 
> Are you going to review and commit this patch?

I've already reviewed and commented on why that patch isn't a good idea,
please see my other email.

I'll be posting the updated set of patches on the other thread shortly
which address the performance concerns previously raised about
(relatively) empty databases and the TAP test suite that I've been
building.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas  wrote:
> On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:
>> Based on our discussion at PGConf.US and the comments up-thread from
>> Tom, I'll work up a patch to remove those checks around SET ROLE and
>> friends which were trying to prevent default roles from possibly being
>> made to own objects.
>>
>> Should the checks, which have been included since nearly the start of
>> this version of the patch, to prevent users from GRANT'ing other rights
>> to the default roles remain?  Or should those also be removed?  I
>> *think* pg_dump/pg_upgrade would be fine with rights being added, and if
>> we aren't preventing ownership of objects then we aren't going to be
>> able to remove such roles in any case.
>
> It'd be good to test that that works.  If it does, I think we may as
> well allow it.
>
>> Of course, with these default roles, users can't REVOKE the rights which
>> are granted to them as that happens in C code, outside of the GRANT
>> system.
>
> I think you mean that they can't revoke the special magic rights, but
> they could revoke any additional privileges which were granted.
>
>> Working up a patch to remove these checks should be pretty quickly done
>> (iirc, I've actually got an independent patch around from when I added
>> them, just need to find it and then go through the committed patches to
>> make sure I take care of everything), but would like to make sure that
>> we're now all on the same page and that *all* of these checks should be
>> removed, making default roles just exactly like "regular" roles, except
>> that they're created at initdb time and have "special" rights provided
>> by C-level code checks.
>
> That's what I'm thinking.  I would welcome other views.

Ping!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] pg_dump broken for non-super user

[Stephen Frost]

* Rushabh Lathia (rushabh.lat...@gmail.com) wrote:
> With commit a9f0e8e5a2e779a888988cb64479a6723f668c84, now pg_dump, use a
> bitmap
> to represent what to include. With this commit if non-super user is unable
> to perform the dump.
[...]
> pg_dump: [archiver (db)] query was: LOCK TABLE pg_catalog.pg_authid IN
> ACCESS SHARE MODE

This does get addressed, in a way, in one of the patches that I've
currently got pending for pg_dump.  That particular change is actually
one for performance and therefore it's only going to help in cases where
none of the catalog tables have had their ACLs changed.

If the ACL has changed for any catalog table then pg_dump will still try
to LOCK the table, because we're going to dump out information about
that table (the ACLs on it).  I'm not sure if that's really an issue or
not..  Generally, if you're using pg_dump as a non-superuser, I'd expect
you to be limiting the tables you're dumping to ones you have access to
normally (using -n).

> getTables() take read-lock target tables to make sure they aren't DROPPED
> or altered in schema before we get around to dumping them. Here it having
> below condition to take  a lock:
> 
> if (tblinfo[i].dobj.dump && tblinfo[i].relkind == RELKIND_RELATION)
> 
> which need to replace with:
> 
> if ((tblinfo[i].dobj.dump & DUMP_COMPONENT_DEFINITION) &&
> tblinfo[i].relkind == RELKIND_RELATION)
> 
> PFA patch to fix the issue.

I don't think we want to limit the cases where we take a lock to just
when we're dumping out the table definition..  Consider what happens if
someone drops the table before we get to dumping out the data in the
table, or gathering the ACLs on it (which happens much, much later).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pgindent fixups

[Robert Haas]

On Tue, May 3, 2016 at 9:40 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> 1. Is pgindent supposed to touch DATA() lines?  Because it does.
>
> It always has.
>
>> 2. CustomPathMethods is not in the buildfarm's typedefs.list.  Why not?
>
> Probably because there are no variables, parameters, or fields declared
> with that typedef, so it doesn't get into the debugger symbol table of
> any .o file.  Grep says that the single use of the struct doesn't use
> the typedef; it's
> const struct CustomPathMethods *methods;
> So you'd need to change that, or else tweak some code somewhere to have
> a variable explicitly declared using the typedef.

Ah.  It's a minor issue, so probably not worth worrying about.

>> - In src/backend/executor/execParallel.c, it dodges two cases where
>> pgindent does stupid things with offsetof.
>
> Well, it's also pretty stupid about sizeof, or casts generally, so
> I'm not really convinced you need to get exercised about these two
> places in particular.  But no objection to tweaking them if you
> want to.

OK, I committed this.  Barring objections, I'll go ahead and pgindent
the whole tree tomorrow.  If we're going to revert anything big then
we might want to hold off, but otherwise I think its better to get
this done sooner rather than later.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] pg_dump broken for non-super user

[Stephen Frost]

* Craig Ringer (cr...@2ndquadrant.com) wrote:
> On 3 May 2016 at 19:04, Rushabh Lathia  wrote:
> 
> > With commit a9f0e8e5a2e779a888988cb64479a6723f668c84, now pg_dump, use a
> > bitmap
> > to represent what to include. With this commit if non-super user is unable
> > to perform the dump.
> >
> 
> Hm. I think we need a src/bin/pg_dump/t/ test for the TAP suite for this
> one, because something this fundamental should really have blown the
> buildfarm up when committed.

I'm in the process of building a pretty comprehensive set, as discussed
in another thread.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Need help debugging why autovacuum seems "stuck" -- until I use superuser to vacuum freeze pg_database

[Robert Haas]

On Sun, May 1, 2016 at 10:39 PM, McCoy, Shawn  wrote:
> I have been debugging a problem on a 9.3.10 Postgres database cluster with
> over 1200 databases.  10 workers, increased maintenance_work_mem, auto
> vacuum settings to run more frequently than default.   What I will notice is
> that autovacuum will run for a week or so and traverse databases as
> expected.  I will be able to see that age(datfrozenxid) for all 1200
> databases will stay close to autovacuum_freeze_max_age as desired.
>
> Then, suddenly I will see it get “stuck”.  Autovacuum launcher will not
> launch worker processes even though databases start to age past
> autovacuum_freeze_max_age.  If I create a list of databases and sort by
> age(datfrozenxid), connect to the database with the oldest and execute a
> simple:  "vacuum freeze pg_database;”, autovacuum springs back into action.
>
> It’s never the same database where autovacuum seems to get “stuck”.  I’m
> attempting to gather more debugging information, but, also can’t understand
> why simply doing a “vacuum freeze pg_database” breaks up the jam.
>
> Any thoughts?

So when it's stuck, there are no AV worker processes running at all,
for a sustained period of time?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] max_worker_processes missing some documentation

[Robert Haas]

On Mon, May 2, 2016 at 2:46 PM, Julien Rouhaud
 wrote:
> I noticed that postgresql.conf.sample doesn't say that changing
> max_worker_processes requires a restart.
>
> Patch attached.

Good catch.  Committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] SPI_exec ERROR in pl/r of R 3.2.4 on PostgreSQL on Windows 7

[dandl]

Windows 7 vs 10 makes no odds here. What matters is whether your Postgres or R 
installations are x64 or x86.

 

Any x64 Windows can run x64 or x86 software perfectly happily, but you can’t 
mix a DLL with an EXE of a different flavour. They have to match.

 

Regards

David M Bennett FACS

  _  

Andl - A New Database Language - andl.org

 

 

From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Dave Cramer
Sent: Tuesday, 3 May 2016 11:55 PM
To: Andre Mikulec 
Cc: Joe Conway ; pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] SPI_exec ERROR in pl/r of R 3.2.4 on PostgreSQL on 
Windows 7

 

Does anyone else have a Windows 7 installation we can test this on ?

 

This 
https://github.com/postgres-plr/plr/files/191013/plr-8.3.0.16-pg9.5-win32.zip 
is actually a 64 bit version built on windows 10. I've had one confirmation 
that it works.

 

Dave




Dave Cramer

da...@postgresintl.com  

www.postgresintl.com  

 

On 30 April 2016 at 12:39, Andre Mikulec mailto:andre_miku...@hotmail.com> > wrote:

Joe,

"
Who did the compiling? Did you compile everything yourself, or use
binary installers for some of it? If so, which ones?
"

This is really a continuation of the experience I had with Dave Cramer in here.

Postgresql 9.5 support #1
https://github.com/postgres-plr/plr/issues/1


To try to figure out the problem, ( and perhaps? eliminate Microsoft from the 
problem),
I compiled a PostgreSQL [debug] version myself.

C:\Users\AnonymousUser\Desktop\PostgreSQL.9.5.1\App\PgSQL>chcp 1252 > nul && 
"%PGSQL%\bin\psql.exe"
psql (9.5.1)
Type "help" for help.

postgres=# select version();
  version

 PostgreSQL 9.5.1 on i686-pc-mingw32, compiled by gcc.exe 
(x86_64-posix-seh-rev0, Built by MinGW-W64 project) 5.3.0, 64-bit
(1 row)

I also built a non-debug plr.dll/plr myself too.
I modified ( mostly simplified ) 
https://github.com/jconway/plr/blob/master/Makefile
in the Makefile, I eliminated ( by much trial and error ) the OS non_window 
stuff, the pkg-config stuff, and the  PGXS stuff .

Then I did,
AnonymousUser@ANONYMOUSX /c/Users/AnonymousUser/postgresql-9.4.1/contrib
$ make -C plr clean

AnonymousUser@ANONYMOUSX /c/Users/AnonymousUser/postgresql-9.4.1/contrib
$  make -C plr all

So now I have my own plr.dll.

Then, I followed the instructions ( INSTALL.txt ) found in here.
http://www.joeconway.com/plr/plr-8.3.0.16-pg9.4-win64.zip

However, I used my own plr.dll/plr
Seems, that in the destination, I had to copy plr.dll to plr, but that seems to 
work fine.

Later, after I finish following "create extension plr;" found in 
http://www.joeconway.com/plr/doc/plr-install.html

I do

postgres=# select plr_version();
 plr_version
-
 08.03.00.16
(1 row)

postgres=#   select plr_environ();

 (PGDATA,C:/Users/AnonymousUser/Desktop/PostgreSQL.9.5.1/Data/data)
 (PGDATABASE,postgres)
 
(PGLOCALEDIR,"C:\\Users\\AnonymousUser\\Desktop\\PostgreSQL.9.5.1\\App\\PgSQL\\share\\")
 (PGLOG,"C:\\Users\\AnonymousUser\\Desktop\\PostgreSQL.9.5.1\\Data\\log.txt")
 (PGSQL,"C:\\Users\\AnonymousUser\\Desktop\\PostgreSQL.9.5.1\\App\\PgSQL")
 (PGSYSCONFDIR,C:/Users/AnonymousUser/Desktop/PostgreSQL.9.5.1/App/PgSQL/etc)
 (PGUSER,postgres)

 (R_ARCH,/x64)
 (R_HOME,C:/Users/AnonymousUser/Desktop/R-3.2.4)
 (R_KEEP_PKG_SOURCE,yes)
 (R_LIBS_USER,"C:\\Users\\AnonymousUser\\Documents/R/win-library/3.2")
 (R_USER,"C:\\Users\\AnonymousUser\\Documents")

NOTE: The directory structure is from Postgre 9.4 Portable,  I just use ONLY 
the directory structure.
The one and ONLY file I use is the pgsql.cmd batch startup file ( I did my 
'environment' and 'user friendly modifications.' )

postgres=#

I do this, I get no results, and no error.

postgres=# SELECT * FROM pg_catalog.pg_class WHERE relname = 'plr_modules' AND 
relnamespace = 2200;
 relname | relnamespace | reltype | reloftype | relowner | relam | relfilenode 
| reltablespace | relpages
-+--+-+---+--+---+-+---+--
(0 rows)

But, then this ( R language code ) strangely works.

postgres=# select r_version(); # THIS IS THE 'R LANGUAGE' ( IF THE EXTENSION 
'works' )
r_version
-
 (platform,x86_64-w64-mingw32)
 (arch,x86_64)
 (os,mingw32)
 (system,"x86_64, mingw32")
 (status,"")
 (major,3)
 (minor,2.4)
 (year,2016)
 (month,03)
 (day,10)
 ("svn rev",70301)
 (language,R)
 (version.string,"R version 3.2.4 (2016-03-10)")
 (nickname,"Very Secure Dishes")
(14 rows)

This does not work.
postgres=# select upper(typname) || 'OID' as typename, oid from 
pg_catalog.pg_type where typtype = 'b' order by typname;
ERROR:  could not open file "base/12373/1247": No such file or direc

Re: [HACKERS] pg_dump broken for non-super user

[Craig Ringer]

On 3 May 2016 at 19:04, Rushabh Lathia  wrote:

> With commit a9f0e8e5a2e779a888988cb64479a6723f668c84, now pg_dump, use a
> bitmap
> to represent what to include. With this commit if non-super user is unable
> to perform the dump.
>

Hm. I think we need a src/bin/pg_dump/t/ test for the TAP suite for this
one, because something this fundamental should really have blown the
buildfarm up when committed.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Processes and caches in postgresql

[Craig Ringer]

On 3 May 2016 at 21:37, Merlin Moncure  wrote:


>
> There is library out there, unfortunately GPL licensed, that attempts
> to fully implement posix including fork(): http://midipix.org/.  One
> of these days I'd like to have a go at porting postgres to it.


... and here I thought you'd be keen to instead remove all use of globals
and 'static' locals to allow thread-safe state tracking, remove use of
shmem, introduce threaded backends, replace use of signals, and release
ThreadedPostgres.

Sounds fun, right? :p

More seriously, shouldn't Microsoft's new (or at least re-blessed and
re-released with a new paint job) Linux/POSIX support offer us some options
here? I suspect not - they're probably restricted to ELF binaries that
won't be able to link to native Windows DLLs to get support for things like
SSPI auth, native Windows SSL API use, etc. But it's worth keeping the
possibility in mind.

Frankly, a library that implements fork() might cause exciting explosions
when using native Windows services, too.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] full table delete query

[David G. Johnston]

On Tue, May 3, 2016 at 5:51 AM, hari.prasath 
wrote:

> Hi all,
>   How postgresql handles full table delete in terms of loading the
> full table in these scenarios
>
> consider one big table(tablename: bigtable)
> and the query will be >> delete from bigtable;
>
> 1)which doesn't have any foreign table reference with any other tables
>
> 2)And when this table is referenced by other table
>
>
You should at least consider whether you can use TRUNCATE, especially in #1

An actual delete has to modify every page for the table so it can mark
every row as having been deleted.  I don't think it needs to load TOAST
data but am uncertain.  I reasonably confident all non-TOASTED data will
end up in buffers.

References would depend on CASCADE behavior but in a restrict mode only FK
resolution triggers will be involved.  In most well-design scenarios
indexes are then used instead of the corresponding triggers.  So less data
but still likely every row will be read in.

David J.​

[HACKERS] Re: Logical decoding timeline following fails to handle records split across segments

[Craig Ringer]

On 3 May 2016 at 22:03, Craig Ringer  wrote:


> A corrected and handily much, much simpler patch is attached. The logic
> for finding the last timeline on a segment was massively more complex than
> it needed to be, and that wasn't the only thing.
>

I'm aware that this is late in the piece, btw, and that revert will be
considered. I think that's probably unnecessary - in part because this code
only gets called when doing logical decoding, and only does anything
remotely interesting when replay from a slot hits a timeline boundary. So
it should be assessed mainly based on its risk of breaking what works. That
said, I also think that now that I've fixed the fundamental
misunderstanding embodied by the original patch it should be pretty clear
and reasonable.

The diff looks big, but it just rewrites the new XLogReadDetermineTimeline
function and otherwise just removes the no-longer-needed
 XlogReaderState->timelineHistory member the prior one added. It's best to
just read the new XLogReadDetermineTimeline function, not the diff of that
function, since diff has done confusing things with it.

The changes here are that:

* XLogReadDetermineTimeline(...) now looks up the page being fetched by
ReadPageInternal. It previously used the record start pointer, but that
didn't work if a continuation spilled over to the first page on a new
segment where there's a timeline switch.

* The function first tests cases where it can say "nothing to do, carry on"
and only then handles a timeline switch if one might be required. Simpler,
clearer.

* The logic for determining the last timeline on a segment was way too
complicated in the old patch. Instead, just look up the timeline of the LSN
of the last byte on the segment. No more loops.

* XlogReaderState->timelineHistory is removed; the timeline file is now
read only when needed when making a timeline decision, then discarded. It's
read only very infrequently. Any time we read it we might need to re-read
it if there's a newer one anyway, so it's simpler this way.

* XLogReaderState->currTLIValidUntil is now the tliSwitchPoint for currTLI
and is no longer truncated to the start of the segment boundary like
before. So it's actually useful now. That's a result of fixing the stupid
logic I used in the old version for calculating the last timeline on a
segment and whether there's a timeline change on the segment.

Álvaro is aware of this and I've added it to open items.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)

[Tomas Vondra]

Hi,

On 05/02/2016 09:18 PM, Robert Haas wrote:

On Sat, Apr 30, 2016 at 1:35 PM, Tom Lane  wrote:

Julien Rouhaud  writes:

On 29/04/2016 18:05, Tom Lane wrote:

Julien Rouhaud  writes:

The segfault is caused by quals_match_foreign_key() calling get_leftop()
and get_rightop() on a ScalarArrayOpExpr node.

I'm not sure that assuming this compatibility is the right way to fix
this though.



It certainly isn't.



Agreed. New attached patch handles explicitly each node tag.


No, this is completely nuts.  The problem is that quals_match_foreign_key
is simply assuming that every clause in the list it's given is an OpExpr,
which is quite wrong.  It should just ignore non-OpExpr quals, since it
cannot do anything useful with them anyway.  There's a comment claiming
that non-OpExpr quals were already rejected:

 * Here since 'usefulquals' only contains bitmap indexes for quals
 * of type "var op var" we can safely skip checking this.

but that doesn't appear to have anything to do with current reality.

While this in itself is about a two-line fix, after reviewing
137805f89acb3611 I'm pretty unhappy that it got committed at all.
I think this obvious bug is a good sign that it wasn't ready.
Other unfinished aspects like invention of an undocumented GUC
don't leave a good impression either.

Moreover, it looks to me like this will add quite a lot of overhead,
probably far more than is justified, because clauselist_join_selectivity
is basically O(N^2) in the relation-footprint of the current join --- and
not with a real small multiplier, either, as the functions it calls
contain about four levels of nested loops themselves.  Maybe that's
unmeasurable on trivial test cases but it's going to be disastrous in
large queries, or for relations with large numbers of foreign keys.

I think this should be reverted and pushed out to 9.7 development.
It needs a significant amount of rewriting to fix the performance
issue, and now's not the time to be doing that.


If this gets reverted, then probably
015e88942aa50f0d419ddac00e63bb06d6e62e86 should also be reverted.


Probably. I think the code is fine / correct, but as the FK estimation 
patch was the only user, there's not much point in keeping it if that 
gets reverted.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)

[Tomas Vondra]

Hi,

On 04/30/2016 07:35 PM, Tom Lane wrote:

Julien Rouhaud  writes:

On 29/04/2016 18:05, Tom Lane wrote:

Julien Rouhaud  writes:

The segfault is caused by quals_match_foreign_key() calling get_leftop()
and get_rightop() on a ScalarArrayOpExpr node.

I'm not sure that assuming this compatibility is the right way to fix
this though.



It certainly isn't.



Agreed. New attached patch handles explicitly each node tag.


No, this is completely nuts.  The problem is that quals_match_foreign_key
is simply assuming that every clause in the list it's given is an OpExpr,
which is quite wrong.  It should just ignore non-OpExpr quals, since it
cannot do anything useful with them anyway.  There's a comment claiming
that non-OpExpr quals were already rejected:

 * Here since 'usefulquals' only contains bitmap indexes for quals
 * of type "var op var" we can safely skip checking this.

but that doesn't appear to have anything to do with current reality.


Yes, I agree - there should be a check that the node is OpExpr in 
quals_match_foreign_key. This is clearly a bug :-(




While this in itself is about a two-line fix, after reviewing
137805f89acb3611 I'm pretty unhappy that it got committed at all.
I think this obvious bug is a good sign that it wasn't ready.
Other unfinished aspects like invention of an undocumented GUC
don't leave a good impression either.


The GUC is meant to make testing during development easier. I haven't 
realized it got committed, but I assume Simon plans to remove it before 
the final release. Can't check right now with Simon, though, as he's is 
out of office this week.


In any case, I do agree the GUC should have been documented better, and 
we should probably put it on the TODO so that it gets removed before the 
actual release.




Moreover, it looks to me like this will add quite a lot of overhead,
probably far more than is justified, because
clauselist_join_selectivity is basically O(N^2) in the
relation-footprint of the current join --- and not with a real small
multiplier, either, as the functions it calls contain about four
levels of nested loops themselves. Maybe that's unmeasurable on
trivial test cases but it's going to be disastrous in large queries,
or for relations with large numbers of foreign keys.

>

That depends on a lot of factors, I guess. Attached are two SQL scripts 
that create 5 tables (f1, f2, f3, f4, f5) and then create N foreign keys 
on each of them. The foreign keys reference other tables, which means 
the loops won't terminate early (after finding the first match) when 
joining the first 5 tables, which makes this the worst case. And then we 
join different numbers of tables (2, 3, 4 or 5) and do explain analyze 
to measure planning time.


The first script (fk-test.sql) does joins on 2 columns, fk-test-6.sql 
does joins on 6 columns (so that we also know how the number of columns

affects the planning time).

Sum of planning time for 100 queries (in milliseconds) with 2 columns, 
where "2/on" means "join on 2 tables with enable_fk_estimates=on":


N   2/on2/off   3/on3/off   4/on4/off   5/on5/off
10  6   4   9   8   22  20  77  68
100 15  11  26  19  64  36  177 93
100097  84  217 133 516 233 1246342

and comparison of the timings:

2   3   4   5
10  155%115%114%113%
100 142%138%177%190%
1000116%163%221%364%

And with the 6 columns:

2/on2/off   3/on3/off   4/on4/off   5/on5/off
10  25  7   23  20  96  82  344 297
100 21  14  65  33  233 104 735 328
1000151 99  492 153 1603320 4627593

and comparison:

2   3   4   5
10  371%120%117%116%
100 149%196%224%224%
1000152%322%502%780%

So yes, the overhead is clearly measurable, no doubt about that.



I think this should be reverted and pushed out to 9.7 development.
It needs a significant amount of rewriting to fix the performance
issue, and now's not the time to be doing that.



There are probably a few reasonably simple things we could do - e.g. 
ignore foreign keys with just a single column, as the primary goal of 
the patch is improving estimates with multi-column foreign keys. I 
believe that covers a vast majority of foreign keys in the wild.


If that's deemed insufficient, we'll have to resort to a more complex 
improvement, perhaps something akin to the cache proposed in in the 
unijoin patch. But if that's required, that's 9.7 material.



regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


fk-test-6.sql
Description: application/sql


fk-test.sql
Description: application/sql

-- 
Sent via pgsql-hackers mailing list (pgs