Re: [HACKERS] On Complex Source Code Reading Strategy

[Zeray Kalayu]

On Wed, Aug 2, 2017 at 9:30 PM, Michael Paquier
 wrote:
> On Wed, Aug 2, 2017 at 7:24 AM, Zeray Kalayu  wrote:
>> Lastly, I strongly believe that Code is the ultimate truth and being
>> able to understand complex and high quality code effectively and
>> strategically is of paramount importance.
>
> Documentation to understand how a system works from the user
> prospective, and comments in the code itself are also important
> properties of a code that can be considered as a good base. Postgres
> has both.

Michael,thanks and I am trying to take advantage of them.
Actually, I have learned that being hacker of PG or other data
management platforms  like Greenplum is not a one-stop story or
journey. It is the sum of deep understanding of computing sciences(the
foundations like compatibility theory, computational complexity...),
the engineering aspect of computing and the tech aspect of the same.

Therefore, I feel and think that I am a bit in a hurry to be DB
hacker. But given time, indefatigability and PG community support, I
believe that I will manage to achieve my dream.

Regards,
Zeray


-- 
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] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
 wrote:
> Alvaro Herrera wrote:
>> I think pg_class is a reasonable place to put more generic relkind lists
>> alongside a matching error message for each, rather than specialized
>> "does this relkind have storage" macros.  What about something like a
>> struct list in pg_class.h,
>
> I just noticed that this doesn't help at all with the initial problem
> statement, which is that some of the relkind checks failed to notice
> that partitioned tables needed to be added to the set.  Maybe it still
> helps because you have something to grep for, as Tom proposed elsewhere.

Having something like relkind_i_t_T, though correct, doesn't make the
test readable. That's another improvement because of using such
macros. The macro name tells the purpose of the test, which is what a
reader would be interested in, rather than the actual values used.

>
> However, if there are multiple places that should be kept in sync
> regarding which relkinds to check, then I don't understand Robert's
> objection that only one place requires the check.  Surely we're having
> this discussion precisely because more than one place needs the check,
> and finding those places is not obvious?
>

A new relkind may be required to be added in tests at multiple places.
But each place may have different set of relkinds in that test, which
gets converted into a macro. Given that we have now 9 relkinds, we
could theoretically have 2^9 tests and those many macros. Maintaining
all those would be more cumbersome than grepping RELKIND_ in code.
>From that perspective Robert's concern is valid, but my feeling is
that we are actually using far lesser combinations than that. As I
said, I haven't gone through all the places, so, I don't know the
whole picture yet. But given the number of places where we added
RELKIND_PARTITIONED_TABLE, I guess, there are more than one places
where at least some of those tests are used.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] PostgreSQL not setting OpenSSL session id context?

[Shay Rojansky]

One more note: https://github.com/netty/netty/pull/5321/files is an
equivalent PR setting the session ID context to a constant value in netty
(which is also a server using OpenSSL). This is in line with the
documentation on SSL_CTX_set_session_id_context (
https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_session_id_context(3)
):

> Sessions are generated within a certain context. When exporting/importing
sessions with *i2d_SSL_SESSION*/*d2i_SSL_SESSION* it would be possible, to
re-import a session generated from another context (e.g. another
application), which might lead to malfunctions. Therefore each application
must set its own session id context *sid_ctx* which is used to distinguish
the contexts and is stored in exported sessions. The *sid_ctx* can be any
kind of binary data with a given length, it is therefore possible to use
e.g. the name of the application and/or the hostname and/or service name ...

Re: [HACKERS] PostgreSQL not setting OpenSSL session id context?

[Shay Rojansky]

>
> Shay Rojansky  writes:
> > Once again, I manged to make the error go away simply by setting the
> > session id context, which seems to be a mandatory server-side step for
> > properly support session tickets.
>
> The fact that you made the error go away doesn't make this a good
> solution.  In particular, using a simple constant session ID is completely
> insecure according to the TLS spec.  RFC 5246, F.1.4, doesn't even care
> for the idea of ever writing session IDs to stable storage; although
> Apache seems to be content with a session ID that is unique per-server
> (it looks like they just use a hash of the server's host name).
>

I think there may be a confusion here - I'm not doing anything with session
IDs, merely setting the session ID *context*. This seems to be an
OpenSSL-specific feature (nothing to do with TLS) that simply allows
distinguishing between different "applications" (or contexts) running in
the same process. See
https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_session_id_context(3)
for the docs.

This feature does not involve writing anything (and definitely not session
IDs) to stable storage. The idea is to provide the client with an opaque
"session ticket", which is passed by the client back to the server on
subsequent connections, and which allows the skipping of a roundtrip in the
SSL handshake.


>
> More generally, PG as currently configured can't do anything with a
> session cache since each new backend would start with an empty cache.
>

Again, there's no backend cache - RFC5077 is about having all state at the
client side.


> So the question here is whether it's safe or worthwhile to allow use
> of session tickets.  I agree with Heikki's opinion that it's unlikely
> to provide any meaningful performance gain for database sessions that
> are of reasonable length.  I'm also pretty concerned about the possibility
> for security problems, eg a client being able to force a server into some
> low-security SSL mode.  Both RFC 5077 and the Apache people say that if
> you use session tickets you'd better rotate the keys for them regularly,
> eg in Apache's changelog we find
>
>  Session ticket creation uses a random key created during web
>  server startup and recreated during restarts. No other key
>  recreation mechanism is available currently. Therefore using session
>  tickets without restarting the web server with an appropriate
> frequency
>  (e.g. daily) compromises perfect forward secrecy. [Rainer Jung]
>
> Since we have no mechanism for that, I think that we need to err on
> the side of security.
>

I may definitely be wrong about this, but I'm under the impression that
management of the session ticket (as of the entire resumption mechanism) is
OpenSSL's responsibility and does not require anything from PostgreSQL
itself. However, if you're suspicious of OpenSSL itself that's another
story (and I'd definitely understand).


>
> Accordingly, what I think we should do is something more like the
> attached.  Could you see whether it fixes your problem?
>

I will be able to test this later tonight and confirm. I'm not sure why
the SSL_OP_NO_TICKET is in an #ifdef, I would simply do it in all cases.
I've seen people reporting that this issue is solved via setting
SSL_OP_NO_TICKET (e.g.
https://forums.aws.amazon.com/message.jspa?messageID=505895) so I'm not
sure what SSL_CTX_set_session_cache_mode is supposed to add, but if the
idea is to defensively disable other forms of caching than it makes sense.

Just to be clear, I don't necessarily have a problem with disabling RFC5077
session resumption as the benefits in the PostgreSQL scenario aren't big
(although I don't think a handshake roundtrip is completely negligible
either). I just think it's advisable we understand exactly what it is we're
disabling - there seems to be a confusion between session IDs, session ID
contexts, server/client-side state etc.

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Amit Khandekar]

On 3 August 2017 at 11:00, Amit Langote  wrote:
> Thanks for the review.
>
> On 2017/08/03 13:54, Amit Khandekar wrote:
>> On 2 August 2017 at 11:51, Amit Langote wrote:
>>> On 2017/08/02 1:33, Amit Khandekar wrote:
 Instead of callers of map_partition_varattnos() reporting error, we
 can have map_partition_varattnos() itself report error. Instead of the
 found_whole_row parameter of map_partition_varattnos(), we can have
 error_on_whole_row parameter. So callers who don't expect whole row,
 would pass error_on_whole_row=true to map_partition_varattnos(). This
 will simplify the resultant code a bit.
>>>
>>> So, I think it's only the callers of
>>> map_partition_varattnos() who know whether finding whole-row vars is an
>>> error and *what kind* of error.
>>
>> Ok. Got it. Thanks for the explanation.
>>
>> How about making found_whole_row as an optional parameter ?
>> map_partition_varattnos() would populate it only if its not NULL. This
>> way, callers who don't bother about whether it is found or not don't
>> have to declare a variable and pass its address. In current code,
>> ExecInitModifyTable() is the one who has to declare found_whole_row
>> without needing it.
>
> Sure, done.

Looks good.

> But while implementing that, I avoided changing anything in
> map_variable_attnos(), such as adding a check for not-NULLness of
> found_whole_row.  The only check added is in map_partition_varattnos().

Yes, I agree. That is anyway not relevant to the fix.


-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database 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] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

On Wed, Aug 2, 2017 at 10:28 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I've thought about this kind of thing, too.  But the thing is that
>> most of these macros you're proposing to introduce only get used in
>> one place.
>
> I think the value would be in having a centralized checklist of
> things-to-fix-when-adding-a-new-relkind.

right.

> There's more than one way
> to reach that goal, though.  I wonder whether the task should be defined
> more like "grep for 'RELKIND_' and fix every place you find that".

That way one has to scan all code and change many files. Having them
centrally at one place reduces that pain.

> If there are places to touch that fail to mention that string, fix
> them, using comments if nothing else.

I didn't get this.

> (But see fe797b4a6 and
> followon commits for other solutions.)

That and the follow-on commits replace hard-coded relkind values by
corresponding macro. Though that work it itself is important, I do not
see how that helps to find all the places where the new relkind added
needs to be checked.

>
>> I think this might cause some problems for translators.
>
> Yeah, the error messages that list a bunch of different relkinds in text
> form are going to be a hassle no matter what.  Most of the ways you might
> think of to change that will violate our translatability rules.
>

Ok. I agree with that. May be for now, we shouldn't touch the error
messages at all.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

On Wed, Aug 2, 2017 at 8:47 PM, Robert Haas  wrote:
>  On Mon, Jul 3, 2017 at 3:52 AM, Ashutosh Bapat
>  wrote:
>>> I noticed, that
>>> after we introduced RELKIND_PARTITIONED_TABLE, we required to change a
>>> number of conditions to include this relkind. We missed some places in
>>> initial commits and fixed those later. I am wondering whether we
>>> should creates macros clubbing relevant relkinds together based on the
>>> purpose of the tests e.g. IS_RELKIND_HAS_STORAGE(). When a new relkind
>>> is added, one can examine these macros to check whether the new
>>> relkind fits in the given macro. If all those macros are placed
>>> together, there is a high chance that we will not miss any place in
>>> the initial commit itself.
>
> I've thought about this kind of thing, too.  But the thing is that
> most of these macros you're proposing to introduce only get used in
> one place.
>
> 0001-RELKIND_HAS_VISIBILITY_MAP.patch - one place
> 0002-RELKIND_HAS_STORAGE.patch - one place
> 0003-RELKIND_HAS_XIDS-macro.patch - one place
> 0004-RELKIND_HAS_COMPOSITE_TYPE-macro.patch - one place
> 0005-RELKIND_CAN_HAVE_TOAST_TABLE-macro.patch - one place
> 0006-RELKIND_CAN_HAVE_COLUMN_COMMENT-macro.patch - one place
> 0007-RELKIND_CAN_HAVE_INDEX-macro.patch - two places
> 0008-RELKIND_CAN_HAVE_COLUMN_SECLABEL-macro.patch - one place
> 0009-RELKIND_CAN_HAVE_STATS-macro.patch - two places
>
> I'm totally cool with doing this where we can use the macro in more
> than one place, but otherwise I don't think it helps.

I started grepping RELKIND_MATVIEW and convert corresponding
conditions into macros. I have gone through all the instances yet, so
I am not sure if all the macros are going to be used in only one
place. I will come to know that only once I have gone through all such
instances.

Some of the macros have same relkinds involved e.g.
RELKIND_HAS_VISIBILITY_MAP and RELKIND_HAS_XIDS or
RELKIND_CAN_HAVE_TOAST_TABLE and RELKIND_CAN_HAVE_INDEX. In such cases
it may be better to use a single macro instead of two by using a name
indicating some common property behind those tests. Can we say that
any relation that has visibility map will also have xids? If yes,
what's that common property? Similarly can any relation that can have
toast table also have an index? If yes, what's the common property?  I
didn't get time to investigate it further.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] map_partition_varattnos() and whole-row vars

[Amit Langote]

Thanks for the review.

On 2017/08/03 13:54, Amit Khandekar wrote:
> On 2 August 2017 at 11:51, Amit Langote wrote:
>> On 2017/08/02 1:33, Amit Khandekar wrote:
>>> Instead of callers of map_partition_varattnos() reporting error, we
>>> can have map_partition_varattnos() itself report error. Instead of the
>>> found_whole_row parameter of map_partition_varattnos(), we can have
>>> error_on_whole_row parameter. So callers who don't expect whole row,
>>> would pass error_on_whole_row=true to map_partition_varattnos(). This
>>> will simplify the resultant code a bit.
>>
>> So, I think it's only the callers of
>> map_partition_varattnos() who know whether finding whole-row vars is an
>> error and *what kind* of error.
> 
> Ok. Got it. Thanks for the explanation.
> 
> How about making found_whole_row as an optional parameter ?
> map_partition_varattnos() would populate it only if its not NULL. This
> way, callers who don't bother about whether it is found or not don't
> have to declare a variable and pass its address. In current code,
> ExecInitModifyTable() is the one who has to declare found_whole_row
> without needing it.

Sure, done.  But while implementing that, I avoided changing anything in
map_variable_attnos(), such as adding a check for not-NULLness of
found_whole_row.  The only check added is in map_partition_varattnos().

Attached updated patches.

Thanks,
Amit
From 00a46ef3cfac3b3b197ccf39b8749f06d0d942ad Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 26 Jul 2017 16:45:46 +0900
Subject: [PATCH 1/2] Fix map_partition_varattnos to not error on
 found_whole_row

It was designed assuming that the expressions passed to it can never
contain whole-row vars, but it's wrong since it's called from places
that pass it WCO constraint expressions and RETURNING target list
expressions, which can very well contain whole-row vars.

Move the responsibility of error'ing out to the callers, because they
are in position to know that finding whole-row vars is an error
condition.

Adds test in insert.sql and updatable_views.sql.

Reported by: Rajkumar Raghuwanshi
Report: 
https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com
---
 src/backend/catalog/partition.c   | 17 +++--
 src/backend/commands/tablecmds.c  |  8 +++-
 src/backend/executor/nodeModifyTable.c| 14 ++
 src/include/catalog/partition.h   |  3 ++-
 src/test/regress/expected/insert.out  | 10 ++
 src/test/regress/expected/updatable_views.out | 10 ++
 src/test/regress/sql/insert.sql   |  6 ++
 src/test/regress/sql/updatable_views.sql  |  9 +
 8 files changed, 65 insertions(+), 12 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 9d50efb6a0..5891af9cf2 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -904,10 +904,11 @@ get_qual_from_partbound(Relation rel, Relation parent,
  */
 List *
 map_partition_varattnos(List *expr, int target_varno,
-   Relation partrel, Relation 
parent)
+   Relation partrel, Relation 
parent,
+   bool *found_whole_row)
 {
AttrNumber *part_attnos;
-   boolfound_whole_row;
+   boolmy_found_whole_row;
 
if (expr == NIL)
return NIL;
@@ -919,10 +920,9 @@ map_partition_varattnos(List *expr, int target_varno,

target_varno, 0,

part_attnos,

RelationGetDescr(parent)->natts,
-   
_whole_row);
-   /* There can never be a whole-row reference here */
+   
_found_whole_row);
if (found_whole_row)
-   elog(ERROR, "unexpected whole-row reference found in partition 
key");
+   *found_whole_row = my_found_whole_row;
 
return expr;
 }
@@ -1783,6 +1783,7 @@ generate_partition_qual(Relation rel)
List   *my_qual = NIL,
   *result = NIL;
Relationparent;
+   boolfound_whole_row;
 
/* Guard against stack overflow due to overly deep partition tree */
check_stack_depth();
@@ -1825,7 +1826,11 @@ generate_partition_qual(Relation rel)
 * in it to bear this relation's attnos. It's safe to assume varno = 1
 * here.
 */
-   result = map_partition_varattnos(result, 1, rel, parent);
+   result = map_partition_varattnos(result, 1, rel, parent,
+ 

Re: [HACKERS] Unused variable scanned_tuples in LVRelStats

[Masahiko Sawada]

On Wed, Aug 2, 2017 at 11:40 PM, Robert Haas  wrote:
> On Tue, Jul 4, 2017 at 10:13 PM, Masahiko Sawada  
> wrote:
>> scanned_tuples variable in LVRelStats is introduced by commit b4b6923e
>> but it seems to me that it's actually not used. We store num_tuples
>> into vacrelstats->scanned_tuples after scanned all blocks, and the
>> comment mentioned that saving it in order to use later but we actually
>> use num_tuples instead of vacrelstats->scanned_tuples from there. I
>> think the since the name of scanned_tuples implies more clearer
>> purpose than num_tuples it's better to use it instead of num_tuples,
>> or we can remove scanned_tuples from LVRelStats.

Thank you for the comment!

>
> I think we should only store stuff in LVRelStats if it needs to be
> passed to some other function.

Agreed. From this point of view, num_tuples is only one variable of
LVRelStats that is not passed to other functions.

> Data that's only used in
> lazy_scan_heap() can just be kept in local variables.  We could rename
> the local variable, though, since I agree with you that scanned_tuples
> is clearer.
>

So we can remove scanned_tuples from LVRelStats struct and change the
variable name num_tuples to scanned_tuples. Attached updated patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


fix_vacuumlazy_v2.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] A bug in mapping attributes in ATExecAttachPartition()

[Amit Langote]

On 2017/08/02 20:35, Robert Haas wrote:
> On Tue, Aug 1, 2017 at 9:44 PM, Amit Langote
>  wrote:
>> I too dislike the shape of attachRel.  How about we rename attachRel to
>> attachrel?  So, attachrel_children, attachrel_constr, etc.  It's still
>> long though... :)
> 
> OK, I can live with that, I guess.

Alright, attached updated 0001 does that.

About the other hunk, it seems we will have to go with the following
structure after all;

if (a)
  if (b)
 c();
  d();

Note that we were missing that there is a d(), which executes if a is
true.  c() executes *only* if b is true.  So I left that hunk unchanged,
viz. the following:

 /*
- * Skip if it's a partitioned table.  Only RELKIND_RELATION
- * relations (ie, leaf partitions) need to be scanned.
+ * Skip if the partition is itself a partitioned table.  We can
+ * only ever scan RELKIND_RELATION relations.
  */
-if (part_rel != attachRel &&
-part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 {
-heap_close(part_rel, NoLock);
+if (part_rel != attachrel)
+heap_close(part_rel, NoLock);
 continue;
 }


You might ask why the earlier code worked if there was this kind of
logical bug - accident; even if we failed skipping attachRel, the AT
rewrite phase which is in charge of actually scanning the table knows to
skip the partitioned tables, so no harm would be done.

Thanks,
Amit
From 4558c1b31f10e5446a22850a7f8b3d80d082330d Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 1 Aug 2017 10:12:39 +0900
Subject: [PATCH 1/4] Cosmetic fixes for code in ATExecAttachPartition

---
 src/backend/commands/tablecmds.c | 125 +++
 1 file changed, 61 insertions(+), 64 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index bb00858ad1..ecfc7e48c7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13419,10 +13419,10 @@ ComputePartitionAttrs(Relation rel, List *partParams, 
AttrNumber *partattrs,
 static ObjectAddress
 ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 {
-   RelationattachRel,
+   Relationattachrel,
catalog;
-   List   *childrels;
-   TupleConstr *attachRel_constr;
+   List   *attachrel_children;
+   TupleConstr *attachrel_constr;
List   *partConstraint,
   *existConstraint;
SysScanDesc scan;
@@ -13434,22 +13434,22 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
ObjectAddress address;
const char *trigger_name;
 
-   attachRel = heap_openrv(cmd->name, AccessExclusiveLock);
+   attachrel = heap_openrv(cmd->name, AccessExclusiveLock);
 
/*
 * Must be owner of both parent and source table -- parent was checked 
by
 * ATSimplePermissions call in ATPrepCmd
 */
-   ATSimplePermissions(attachRel, ATT_TABLE | ATT_FOREIGN_TABLE);
+   ATSimplePermissions(attachrel, ATT_TABLE | ATT_FOREIGN_TABLE);
 
/* A partition can only have one parent */
-   if (attachRel->rd_rel->relispartition)
+   if (attachrel->rd_rel->relispartition)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 errmsg("\"%s\" is already a partition",
-   
RelationGetRelationName(attachRel;
+   
RelationGetRelationName(attachrel;
 
-   if (OidIsValid(attachRel->rd_rel->reloftype))
+   if (OidIsValid(attachrel->rd_rel->reloftype))
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 errmsg("cannot attach a typed table as 
partition")));
@@ -13462,7 +13462,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
ScanKeyInit(,
Anum_pg_inherits_inhrelid,
BTEqualStrategyNumber, F_OIDEQ,
-   ObjectIdGetDatum(RelationGetRelid(attachRel)));
+   ObjectIdGetDatum(RelationGetRelid(attachrel)));
scan = systable_beginscan(catalog, InheritsRelidSeqnoIndexId, true,
  NULL, 1, );
if (HeapTupleIsValid(systable_getnext(scan)))
@@ -13475,11 +13475,11 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
ScanKeyInit(,
Anum_pg_inherits_inhparent,
BTEqualStrategyNumber, F_OIDEQ,
- 

Re: [HACKERS] map_partition_varattnos() and whole-row vars

[Amit Khandekar]

On 2 August 2017 at 11:51, Amit Langote  wrote:
> Thanks Fuita-san and Amit for reviewing.
>
> On 2017/08/02 1:33, Amit Khandekar wrote:
>> On 1 August 2017 at 15:11, Etsuro Fujita  wrote:
>>> On 2017/07/31 18:56, Amit Langote wrote:
 Yes, that's what's needed here.  So we need to teach
 map_variable_attnos_mutator() to convert whole-row vars just like it's
 done in adjust_appendrel_attrs_mutator().
>>>
>>>
>>> Seems reasonable.  (Though I think it might be better to do this kind of
>>> conversion in the planner, not the executor, because that would increase the
>>> efficiency of cached plans.)
>
> That's a good point, although it sounds like a bigger project that, IMHO,
> should be undertaken separately, because that would involve designing for
> considerations of expanding inheritance even in the INSERT case.
>
>> I think the work of shifting to planner should be taken as a different
>> task when we shift the preparation of DispatchInfo to the planner.
>
> Yeah, I think it'd be a good idea to do those projects together.  That is,
> doing what Fujita-san suggested and expanding partitioned tables in
> partition bound order in the planner.
>
 Attached 2 patches:

 0001: Addresses the bug that Rajkumar reported (handling whole-row vars in
WITH CHECK and RETURNING expressions at all)

 0002: Addressed the bug that Amit reported (converting whole-row vars
that could occur in WITH CHECK and RETURNING expressions)
>>>
>>>
>>> I took a quick look at the patches.  One thing I noticed is this:
>>>
>>>  map_variable_attnos(Node *node,
>>> int target_varno, int sublevels_up,
>>> const AttrNumber *attno_map, int
>>> map_length,
>>> +   Oid from_rowtype, Oid to_rowtype,
>>> bool *found_whole_row)
>>>  {
>>> map_variable_attnos_context context;
>>> @@ -1291,6 +1318,8 @@ map_variable_attnos(Node *node,
>>> context.sublevels_up = sublevels_up;
>>> context.attno_map = attno_map;
>>> context.map_length = map_length;
>>> +   context.from_rowtype = from_rowtype;
>>> +   context.to_rowtype = to_rowtype;
>>> context.found_whole_row = found_whole_row;
>>>
>>> You added two arguments to pass to map_variable_attnos(): from_rowtype and
>>> to_rowtype.  But I'm not sure that we really need the from_rowtype argument
>>> because it's possible to get the Oid from the vartype of target whole-row
>>> Vars.  And in this:
>>>
>>> +   /*
>>> +* If the callers expects us to convert the
>>> same, do so if
>>> +* necessary.
>>> +*/
>>> +   if (OidIsValid(context->to_rowtype) &&
>>> +   OidIsValid(context->from_rowtype) &&
>>> +   context->to_rowtype !=
>>> context->from_rowtype)
>>> +   {
>>> +   ConvertRowtypeExpr *r =
>>> makeNode(ConvertRowtypeExpr);
>>> +
>>> +   r->arg = (Expr *) newvar;
>>> +   r->resulttype =
>>> context->from_rowtype;
>>> +   r->convertformat =
>>> COERCE_IMPLICIT_CAST;
>>> +   r->location = -1;
>>> +   /* Make sure the Var node has the
>>> right type ID, too */
>>> +   newvar->vartype =
>>> context->to_rowtype;
>>> +   return (Node *) r;
>>> +   }
>>>
>>> I think we could set r->resulttype to the vartype (ie, "r->resulttype =
>>> newvar->vartype" instead of "r->resulttype = context->from_rowtype").
>>
>> I agree.
>
> You're right, from_rowtype is unnecessary.
>
>> ---
>>
>> Few more comments :
>>
>> @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,
>>  var->varlevelsup == context->sublevels_up)
>>  {
>>  /* Found a matching variable, make the substitution */
>>
>> - Var*newvar = (Var *) palloc(sizeof(Var));
>> + Var*newvar = copyObject(var);
>>   int attno = var->varattno;
>>
>>  *newvar = *var;
>>
>> Here, "*newvar = *var" should be removed.
>
> Done.
>
>> ---
>>
>> -   result = map_partition_varattnos(result, 1, rel, parent);
>> +   result = map_partition_varattnos(result, 1, rel, parent,
>> +
>>   _whole_row);
>> +   /* There can never be a whole-row reference here */
>> +   if (found_whole_row)
>> +   elog(ERROR, "unexpected whole-row reference found in
>> partition key");
>>
>> Instead of callers of map_partition_varattnos() reporting error, 

Re: [HACKERS] foreign table creation and NOT VALID check constraints

[Tom Lane]

Robert Haas  writes:
> On Wed, Aug 2, 2017 at 9:41 PM, Amit Langote
>  wrote:
>> Attached is a patch.  I think this could be considered a bug-fix,
>> backpatchable to 9.6 which introduced this behavior change [1].

> I could go either way on that.  It's not inconceivable somebody could
> be unhappy about seeing this behavior change in a minor release.

FWIW, I vote with the camp that this is a clear bug and needs to be
fixed.  9.6 broke a behavior that could be relied on before that.
We do not normally hesitate to fix regressions in minor releases.

(That's not a vote for the patch as submitted; I haven't reviewed it.
But we need to fix this.)

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] INSERT ON CONFLICT and partitioned tables

[Amit Langote]

Thanks Jeevan for looking at this.  See comments below.

On 2017/08/02 19:04, Jeevan Ladhe wrote:
> On Wed, Aug 2, 2017 at 10:26 AM, Amit Langote wrote:
>> The patch's job is simple:
>>
>> - Remove the check in the parser that causes an error the moment the
>>   ON CONFLICT clause is found.
>>
>> - Fix leaf partition ResultRelInfo initialization code so that the call
>>   ExecOpenIndices() specifies 'true' for speculative, so that the
>>   information necessary for conflict checking will be initialized in the
>>   leaf partition's ResultRelInfo
>
> I applied the patch on latest master sources and the patch applies cleanly.
> The documentation is built without errors.
>
> We do not support following syntax for 'do nothing':
>
> postgres=# insert into parted_conflict_test values (1, 'a') on conflict (b)
> do nothing;
> ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT
> specification

To nitpick, the above is not a syntax error; we *do* support the syntax to
specify conflict target even when the conflict action is DO NOTHING.  The
error is emitted by the planner when it fails to find the index to cover
column 'b' that's specified as the conflict target.

> This limitation is because we do not support unique index on partitioned
> table.

Yes.

> But, in that sense the following snippet of the documentation seems
> misleading:
>
> +   will cause an error if the conflict target is specified (see
> +for more details).  That means it's not
> +   possible to specify DO UPDATE as the alternative
> +   action, because it requires the conflict target to be specified.
> +   On the other hand, specifying DO NOTHING as the
> +   alternative action works fine.
> May be the last sentence can be rephrased as below:
>
> "On the other hand, specifying DO NOTHING without target
> as
> an alternative action works fine."

I have updated the text.

> Other than this patch looks good to me.

Updated patch attached.

Thanks,
Amit

[1]
https://www.postgresql.org/docs/devel/static/sql-insert.html#sql-on-conflict
From ec32e99310c034b26db1c5478ed38641150a7aec Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 3 Apr 2017 19:13:38 +0900
Subject: [PATCH] Allow ON CONFLICT DO NOTHING on partitioned tables

ON CONFLICT .. DO UPDATE still doesn't work, because it requires
specifying the conflict target.  DO NOTHING doesn't require it,
but the executor will check for conflicts within only a given
leaf partitions, if relevant constraints exist.

Specifying the conflict target makes the planner look for the
required indexes on the parent table, which are not allowed, so an
error will always be reported in that case.
---
 doc/src/sgml/ddl.sgml | 12 +---
 src/backend/executor/execMain.c   | 10 ++
 src/backend/parser/analyze.c  |  8 
 src/test/regress/expected/insert_conflict.out | 10 ++
 src/test/regress/sql/insert_conflict.sql  | 10 ++
 5 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index b05a9c2150..23b0c42dba 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3276,9 +3276,15 @@ ALTER TABLE measurement ATTACH PARTITION 
measurement_y2008m02
  
   
Using the ON CONFLICT clause with partitioned tables
-   will cause an error, because unique or exclusion constraints can only be
-   created on individual partitions.  There is no support for enforcing
-   uniqueness (or an exclusion constraint) across an entire partitioning
+   will cause an error if the conflict target is specified (see
+for more details on how the clause
+   works).  That means it's not possible to specify
+   DO UPDATE as the alternative action, because
+   specifying the conflict target is mandatory in that case.  On the other
+   hand, specifying DO NOTHING as the alternative action
+   works fine provided the conflict target is not specified.  In that case,
+   unique constraints (or exclusion constraints) of the individual leaf
+   partitions are considered, not those across the whole partitioning
hierarchy.
   
  
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c11aa4fe21..b313ad1efa 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -3300,13 +3300,15 @@ ExecSetupPartitionTupleRouting(Relation rel,
  0);
 
/*
-* Open partition indices (remember we do not support ON 
CONFLICT in
-* case of partitioned tables, so we do not need support 
information
-* for speculative insertion)
+* Open partition indices.  The user may have asked to check for
+* conflicts within this leaf partition and do "nothing" 
instead of
+* 

Re: [HACKERS] fixing pg_upgrade strings (was Re: pgsql: Add new files to nls.mk and add translation)

[Tom Lane]

Alvaro Herrera  writes:
> It does this:

> pg_fatal("You must identify the directory where the %s.\n"
>  "Please use the %s command-line option or the %s 
> environment variable.\n",
>  description, cmdLineOption, envVarName);

> and the callsites are like this:

> check_required_directory(_cluster.bindir, NULL, "PGBINNEW", "-B",
>  _("new cluster binaries reside"));
> check_required_directory(_cluster.pgdata, _cluster.pgconfig,
>  "PGDATAOLD", "-d", _("old cluster data 
> resides"));

> note the declensions don't match even in the English original.

FWIW, I think the English declensions are fine as long as you consider
"data" to be a group noun --- "data reside" would read a bit funny IMO.
But certainly this is an utter translatability fail.  Maybe pass a boolean
to indicate one of two first sentences to use?  Or actually, might as well
consider the entire message string to be one of two translatable options.
It's not like we have a need to indicate more than one option or envar
name for each case.

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] [COMMITTERS] pgsql: Fix pg_dump's errno checking for zlib I/O

[Tom Lane]

Alvaro Herrera  writes:
> Alvaro Herrera wrote:
>> Fix pg_dump's errno checking for zlib I/O

> So this broke a few buildfarm members.  I'll into it tomorrow.

I think you forgot to consider the !HAVE_LIBZ case.

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] intermittent failures in Cygwin from select_parallel tests

[Noah Misch]

On Wed, Jun 21, 2017 at 06:44:09PM -0400, Tom Lane wrote:
> Today, lorikeet failed with a new variant on the bgworker start crash:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet=2017-06-21%2020%3A29%3A10
> 
> This one is even more exciting than the last one, because it sure looks
> like the crashing bgworker took the postmaster down with it.  That is
> Not Supposed To Happen.
> 
> Wondering if we broke something here recently, I tried to reproduce it
> on a Linux machine by adding a randomized Assert failure in
> shm_mq_set_sender.  I don't see any such problem, even with EXEC_BACKEND;
> we recover from the crash as-expected.
> 
> So I'm starting to get a distinct feeling that there's something wrong
> with the cygwin port.  But I dunno what.

I think signal blocking broke on Cygwin.

On a system (gcc 5.4.0, CYGWIN_NT-10.0 2.7.0(0.306/5/3) 2017-02-12 13:18
x86_64) that reproduces lorikeet's symptoms, I instrumented the postmaster as
attached.  The patch's small_parallel.sql is a subset of select_parallel.sql
sufficient to reproduce the mq_sender Assert failure and the postmaster silent
exit.  (It occasionally needed hundreds of iterations to do so.)  The parallel
query normally starts four bgworkers; when the mq_sender Assert fired, the
test had started five workers in response to four registrations.

The postmaster.c instrumentation regularly detects sigusr1_handler() calls
while another sigusr1_handler() is already on the stack:

6328 2017-08-02 07:25:42.788 GMT LOG:  forbid signals @ sigusr1_handler
6328 2017-08-02 07:25:42.788 GMT DEBUG:  saw slot-0 registration, want 0
6328 2017-08-02 07:25:42.788 GMT DEBUG:  saw slot-0 registration, want 1
6328 2017-08-02 07:25:42.788 GMT DEBUG:  slot 1 not yet registered
6328 2017-08-02 07:25:42.789 GMT DEBUG:  registering background worker 
"parallel worker for PID 4776" (slot 1)
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-1 registration, want 2
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-0 registration, want 2
6328 2017-08-02 07:25:42.789 GMT DEBUG:  slot 2 not yet registered
6328 2017-08-02 07:25:42.789 GMT DEBUG:  registering background worker 
"parallel worker for PID 4776" (slot 2)
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-2 registration, want 3
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-1 registration, want 3
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-0 registration, want 3
6328 2017-08-02 07:25:42.789 GMT DEBUG:  slot 3 not yet registered
6328 2017-08-02 07:25:42.789 GMT DEBUG:  registering background worker 
"parallel worker for PID 4776" (slot 3)
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-3 registration, want 4
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-2 registration, want 4
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-1 registration, want 4
6328 2017-08-02 07:25:42.789 GMT DEBUG:  saw slot-0 registration, want 4
6328 2017-08-02 07:25:42.789 GMT DEBUG:  slot 4 not yet registered
6328 2017-08-02 07:25:42.789 GMT DEBUG:  registering background worker 
"parallel worker for PID 4776" (slot 4)
6328 2017-08-02 07:25:42.789 GMT DEBUG:  starting background worker process 
"parallel worker for PID 4776"
6328 2017-08-02 07:25:42.790 GMT LOG:  forbid signals @ sigusr1_handler
6328 2017-08-02 07:25:42.790 GMT WARNING:  signals already forbidden @ 
sigusr1_handler
6328 2017-08-02 07:25:42.790 GMT LOG:  permit signals @ sigusr1_handler

postmaster algorithms rely on the PG_SETMASK() calls preventing that.  Without
such protection, duplicate bgworkers are an understandable result.  I caught
several other assertions; the PMChildFlags failure is another case of
duplicate postmaster children:

  6 TRAP: FailedAssertion("!(entry->trans == ((void *)0))", File: 
"pgstat.c", Line: 871)
  3 TRAP: FailedAssertion("!(PMSignalState->PMChildFlags[slot] == 1)", 
File: "pmsignal.c", Line: 229)
 20 TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 
2523)
 21 TRAP: FailedAssertion("!(vmq->mq_sender == ((void *)0))", File: 
"shm_mq.c", Line: 221)
 Also, got a few "select() failed in postmaster: Bad address"

I suspect a Cygwin signals bug.  I'll try to distill a self-contained test
case for the Cygwin hackers.  The lack of failures on buildfarm member brolga
argues that older Cygwin is not affected.
diff --git a/src/backend/access/transam/parallel.c 
b/src/backend/access/transam/parallel.c
index 17b1038..f42034d 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -985,6 +985,8 @@ ParallelWorkerMain(Datum main_arg)
error_queue_space = shm_toc_lookup(toc, PARALLEL_KEY_ERROR_QUEUE, 
false);
mq = (shm_mq *) (error_queue_space +
 ParallelWorkerNumber * 
PARALLEL_ERROR_QUEUE_SIZE);
+   write_stderr("PID %d claiming queue %d (%p)\n",
+MyProc->pid, ParallelWorkerNumber, mq);
shm_mq_set_sender(mq, MyProc);
mqh = 

[HACKERS] fixing pg_upgrade strings (was Re: pgsql: Add new files to nls.mk and add translation)

[Alvaro Herrera]

Peter Eisentraut wrote:
> Add new files to nls.mk and add translation markers

This reminds me that I noticed a few days ago another really serious
broken piece in pg_upgrade where check_required_directory() is incurring
in the ugliest case of string building I've ever seen.  I didn't have
the courage to try to fix it back then, but I think we should dedicate
it some time.  It does this:

pg_fatal("You must identify the directory where the %s.\n"
 "Please use the %s command-line option or the %s 
environment variable.\n",
 description, cmdLineOption, envVarName);

and the callsites are like this:

check_required_directory(_cluster.bindir, NULL, "PGBINNEW", "-B",
 _("new cluster binaries reside"));
check_required_directory(_cluster.pgdata, _cluster.pgconfig,
 "PGDATAOLD", "-d", _("old cluster data resides"));

note the declensions don't match even in the English original.

-- 
Álvaro Herrerahttps://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] map_partition_varattnos() and whole-row vars

[Etsuro Fujita]

On 2017/08/03 12:06, Robert Haas wrote:

On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote
 wrote:

On 2017/08/02 15:21, Amit Langote wrote:

Thanks Fuita-san and Amit for reviewing.


Sorry, I meant Fujita-san.


Time is growing short here.  Is there more review or coding that needs
to be done here?


I'll take another look at the patch from now.

Best regards,
Etsuro Fujita



--
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] foreign table creation and NOT VALID check constraints

[Robert Haas]

On Wed, Aug 2, 2017 at 9:41 PM, Amit Langote
 wrote:
> On 2017/08/02 20:40, Robert Haas wrote:
>> On Wed, Aug 2, 2017 at 3:46 AM, Ashutosh Bapat
>>  wrote:
>>> If the user has specified "not valid" for a constraint on the foreign
>>> table, there is high chance that s/he is aware of the fact that the
>>> remote table that the foreign table points to has some rows which will
>>> violet the constraint. So, +1.
>>
>> +1 from me, too.
>
> Alright, thanks.
>
> Attached is a patch.  I think this could be considered a bug-fix,
> backpatchable to 9.6 which introduced this behavior change [1].

I could go either way on that.  It's not inconceivable somebody could
be unhappy about seeing this behavior change in a minor release.

-- 
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] map_partition_varattnos() and whole-row vars

[Robert Haas]

On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote
 wrote:
> On 2017/08/02 15:21, Amit Langote wrote:
>> Thanks Fuita-san and Amit for reviewing.
>
> Sorry, I meant Fujita-san.

Time is growing short here.  Is there more review or coding that needs
to be done here?

-- 
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] Another comment typo in execMain.c

[Etsuro Fujita]

On 2017/08/01 6:09, Peter Eisentraut wrote:

On 7/6/17 03:23, Etsuro Fujita wrote:

Here is a comment in ExecFindPartition() in execMain.c:

  /*
   * First check the root table's partition constraint, if any.  No
point in
   * routing the tuple it if it doesn't belong in the root table itself.
   */

I think that in the second sentence "it" just before "if" is a typo.
Attached is a small patch for fixing that.


fixed


Thanks!

Best regards,
Etsuro Fujita



--
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(wait_for_archive := true) on standby server

[Stephen Frost]

Noah,

On Tue, Aug 1, 2017 at 20:52 Noah Misch  wrote:

> On Thu, Jul 27, 2017 at 10:27:36AM -0400, Stephen Frost wrote:
> > Noah, all,
> >
> > * Noah Misch (n...@leadboat.com) wrote:
> > > This PostgreSQL 10 open item is past due for your status update.
> Kindly send
> > > a status update within 24 hours, and include a date for your
> subsequent status
> > > update.  Refer to the policy on open item ownership:
> >
> > Based on the ongoing discussion, this is really looking like it's
> > actually a fix that needs to be back-patched to 9.6 rather than a PG10
> > open item.  I don't have any issue with keeping it as an open item
> > though, just mentioning it.  I'll provide another status update on or
> > before Monday, July 31st.
>
> This PostgreSQL 10 open item is past due for your status update.  Kindly
> send
> a status update within 24 hours, and include a date for your subsequent
> status
> update.


I'll provide another update tomorrow.  Hopefully Michael is able to produce
a 9.6 patch, otherwise I'll do it.

Thanks!

Stephen

>

Re: [HACKERS] [COMMITTERS] pgsql: Fix pg_dump's errno checking for zlib I/O

[Alvaro Herrera]

Alvaro Herrera wrote:
> Fix pg_dump's errno checking for zlib I/O

So this broke a few buildfarm members.  I'll into it tomorrow.

-- 
Álvaro Herrerahttps://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] foreign table creation and NOT VALID check constraints

[Amit Langote]

On 2017/08/02 20:40, Robert Haas wrote:
> On Wed, Aug 2, 2017 at 3:46 AM, Ashutosh Bapat
>  wrote:
>> If the user has specified "not valid" for a constraint on the foreign
>> table, there is high chance that s/he is aware of the fact that the
>> remote table that the foreign table points to has some rows which will
>> violet the constraint. So, +1.
> 
> +1 from me, too.

Alright, thanks.

Attached is a patch.  I think this could be considered a bug-fix,
backpatchable to 9.6 which introduced this behavior change [1].

Thoughts?

Thanks,
Amit

[1]
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=f27a6b15e656
From a0967f1a71a65e7802f504002a6dc3dfd1f4a6bb Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 3 Aug 2017 10:31:21 +0900
Subject: [PATCH] Honor NOT VALID specification in CREATE FOREIGN TABLE

It should be possible for a user to mark the constraints on foreign
tables as NOT VALID even when creating the table, because the remote
data they point to may contain constraint-violating rows, which the
database has no way to detect and show an error message for.
---
 doc/src/sgml/ref/create_foreign_table.sgml | 7 +++
 doc/src/sgml/ref/create_table.sgml | 7 +++
 src/backend/parser/parse_utilcmd.c | 3 ++-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/create_foreign_table.sgml 
b/doc/src/sgml/ref/create_foreign_table.sgml
index 065c982082..72bf37b8b9 100644
--- a/doc/src/sgml/ref/create_foreign_table.sgml
+++ b/doc/src/sgml/ref/create_foreign_table.sgml
@@ -222,6 +222,13 @@ CHECK ( expression ) [ NO INHERIT ]
   A constraint marked with NO INHERIT will not propagate to
   child tables.
  
+
+ 
+  It is possible to mark the constraint as NOT VALID if it is
+  specified as a table constraint.  If marked as such, the database will
+  not assume that the constraint holds for all the rows in the table until
+  it is validated by using the VALIDATE CONSTRAINT command.
+  See .
 

 
diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index e9c2c49533..72de64a03e 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -652,6 +652,13 @@ FROM ( { numeric_literal | PostgreSQL versions before 9.5 did not honor any
   particular firing order for CHECK constraints.)
  
+
+ 
+  Note that even if the constraint is marked as NOT VALID,
+  it is considered validated as the table that is just created cannot
+  contain any data.  In other words, specifying NOT VALID in
+  this case has no effect.
+ 
 

 
diff --git a/src/backend/parser/parse_utilcmd.c 
b/src/backend/parser/parse_utilcmd.c
index 9f37f1b920..e54322b460 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -165,6 +165,7 @@ transformCreateStmt(CreateStmt *stmt, const char 
*queryString)
Oid existing_relid;
ParseCallbackState pcbstate;
boollike_found = false;
+   boolis_foreign_table = IsA(stmt, CreateForeignTableStmt);
 
/*
 * We must not scribble on the passed-in CreateStmt, so copy it.  (This 
is
@@ -330,7 +331,7 @@ transformCreateStmt(CreateStmt *stmt, const char 
*queryString)
/*
 * Postprocess check constraints.
 */
-   transformCheckConstraints(, true);
+   transformCheckConstraints(, !is_foreign_table ? true : false);
 
/*
 * Output results.
-- 
2.11.0


-- 
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] Why does logical replication launcher exit with exit code 1?

[Robert Haas]

On Wed, Aug 2, 2017 at 9:33 PM, Peter Eisentraut
 wrote:
> On 8/2/17 16:52, Robert Haas wrote:
>> I actually don't think it's that unreasonable to get notified when
>> system-wide processes like the autovacuum launcher or the logical
>> replication launcher start or stop.
>
> But we got rid of those start messages recently due to complaints.

Yeah, true.  I'm just talking about what *I* 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] Why does logical replication launcher exit with exit code 1?

[Peter Eisentraut]

On 8/2/17 16:52, Robert Haas wrote:
> I actually don't think it's that unreasonable to get notified when
> system-wide processes like the autovacuum launcher or the logical
> replication launcher start or stop.

But we got rid of those start messages recently due to complaints.

-- 
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] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Mon, Jul 31, 2017 at 9:07 AM, Ashutosh Bapat
 wrote:
> Forgot the patch set. Here it is.

The commit message for 0005 isn't really accurate given that it
follows 0004.  I think you could just flatten 0005 and 0006 into one
patch.

Reviewing those together:

- Existing code does partdesc = RelationGetPartitionDesc(relation) but
this has got it as part_desc.  Seems better to be consistent.
Likewise existing variables for PartitionKey are key or partkey, not
part_key.

- get_relation_partition_info has a useless trailing return.

- Instead of adding nparts, boundinfo, and part_oids to RelOptInfo,
how about just adding partdesc?  Seems cleaner.

- pkexprs seems like a potentially confusing name, since PK is widely
used to mean "primary key" but here you mean "partition key".  Maybe
partkeyexprs.

- build_simple_rel's matching algorithm is O(n^2).  We may have talked
about this problem before...

- This patch introduces some bits that are not yet used, like
nullable_pkexprs, or even the code to set the partition scheme for
joinrels.  I think perhaps some of that logic should be moved from
0008 to here - e.g. the initial portion of
build_joinrel_partition_info.

There may be more, but I've run out of energy for tonight.

-- 
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] Confusing error message in pgbench

[Tatsuo Ishii]

>> Not really objecting, but an even better fix might be to remove the
>> restriction on the order in which the options can be specified.
> 
> Indeed.  It doesn't look that hard: AFAICS the problem is just that
> process_sql_command() is making premature decisions about whether to
> extract parameters from the SQL commands.  Proposed patch attached.

Great. Patch looks good to me.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] asynchronous execution

[Kyotaro HORIGUCHI]

Thank you for the comment.

At Tue, 1 Aug 2017 16:27:41 -0400, Robert Haas  wrote in 

> On Mon, Jul 31, 2017 at 5:42 AM, Kyotaro HORIGUCHI
>  wrote:
> > Another is getting rid of recursive call to run an execution
> > tree.
> 
> That happens to be exactly what Andres did for expression evaluation
> in commit b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755, and I think
> generalizing that to include the plan tree as well as expression trees
> is likely to be the long-term way forward here.

I read it in the source tree. The patch implements converting
expression tree to an intermediate expression then run it on a
custom-made interpreter. Guessing from the word "upside down"
from Andres, the whole thing will become source-driven.

> Unfortunately, that's probably another gigantic patch (that
> should probably be written by Andres).

Yeah, but async executor on the current style of executor seems
furtile work, or sitting until the patch comes is also waste of
time. So I'm planning to include the following sutff in the next
PoC patch. Even I'm not sure it can land on the coming
Andres'patch.

- Tuple passing outside call-stack. (I remember it was in the
  past of the thread around but not found)

  This should be included in the Andres' patch.

- Give executor an ability to run from data-source (or driver)
  nodes to the root.

  I'm not sure this is included, but I suppose he is aiming this
  kind of thing.

- Rebuid asynchronous execution on the upside-down executor.


regrds,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
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] Confusing error message in pgbench

[Tatsuo Ishii]

> Not really objecting, but an even better fix might be to remove the
> restriction on the order in which the options can be specified.

+100 :-)

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] Parallel tuplesort (for parallel B-Tree index creation)

[Andres Freund]

On 2017-02-10 07:52:57 -0500, Robert Haas wrote:
> On Thu, Feb 9, 2017 at 6:38 PM, Thomas Munro
> > Up until two minutes ago I assumed that policy would leave only two
> > possibilities: you attach to the DSM segment and attach to the
> > SharedBufFileManager successfully or you attach to the DSM segment and
> > then die horribly (but not throw) and the postmaster restarts the
> > whole cluster and blows all temp files away with RemovePgTempFiles().
> > But I see now in the comment of that function that crash-induced
> > restarts don't call that because "someone might want to examine the
> > temp files for debugging purposes".  Given that policy for regular
> > private BufFiles, I don't see why that shouldn't apply equally to
> > shared files: after a crash restart, you may have some junk files that
> > won't be cleaned up until your next clean restart, whether they were
> > private or shared BufFiles.
> 
> I think most people (other than Tom) would agree that that policy
> isn't really sensible any more; it probably made sense when the
> PostgreSQL user community was much smaller and consisted mostly of the
> people developing PostgreSQL, but these days it's much more likely to
> cause operational headaches than to help a developer debug.

FWIW, we have restart_after_crash = false. If you need to debug things,
you can enable that. Hence the whole RemovePgTempFiles() crash-restart
exemption isn't required anymore, we have a much more targeted solution.

- 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] [patch] pg_dump/pg_restore zerror() and strerror() mishap

[Alvaro Herrera]

Kunshchikov Vladimir wrote:
> Hello Alvaro,
> 
> here goes v4 version: removed unused header.
> 
> Compilation of this code snippet with -Wall -Wexter -std=c89 doesn't produce 
> any warnings.

Great, thanks!  I have pushed this to all branches since 9.4.  Would you
please give it a look?  Please let me know if you find any problems
(particularly since you seem to have a test rig to verify it on
corrupted files).

I noticed that with this patch we no longer use WRITE_ERROR_EXIT in
certain cases but instead do the printing/exiting directly.  I think
that's fine, but it would be neater to improve the WRITE_ERROR_EXIT
macro so that it takes the cfp as an argument, and then the macro is in
charge of calling get_cfp_error.  But then I noticed that it wasn't very
easy to improve things that way.  I also noticed that the usage of mixed
compressed/uncompressed file pointers in pg_dump is not very consistent,
and it would take rather a lot of effort to clean up.  So I gave up for
now, particularly as a backpatchable bugfix.  If you're interested in
mop-up work, I think we can improve pg_dump some more there.

At least, I think most common cases should correctly report any zlib
problems now.  Thanks for the patch!

-- 
Álvaro Herrerahttps://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] Why does logical replication launcher exit with exit code 1?

[Andres Freund]

On 2017-08-02 17:09:10 -0400, Tom Lane wrote:
> Even if we fix that, though, I think it is reasonable to downgrade it to
> DEBUG1.  We did that already for other standard background processes such
> as the autovac launcher, and it's not apparent to me why the logrep
> launcher should be chattier.  Now, *unexpected* starts or stops should
> certainly deserve a higher log rating ... but the general rule ought to be
> that totally-expected behavior does not deserve a log entry by default.

Well, every exit *other* than during a shutdown is unexpected.  That's
why I suggested changing the log level for exit code 1 depending on the
cluster being shut down or not.

- 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] Why does logical replication launcher exit with exit code 1?

[Tom Lane]

Andres Freund  writes:
> On 2017-08-02 16:52:01 -0400, Robert Haas wrote:
>> I actually don't think it's that unreasonable to get notified when
>> system-wide processes like the autovacuum launcher or the logical
>> replication launcher start or stop.  That's stuff somebody might want
>> to know.  It's not going to generate a lot of log volume, and it might
>> be useful, so why suppress it?

> I generally agree.  But in the shutdown case it's just useless and
> confusing - the launcher is stopping because the entire server is being
> stopped, and that's very much not clear from the message.

Yes.  The main complaint here is not about the existence of the message
but about the fact that it looks like it's reporting a failure.  I would
vote for removing it if it's not simple to fix that problem.

Even if we fix that, though, I think it is reasonable to downgrade it to
DEBUG1.  We did that already for other standard background processes such
as the autovac launcher, and it's not apparent to me why the logrep
launcher should be chattier.  Now, *unexpected* starts or stops should
certainly deserve a higher log rating ... but the general rule ought to be
that totally-expected behavior does not deserve a log entry by default.

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] Why does logical replication launcher exit with exit code 1?

[Andres Freund]

On 2017-08-02 16:52:01 -0400, Robert Haas wrote:
> On Wed, Aug 2, 2017 at 4:38 PM, Peter Eisentraut
>  wrote:
> > Maybe it doesn't need to be logged at all (other than perhaps as DEBUG)?
> >  A few months ago, people were complaining about too many messages about
> > background workers starting.  Now we are having complaints about
> > messages about background workers stopping.
> 
> I actually don't think it's that unreasonable to get notified when
> system-wide processes like the autovacuum launcher or the logical
> replication launcher start or stop.  That's stuff somebody might want
> to know.  It's not going to generate a lot of log volume, and it might
> be useful, so why suppress it?
> 
> Where things get ugly is if you start to get a high rate of messages -
> e.g. from starting and stopping parallel query workers or other kinds
> of things where you might have workers starting and stopping very
> frequently.  But surely this isn't an example of that.

I generally agree.  But in the shutdown case it's just useless and
confusing - the launcher is stopping because the entire server is being
stopped, and that's very much not clear from the message.

- 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] Why does logical replication launcher exit with exit code 1?

[Robert Haas]

On Wed, Aug 2, 2017 at 4:38 PM, Peter Eisentraut
 wrote:
> Maybe it doesn't need to be logged at all (other than perhaps as DEBUG)?
>  A few months ago, people were complaining about too many messages about
> background workers starting.  Now we are having complaints about
> messages about background workers stopping.

I actually don't think it's that unreasonable to get notified when
system-wide processes like the autovacuum launcher or the logical
replication launcher start or stop.  That's stuff somebody might want
to know.  It's not going to generate a lot of log volume, and it might
be useful, so why suppress it?

Where things get ugly is if you start to get a high rate of messages -
e.g. from starting and stopping parallel query workers or other kinds
of things where you might have workers starting and stopping very
frequently.  But surely this isn't an example of that.

-- 
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] POC: Cache data in GetSnapshotData()

[Andres Freund]

Hi,

I think this patch should have a "cover letter" explaining what it's
trying to achieve, how it does so and why it's safe/correct.  I think
it'd also be good to try to show some of the worst cases of this patch
(i.e. where the caching only adds overhead, but never gets used), and
some of the best cases (where the cache gets used quite often, and
reduces contention significantly).

I wonder if there's a way to avoid copying the snapshot into the cache
in situations where we're barely ever going to need it. But right now
the only way I can think of is to look at the length of the
ProcArrayLock wait queue and count the exclusive locks therein - which'd
be expensive and intrusive...


On 2017-08-02 15:56:21 +0530, Mithun Cy wrote:
> diff --git a/src/backend/storage/ipc/procarray.c 
> b/src/backend/storage/ipc/procarray.c
> index a7e8cf2..4d7debe 100644
> --- a/src/backend/storage/ipc/procarray.c
> +++ b/src/backend/storage/ipc/procarray.c
> @@ -366,11 +366,13 @@ ProcArrayRemove(PGPROC *proc, TransactionId latestXid)
>   (arrayP->numProcs - index - 1) * 
> sizeof(int));
>   arrayP->pgprocnos[arrayP->numProcs - 1] = -1;   /* for 
> debugging */
>   arrayP->numProcs--;
> + ProcGlobal->is_snapshot_valid = false;
>   LWLockRelease(ProcArrayLock);
>   return;
>   }
>   }
>  
> + ProcGlobal->is_snapshot_valid = false;
>   /* Oops */
>   LWLockRelease(ProcArrayLock);

Why do we need to do anything here? And if we need to, why not in
ProcArrayAdd?


> @@ -1552,6 +1557,8 @@ GetSnapshotData(Snapshot snapshot)
>errmsg("out of memory")));
>   }
>  
> + snapshot->takenDuringRecovery = RecoveryInProgress();
> +
>   /*
>* It is sufficient to get shared lock on ProcArrayLock, even if we are
>* going to set MyPgXact->xmin.
> @@ -1566,100 +1573,200 @@ GetSnapshotData(Snapshot snapshot)
>   /* initialize xmin calculation with xmax */
>   globalxmin = xmin = xmax;
>  
> - snapshot->takenDuringRecovery = RecoveryInProgress();
> -

This movement of code seems fairly unrelated?


>   if (!snapshot->takenDuringRecovery)
>   {

Do we really need to restrict this to !recovery snapshots? It'd be nicer
if we could generalize it - I don't immediately see anything !recovery
specific in the logic here.


> - int*pgprocnos = arrayP->pgprocnos;
> - int numProcs;
> + if (ProcGlobal->is_snapshot_valid)
> + {
> + Snapshotcsnap = >cached_snapshot;
> + TransactionId *saved_xip;
> + TransactionId *saved_subxip;
>  
> - /*
> -  * Spin over procArray checking xid, xmin, and subxids.  The 
> goal is
> -  * to gather all active xids, find the lowest xmin, and try to 
> record
> -  * subxids.
> -  */
> - numProcs = arrayP->numProcs;
> - for (index = 0; index < numProcs; index++)
> + saved_xip = snapshot->xip;
> + saved_subxip = snapshot->subxip;
> +
> + memcpy(snapshot, csnap, sizeof(SnapshotData));
> +
> + snapshot->xip = saved_xip;
> + snapshot->subxip = saved_subxip;
> +
> + memcpy(snapshot->xip, csnap->xip,
> +sizeof(TransactionId) * csnap->xcnt);
> + memcpy(snapshot->subxip, csnap->subxip,
> +sizeof(TransactionId) * csnap->subxcnt);
> + globalxmin = ProcGlobal->cached_snapshot_globalxmin;
> + xmin = csnap->xmin;
> +
> + count = csnap->xcnt;
> + subcount = csnap->subxcnt;
> + suboverflowed = csnap->suboverflowed;
> +
> + Assert(TransactionIdIsValid(globalxmin));
> + Assert(TransactionIdIsValid(xmin));
> + }

Can we move this to a separate function? In fact, could you check how
much effort it'd be to split cached, !recovery, recovery cases into
three static inline helper functions? This is starting to be hard to
read, the indentation added in this patch doesn't help... Consider doing
recovery, !recovery cases in a preliminary separate patch.

> +  * Let only one of the caller cache the computed 
> snapshot, and
> +  * others can continue as before.
>*/
> - if (!suboverflowed)
> + if (!ProcGlobal->is_snapshot_valid &&
> + 
> LWLockConditionalAcquire(>CachedSnapshotLock,
> + 
>  LW_EXCLUSIVE))
>   

Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Mon, Jul 31, 2017 at 7:59 AM, Ashutosh Bapat
 wrote:
> Adding AppendRelInfos to root->append_rel_list as and when they are
> created would keep parent AppendRelInfos before those of children. But
> that function throws away the AppendRelInfo it created when their are
> no real children i.e. in partitioned table's case when has no leaf
> partitions. So, we can't do that. Hence, I chose to change the API to
> return the list of AppendRelInfos when the given RTE has real
> children.

So, IIUC, the case you're concerned about is when you have a hierarchy
of only partitioned tables, with no plain tables.  For example, if B
is a partitioned table and a partition of A, and that's all there is,
A will recurse to B and B will return NIL.

Is it necessary to get rid of the extra AppendRelInfos, or are they
harmless like the duplicate RTE and PlanRowMark nodes?

/*
 * If all the children were temp tables or a partitioned parent did not
 * have any leaf partitions, pretend it's a non-inheritance situation; we
 * don't need Append node in that case.  The duplicate RTE we added for
 * the parent table is harmless, so we don't bother to get rid of it;
 * ditto for the useless PlanRowMark node.
 */
if (!need_append)
{
/* Clear flag before returning */
rte->inh = false;
return;
}

If we do need to get rid of the extra AppendRelInfos, maybe a less
invasive solution would be to change the if (!need_append) case to do
root->append_rel_list = list_truncate(root->append_rel_list,
original_append_rel_length).

> The code avoids creating AppendRelInfos for a child which represents
> the parent in its role as a simple member of inheritance set.

OK, I suggest we rewrite the whole comment like this: "We need an
AppendRelInfo if paths will be built for the child RTE.  If
childrte->inh is true, then we'll always need to generate append paths
for it.  If childrte->inh is false, we must scan it if it's not a
partitioned table; but if it is a partitioned table, then it never has
any data of its own and need not be scanned.  It does, however, need
to be locked, so note the OID for inclusion in the
PartitionedChildRelInfo we're going to build."

It looks like you also need to update the header comment for
AppendRelInfo itself, in nodes/relation.h.

+ * PlannerInfo for every child is obtained by translating relevant members

Insert "The" at the start of the sentence.

-subroot->parse = (Query *)
-adjust_appendrel_attrs(root,
-   (Node *) parse,
-   appinfo);
+subroot->parse = (Query *) adjust_appendrel_attrs(parent_root,
+  (Node *)
parent_parse,
+  1, );

I suggest that you don't remove the line break after the cast.

+ * If the child is further partitioned, remember it as a parent. Since
+ * partitioned tables do not have any data, we don't need to create
+ * plan for it. We just need its PlannerInfo set up to be used while
+ * planning its children.

Most of this comment is in the singular, but the first half of the
second sentence is plural.  Should be "Since a partitioned table does
not have any data...".  I might replace the last sentence by "We do,
however, need to remember the PlannerInfo for use when planning its
children."

+-- Check UPDATE with *multi-level partitioned* inherited target

Asterisks seem like overkill.

Since expand_inherited_rtentry() and set_append_rel_size() can now
recurse down to as many levels as there are levels in the inheritance
hierarchy, they should probably have a check_stack_depth() check.

>> Overall I think this is a reasonable direction to go but I'm worried
>> that there may be bugs lurking -- other code that needs adjusting that
>> hasn't been found, really.
>>
> Planner code is already aware of such hierarchies except DMLs, which
> this patch adjusts. We have fixed issues revealed by mine and
> Rajkumar's testing.
> What kinds of things you suspect?

I'm not sure exactly.  It's just hard with this kind of patch to make
sure you've caught everything.

-- 
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] Why does logical replication launcher exit with exit code 1?

[Peter Eisentraut]

On 8/1/17 20:20, Andres Freund wrote:
> Well, that's how it is for all bgworkers - maybe a better solution is to
> adjust that message in the postmaster rather than fiddle with the worker
> exist code?  Seems like we could easily take pmStatus into account
> inside LogChildExit() and set the log level to DEBUG1 even for
> EXIT_STATUS_1 in that case?  Additionally we probably should always log
> a better message for bgworkers exiting with exit 1, something about
> unregistering the worker or such.

Maybe it doesn't need to be logged at all (other than perhaps as DEBUG)?
 A few months ago, people were complaining about too many messages about
background workers starting.  Now we are having complaints about
messages about background workers stopping.

-- 
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] [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.

[Robert Haas]

On Tue, Jul 25, 2017 at 9:54 PM, Kyotaro HORIGUCHI
 wrote:
> The patch is a kind of straightforward and looks fine for me.

+1 for this change.

-- 
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] WIP: Failover Slots

[Robert Haas]

On Tue, Jul 25, 2017 at 8:44 PM, Craig Ringer  wrote:
> No. The whole approach seems to have been bounced from core. I don't agree
> and continue to think this functionality is desirable but I don't get to
> make that call.

I actually think failover slots are quite desirable, especially now
that we've got logical replication in core.  In a review of this
thread I don't see anyone saying otherwise.  The debate has really
been about the right way of implementing that.  Suppose we did
something like this:

- When a standby connects to a master, it can optionally supply a list
of slot names that it cares about.
- The master responds by periodically notifying the standby of changes
to the slot contents using some new replication sub-protocol message.
- The standby applies those updates to its local copies of the slots.

So, you could create a slot on a standby with an "uplink this" flag of
some kind, and it would then try to keep it up to date using the
method described above.  It's not quite clear to me how to handle the
case where the corresponding slot doesn't exist on the master, or
initially does but then it's later dropped, or it initially doesn't
but it's later created.

Thoughts?

-- 
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] Subscription code improvements

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Alvaro Herrera  writes:
> > I wish you'd stop splitting error message strings across multiple lines.
> > I've been trapped by a faulty grep not matching a split error message a
> > number of times :-(  I know by now to remove words until I get a match,
> > but it seems an unnecessary trap for the unwary.
> 
> Yeah, that's my number one reason for not splitting error messages, too.
> It's particularly nasty if similar strings appear in multiple places and
> they're not all split alike, as you can get misled into thinking that a
> reported error must have occurred in a place you found, rather than
> someplace you didn't.

+1.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Subscription code improvements

[Tom Lane]

Alvaro Herrera  writes:
> I wish you'd stop splitting error message strings across multiple lines.
> I've been trapped by a faulty grep not matching a split error message a
> number of times :-(  I know by now to remove words until I get a match,
> but it seems an unnecessary trap for the unwary.

Yeah, that's my number one reason for not splitting error messages, too.
It's particularly nasty if similar strings appear in multiple places and
they're not all split alike, as you can get misled into thinking that a
reported error must have occurred in a place you found, rather than
someplace you didn't.

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] Subscription code improvements

[Alvaro Herrera]

Petr Jelinek wrote:

> I split it into several patches:

I wish you'd stop splitting error message strings across multiple lines.
I've been trapped by a faulty grep not matching a split error message a
number of times :-(  I know by now to remove words until I get a match,
but it seems an unnecessary trap for the unwary.

-- 
Álvaro Herrerahttps://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] Subscription code improvements

[Peter Eisentraut]

On 8/1/17 00:17, Noah Misch wrote:
> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

I'm looking into this now and will report by Friday.

-- 
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] reload-through-the-top-parent switch the partition table

[Peter Eisentraut]

On 8/2/17 13:58, Tom Lane wrote:
> I notice that the option list already includes some references to
> "insert", so maybe "--insert-via-partition-root"?  Although you could
> argue that that's confusing when we're using COPY.

"load" could be more general.  But I'm also OK with "restore".

-- 
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] Confusing error message in pgbench

[Tom Lane]

Robert Haas  writes:
> On Tue, Aug 1, 2017 at 10:03 PM, Tatsuo Ishii  wrote:
>> I found an error message in pgbench is quite confusing.

> Not really objecting, but an even better fix might be to remove the
> restriction on the order in which the options can be specified.

Indeed.  It doesn't look that hard: AFAICS the problem is just that
process_sql_command() is making premature decisions about whether to
extract parameters from the SQL commands.  Proposed patch attached.

regards, tom lane

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4d364a1..ae78c7b 100644
*** a/src/bin/pgbench/pgbench.c
--- b/src/bin/pgbench/pgbench.c
*** init(bool is_no_vacuum)
*** 2844,2858 
  }
  
  /*
!  * Parse the raw sql and replace :param to $n.
   */
  static bool
! parseQuery(Command *cmd, const char *raw_sql)
  {
  	char	   *sql,
  			   *p;
  
! 	sql = pg_strdup(raw_sql);
  	cmd->argc = 1;
  
  	p = sql;
--- 2844,2861 
  }
  
  /*
!  * Replace :param with $n throughout the command's SQL text, which
!  * is a modifiable string in cmd->argv[0].
   */
  static bool
! parseQuery(Command *cmd)
  {
  	char	   *sql,
  			   *p;
  
! 	/* We don't want to scribble on cmd->argv[0] until done */
! 	sql = pg_strdup(cmd->argv[0]);
! 
  	cmd->argc = 1;
  
  	p = sql;
*** parseQuery(Command *cmd, const char *raw
*** 2874,2880 
  
  		if (cmd->argc >= MAX_ARGS)
  		{
! 			fprintf(stderr, "statement has too many arguments (maximum is %d): %s\n", MAX_ARGS - 1, raw_sql);
  			pg_free(name);
  			return false;
  		}
--- 2877,2884 
  
  		if (cmd->argc >= MAX_ARGS)
  		{
! 			fprintf(stderr, "statement has too many arguments (maximum is %d): %s\n",
! 	MAX_ARGS - 1, cmd->argv[0]);
  			pg_free(name);
  			return false;
  		}
*** parseQuery(Command *cmd, const char *raw
*** 2886,2891 
--- 2890,2896 
  		cmd->argc++;
  	}
  
+ 	pg_free(cmd->argv[0]);
  	cmd->argv[0] = sql;
  	return true;
  }
*** process_sql_command(PQExpBuffer buf, con
*** 2983,2992 
  	my_command = (Command *) pg_malloc0(sizeof(Command));
  	my_command->command_num = num_commands++;
  	my_command->type = SQL_COMMAND;
- 	my_command->argc = 0;
  	initSimpleStats(_command->stats);
  
  	/*
  	 * If SQL command is multi-line, we only want to save the first line as
  	 * the "line" label.
  	 */
--- 2988,3003 
  	my_command = (Command *) pg_malloc0(sizeof(Command));
  	my_command->command_num = num_commands++;
  	my_command->type = SQL_COMMAND;
  	initSimpleStats(_command->stats);
  
  	/*
+ 	 * Install query text as the sole argv string.  If we are using a
+ 	 * non-simple query mode, we'll extract parameters from it later.
+ 	 */
+ 	my_command->argv[0] = pg_strdup(p);
+ 	my_command->argc = 1;
+ 
+ 	/*
  	 * If SQL command is multi-line, we only want to save the first line as
  	 * the "line" label.
  	 */
*** process_sql_command(PQExpBuffer buf, con
*** 3000,3020 
  	else
  		my_command->line = pg_strdup(p);
  
- 	switch (querymode)
- 	{
- 		case QUERY_SIMPLE:
- 			my_command->argv[0] = pg_strdup(p);
- 			my_command->argc++;
- 			break;
- 		case QUERY_EXTENDED:
- 		case QUERY_PREPARED:
- 			if (!parseQuery(my_command, p))
- exit(1);
- 			break;
- 		default:
- 			exit(1);
- 	}
- 
  	return my_command;
  }
  
--- 3011,3016 
*** main(int argc, char **argv)
*** 3896,3906 
  break;
  			case 'M':
  benchmarking_option_set = true;
- if (num_scripts > 0)
- {
- 	fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f or -b)\n");
- 	exit(1);
- }
  for (querymode = 0; querymode < NUM_QUERYMODE; querymode++)
  	if (strcmp(optarg, QUERYMODE[querymode]) == 0)
  		break;
--- 3892,3897 
*** main(int argc, char **argv)
*** 4006,4011 
--- 3997,4020 
  		internal_script_used = true;
  	}
  
+ 	/* if not simple query mode, parse the script(s) to find parameters */
+ 	if (querymode != QUERY_SIMPLE)
+ 	{
+ 		for (i = 0; i < num_scripts; i++)
+ 		{
+ 			Command   **commands = sql_script[i].commands;
+ 			int			j;
+ 
+ 			for (j = 0; commands[j] != NULL; j++)
+ 			{
+ if (commands[j]->type != SQL_COMMAND)
+ 	continue;
+ if (!parseQuery(commands[j]))
+ 	exit(1);
+ 			}
+ 		}
+ 	}
+ 
  	/* compute total_weight */
  	for (i = 0; i < num_scripts; i++)
  		/* cannot overflow: weight is 32b, total_weight 64b */

-- 
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] Function Volatility and Views Unexpected Behavior

[Robert Haas]

On Wed, Jul 12, 2017 at 3:23 PM, Tom Lane  wrote:
> David Kohn  writes:
>> I encountered some unexpected behavior when debugging a query that was
>> taking longer than expected, basically, a volatile function that makes a
>> column in a view is called even when that column is not selected in the
>> query, making it so that the function is called for every row in the view,
>> I'm not sure that that would necessarily be the expected behavior, as it
>> was my understanding that columns that are not selected are not evaluated,
>> for instance if there was a join in a view that produced some columns and
>> said columns were not selected, I would expect it to be optimized away.
>
> No, this is the expected behavior; we don't like optimization to change
> the number of calls of a volatile function from what would occur in naive
> evaluation of the query.  If that prospect doesn't bother you, it's
> likely because your function isn't really volatile ...

I don't think I agree with that.  If something is VOLATILE, that means
you want it to be recalculated each time, but it doesn't necessarily
mean that you want it calculated if it in no way changes the result
set.

I guess maybe there's a difference between a VOLATILE function like
random(), which is expected to produce a different answer each time
but probably has no side effects that you care about (unless you care
about the fact that the state of the PRNG has changed) and pg_sleep(),
whose return value is always the same but whose side effects are of
critical importance.  Maybe we need separate terms for
volatile-because-the-answer-is-unstable and
volatile-because-it-has-side-effects.

-- 
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] Gettting warning message during PostgreSQL-9.5 installation on Windows

[Tom Lane]

Ashutosh Sharma  writes:
> I am getting this warning message when trying to install
> PostgreSQL-v9.5 on Windows with Perl-5.22 and above,
> Unescaped left brace in regex is deprecated, passed through in regex;
> Please note that from perl-5.26 onwards, this is considered as a
> syntax error instead of warning.

Mmm, yeah, we'd better fix it then, because people will surely try
to use older branches with current Perl.  Pushed.

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] reload-through-the-top-parent switch the partition table

[Robert Haas]

On Wed, Aug 2, 2017 at 1:58 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Aug 2, 2017 at 1:08 PM, Tom Lane  wrote:
>>> --restore-via-partition-root ?
>
>> I worry someone will think that pg_dump is now restoring stuff, but it isn't.
>
> Well, the point is that the commands it emits will cause the eventual
> restore to go through the root.  Anyway, I think trying to avoid using
> a verb altogether is going to result in a very stilted option name.
>
> I notice that the option list already includes some references to
> "insert", so maybe "--insert-via-partition-root"?  Although you could
> argue that that's confusing when we're using COPY.

Yeah, that's definitely confusing.  I realize that my verbless version
is a little odd, but there are numerous precedents for it: --inserts,
--column-inserts, --if-exists, --strict-names, ...

-- 
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] pgbench: Skipping the creating primary keys after initialization

[Michael Paquier]

On Wed, Aug 2, 2017 at 5:50 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Aug 2, 2017 at 10:17 AM, Tom Lane  wrote:
>>> Or in other words, this looks to me quite a bit like the hackery
>>> that resulted in pgbench's -S and -N options, before we figured out
>>> that making it scriptable was a better answer.
>
>> But it's not very clear to me how we could make this case scriptable,
>
> Well, I'm imagining that "-i" would essentially become a short form
> of "-b initialize", as already happened for -S and -N, where the script
> looks something like

Yes, I would imagine a facility where one could do pgbench $script and
issue a complete test set. Here is for example a funky idea: let's
separate each script with a set of meta-commands, \init being what is
used just for initialization, and then use \script to define a set of
commands with a custom weight. Say:
\init
CREATE TABLE foo(a int);
\script select_query [weight N]
SELECT count(*) FROM foo;
\script insert_query [weight N]
INSERT INTO foo VALUES ('1');

That may be over-engineering things, but personally I don't like much
having just a switch to remove indexes. Next time we will come with
another option that only selects a portion of the indexes created.
-- 
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] On Complex Source Code Reading Strategy

[Michael Paquier]

On Wed, Aug 2, 2017 at 7:24 AM, Zeray Kalayu  wrote:
> Lastly, I strongly believe that Code is the ultimate truth and being
> able to understand complex and high quality code effectively and
> strategically is of paramount importance.

Documentation to understand how a system works from the user
prospective, and comments in the code itself are also important
properties of a code that can be considered as a good base. Postgres
has both.
-- 
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] reload-through-the-top-parent switch the partition table

[David G. Johnston]

On Wed, Aug 2, 2017 at 10:58 AM, Tom Lane  wrote:

> Robert Haas  writes:
> > On Wed, Aug 2, 2017 at 1:08 PM, Tom Lane  wrote:
> >> --restore-via-partition-root ?
>
> > I worry someone will think that pg_dump is now restoring stuff, but it
> isn't.
>
> Well, the point is that the commands it emits will cause the eventual
> restore to go through the root.  Anyway, I think trying to avoid using
> a verb altogether is going to result in a very stilted option name.
>
> I notice that the option list already includes some references to
> "insert", so maybe "--insert-via-partition-root"?  Although you could
> argue that that's confusing when we're using COPY.


--use-partitioned-table [partition-name, ...]  # if names are omitted it
defaults to all partitioned tables

I don't know that we need to use "root" in the argument name to communicate
"the top-most if partitioned tables are nested".  We have the docs to
describe exactly what it does.  "Partitioned Table" is what we are calling
the main routing table in the docs.  "Use" seems adequate.

FWIW my first thought was "--insert-via-partition-root" and I didn't mind
that "COPY" commands would be affected implicitly.

David J.

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Tom Lane]

Alvaro Herrera  writes:
> Peter Eisentraut wrote:
>> The actual error, from the perspective of the user, is something like
>> ERROR: "someview" is a view
>> DETAIL: Views cannot have constraints.

> OK.  "%s is a %s" is a reasonable set of errors -- we just need one for
> each relkind.  So the first one is easy.

> But the second one is not easy, because we'd need one message per
> relkind per operation kind.  We cannot possibly write/translate that
> many messages.  If we make the relkind generic in the errdetail message,
> maybe it can work; something like "Relations of that type cannot have
> constraints" would work, for example.  Or "Relations of type "view"
> cannot have constraints", although this reads very strangely.  Maybe
> someone has a better idea?

I think Peter's got the error and the detail backwards.  It should be
more like

ERROR: "someview" cannot have constraints
DETAIL: "someview" is a view.

If we do it like that, we need one ERROR message per error reason,
and one DETAIL per relkind, which should be manageable.

A more verbose approach is

ERROR: "someview" cannot have constraints
DETAIL: "someview" is a view, which is not a supported kind of relation
for this 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] reload-through-the-top-parent switch the partition table

[Tom Lane]

Robert Haas  writes:
> On Wed, Aug 2, 2017 at 1:08 PM, Tom Lane  wrote:
>> --restore-via-partition-root ?

> I worry someone will think that pg_dump is now restoring stuff, but it isn't.

Well, the point is that the commands it emits will cause the eventual
restore to go through the root.  Anyway, I think trying to avoid using
a verb altogether is going to result in a very stilted option name.

I notice that the option list already includes some references to
"insert", so maybe "--insert-via-partition-root"?  Although you could
argue that that's confusing when we're using COPY.

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(wait_for_archive := true) on standby server

[Stephen Frost]

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Mon, Jul 31, 2017 at 9:13 PM, Stephen Frost  wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> On Thu, Jul 27, 2017 at 10:27 AM, Stephen Frost  wrote:
> >> > * Noah Misch (n...@leadboat.com) wrote:
> >> >> This PostgreSQL 10 open item is past due for your status update.  
> >> >> Kindly send
> >> >> a status update within 24 hours, and include a date for your subsequent 
> >> >> status
> >> >> update.  Refer to the policy on open item ownership:
> >> >
> >> > Based on the ongoing discussion, this is really looking like it's
> >> > actually a fix that needs to be back-patched to 9.6 rather than a PG10
> >> > open item.  I don't have any issue with keeping it as an open item
> >> > though, just mentioning it.  I'll provide another status update on or
> >> > before Monday, July 31st.
> >> >
> >> > I'll get to work on the back-patch and try to draft up something to go
> >> > into the release notes for 9.6.4.
> >>
> >> Whether this is going to be back-patched or not, you should do
> >> something about it quickly, because we're wrapping a new beta and a
> >> full set of back-branch releases next week.  I'm personally hoping
> >> that what follows beta3 will be rc1, but if we have too much churn
> >> after beta3 we'll end up with a beta4, which could end up slipping the
> >> whole release cycle.
> >
> > Yes, I've been working on this and the other issues with pg_dump today.
> 
> Do you need a back-patchable version for 9.6? I could get one out of
> my pocket if necessary.

I was just trying to find a bit of time to generate exactly that- if
you have a couple spare cycles, it would certainly help.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server

[Michael Paquier]

On Mon, Jul 31, 2017 at 9:13 PM, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Thu, Jul 27, 2017 at 10:27 AM, Stephen Frost  wrote:
>> > * Noah Misch (n...@leadboat.com) wrote:
>> >> This PostgreSQL 10 open item is past due for your status update.  Kindly 
>> >> send
>> >> a status update within 24 hours, and include a date for your subsequent 
>> >> status
>> >> update.  Refer to the policy on open item ownership:
>> >
>> > Based on the ongoing discussion, this is really looking like it's
>> > actually a fix that needs to be back-patched to 9.6 rather than a PG10
>> > open item.  I don't have any issue with keeping it as an open item
>> > though, just mentioning it.  I'll provide another status update on or
>> > before Monday, July 31st.
>> >
>> > I'll get to work on the back-patch and try to draft up something to go
>> > into the release notes for 9.6.4.
>>
>> Whether this is going to be back-patched or not, you should do
>> something about it quickly, because we're wrapping a new beta and a
>> full set of back-branch releases next week.  I'm personally hoping
>> that what follows beta3 will be rc1, but if we have too much churn
>> after beta3 we'll end up with a beta4, which could end up slipping the
>> whole release cycle.
>
> Yes, I've been working on this and the other issues with pg_dump today.

Do you need a back-patchable version for 9.6? I could get one out of
my pocket if necessary.
-- 
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] Macros bundling RELKIND_* conditions

[Alvaro Herrera]

Peter Eisentraut wrote:

> I don't find this style of error message optimal anyway.  If I do, for
> example
> 
> ALTER TABLE someview ADD CONSTRAINT ...
> ERROR: "someview" is not a table, foreign table, whatever
> 
> then this information is not helpful.  It's not like I'm going to turn
> my view into a foreign table in order to be able to proceed with that
> command.

Hmm, this is a good point ... not against my proposal, but rather
against the current coding.  I agree it could be more user-friendly.

> The actual error, from the perspective of the user, is something like
> 
> ERROR: "someview" is a view
> DETAIL: Views cannot have constraints.

OK.  "%s is a %s" is a reasonable set of errors -- we just need one for
each relkind.  So the first one is easy.

But the second one is not easy, because we'd need one message per
relkind per operation kind.  We cannot possibly write/translate that
many messages.  If we make the relkind generic in the errdetail message,
maybe it can work; something like "Relations of that type cannot have
constraints" would work, for example.  Or "Relations of type "view"
cannot have constraints", although this reads very strangely.  Maybe
someone has a better idea?

-- 
Álvaro Herrerahttps://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] reload-through-the-top-parent switch the partition table

[Robert Haas]

On Wed, Aug 2, 2017 at 1:08 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> The patch itself looks just fine on a quick glance, modulo the lack of
>> documentation, but I think we need to bikeshed the name of the flag.
>> --reload-through-root is clear as daylight to me, but I'm not sure
>> users will agree.   The lack of the word "partition" is perhaps a
>> significant flaw, and pg_dump doesn't really reload anything; it just
>> dumps.
>
>> The best thing I can come up with after brief thought is
>> --partition-data-via-root, but maybe somebody else has a better idea?
>
> --restore-via-partition-root ?

I worry someone will think that pg_dump is now restoring stuff, but it isn't.

-- 
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] Macros bundling RELKIND_* conditions

[Alvaro Herrera]

Alvaro Herrera wrote:
> I think pg_class is a reasonable place to put more generic relkind lists
> alongside a matching error message for each, rather than specialized
> "does this relkind have storage" macros.  What about something like a
> struct list in pg_class.h,

I just noticed that this doesn't help at all with the initial problem
statement, which is that some of the relkind checks failed to notice
that partitioned tables needed to be added to the set.  Maybe it still
helps because you have something to grep for, as Tom proposed elsewhere.

However, if there are multiple places that should be kept in sync
regarding which relkinds to check, then I don't understand Robert's
objection that only one place requires the check.  Surely we're having
this discussion precisely because more than one place needs the check,
and finding those places is not obvious?

-- 
Álvaro Herrerahttps://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] Macros bundling RELKIND_* conditions

[Peter Eisentraut]

On 8/2/17 13:28, Alvaro Herrera wrote:
> I think pg_class is a reasonable place to put more generic relkind lists
> alongside a matching error message for each, rather than specialized
> "does this relkind have storage" macros.  What about something like a
> struct list in pg_class.h,
> 
> {
> {
>   relkinds_r_i_T,
>   { 'r', 'i', 'T' },
>   gettext_noop("relation %s is not a table, index or toast table")
> },
> ...
> }

I don't find this style of error message optimal anyway.  If I do, for
example

ALTER TABLE someview ADD CONSTRAINT ...
ERROR: "someview" is not a table, foreign table, whatever

then this information is not helpful.  It's not like I'm going to turn
my view into a foreign table in order to be able to proceed with that
command.  The actual error, from the perspective of the user, is
something like

ERROR: "someview" is a view
DETAIL: Views cannot have constraints.

(Maybe they can.  This is an example.)

-- 
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] why not parallel seq scan for slow functions

[Jeff Janes]

On Wed, Jul 12, 2017 at 7:08 PM, Amit Kapila 
wrote:

> On Wed, Jul 12, 2017 at 11:20 PM, Jeff Janes  wrote:
> > On Tue, Jul 11, 2017 at 10:25 PM, Amit Kapila 
> > wrote:
> >>
> >> On Wed, Jul 12, 2017 at 1:50 AM, Jeff Janes 
> wrote:
> >> > On Mon, Jul 10, 2017 at 9:51 PM, Dilip Kumar 
> >> > wrote:
> >> >>
> >> >> So because of this high projection cost the seqpath and parallel path
> >> >> both have fuzzily same cost but seqpath is winning because it's
> >> >> parallel safe.
> >> >
> >> >
> >> > I think you are correct.  However, unless parallel_tuple_cost is set
> >> > very
> >> > low, apply_projection_to_path never gets called with the Gather path
> as
> >> > an
> >> > argument.  It gets ruled out at some earlier stage, presumably because
> >> > it
> >> > assumes the projection step cannot make it win if it is already behind
> >> > by
> >> > enough.
> >> >
> >>
> >> I think that is genuine because tuple communication cost is very high.
> >
> >
> > Sorry, I don't know which you think is genuine, the early pruning or my
> > complaint about the early pruning.
> >
>
> Early pruning.  See, currently, we don't have a way to maintain both
> parallel and non-parallel paths till later stage and then decide which
> one is better. If we want to maintain both parallel and non-parallel
> paths, it can increase planning cost substantially in the case of
> joins.  Now, surely it can have benefit in many cases, so it is a
> worthwhile direction to pursue.
>

If I understand it correctly, we have a way, it just can lead to
exponential explosion problem, so we are afraid to use it, correct?  If I
just lobotomize the path domination code (make pathnode.c line 466 always
test false)

if (JJ_all_paths==0 && costcmp != COSTS_DIFFERENT)

Then it keeps the parallel plan and later chooses to use it (after applying
your other patch in this thread) as the overall best plan.  It even doesn't
slow down "make installcheck-parallel" by very much, which I guess just
means the regression tests don't have a lot of complex joins.

But what is an acceptable solution?  Is there a heuristic for when
retaining a parallel path could be helpful, the same way there is for
fast-start paths?  It seems like the best thing would be to include the
evaluation costs in the first place at this step.

Why is the path-cost domination code run before the cost of the function
evaluation is included?  Is that because the information needed to compute
it is not available at that point, or because it would be too slow to
include it at that point? Or just because no one thought it important to do?

Cheers,

Jeff

Re: [HACKERS] Walsender timeouts and large transactions

[Yura Sokolov]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

There is no check for (last_reply_timestamp <= 0 || wal_sender_timeout <= 0) as 
in other places
(in WalSndKeepaliveIfNecessary for example).

I don't think moving update of 'now' down to end of loop body is correct:
there are calls to ProcessConfigFile with SyncRepInitConfig, 
ProcessRepliesIfAny that can
last non-negligible time. It could lead to over sleeping due to larger computed 
sleeptime.
Though I could be mistaken.

I'm not sure about moving `if (!pg_is_send_pending())` in a body loop after 
WalSndKeepaliveIfNecessary.
Is it necessary? But it looks harmless at least.

Could patch be reduced to check after first `if (!pg_is_sendpending())` ? like:

if (!pq_is_send_pending())
-   return;
+   {
+   if (last_reply_timestamp <= 0 || wal_sender_timeout <= 0)
+   {
+   CHECK_FOR_INTERRUPTS();
+   return;
+   }
+   if (now <= TimestampTzPlusMilliseconds(last_reply_timestamp, 
wal_sender_timeout / 2))
+   return;
+   }

If not, what problem prevents?

The new status of this patch is: Waiting on Author

-- 
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] Macros bundling RELKIND_* conditions

[Alvaro Herrera]

I think pg_class is a reasonable place to put more generic relkind lists
alongside a matching error message for each, rather than specialized
"does this relkind have storage" macros.  What about something like a
struct list in pg_class.h,

{
{
relkinds_r_i_T,
{ 'r', 'i', 'T' },
gettext_noop("relation %s is not a table, index or toast table")
},
...
}

and then in the .c checks you do something like

relkinds = relkind_r_i_T;
if (rel_doesnt_match(rel, relkinds))
ereport(ERROR, (errcode(...),
errmsg(relkinds_get_message(relkinds)));

then, in order to update the set of relkinds that some particular
operation works with, you just need to change the "relkinds" variable,
and the message is automatically up to date (you may need to add a new
entry, if there isn't one for the set you want, but the number of
permutations needed shouldn't grow too large).  This doesn't create a
problem for translators because you're not constructing an error
message, and it doesn't pollute pg_class.h with things that don't really
belong there.

One possible objection is that rel_doesnt_match() in the above
formulation is slow, because it has to scan the entire list.  Maybe this
is not a problem because the list isn't large anyway, or maybe there's
some better formulation -- for example, we could assign a distinct bit
to each relkind, and create bitmasks for each set; then figuring out
whether there's a match or not is just a matter of bit-anding.

-- 
Álvaro Herrerahttps://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] reload-through-the-top-parent switch the partition table

[Tom Lane]

Robert Haas  writes:
> The patch itself looks just fine on a quick glance, modulo the lack of
> documentation, but I think we need to bikeshed the name of the flag.
> --reload-through-root is clear as daylight to me, but I'm not sure
> users will agree.   The lack of the word "partition" is perhaps a
> significant flaw, and pg_dump doesn't really reload anything; it just
> dumps.

> The best thing I can come up with after brief thought is
> --partition-data-via-root, but maybe somebody else has a better idea?

--restore-via-partition-root ?

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] pgbench: Skipping the creating primary keys after initialization

[Robert Haas]

On Wed, Aug 2, 2017 at 12:34 PM, Tom Lane  wrote:
> Of course.  It's also a heck of a lot more flexible.  Adding on another
> ad-hoc option that does the minimum possible amount of work needed to
> address one specific problem is always going to be less work; but after
> we repeat that process five or ten times, we're going to have a mess.

Well, I still like Masahiko-san's proposal, but I'm not prepared to
keep arguing about it right now.  Maybe some other people will weigh
in with an opinion.

-- 
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] Macros bundling RELKIND_* conditions

[Tom Lane]

Robert Haas  writes:
> I've thought about this kind of thing, too.  But the thing is that
> most of these macros you're proposing to introduce only get used in
> one place.

I think the value would be in having a centralized checklist of
things-to-fix-when-adding-a-new-relkind.  There's more than one way
to reach that goal, though.  I wonder whether the task should be defined
more like "grep for 'RELKIND_' and fix every place you find that".
If there are places to touch that fail to mention that string, fix
them, using comments if nothing else.  (But see fe797b4a6 and
followon commits for other solutions.)

> I think this might cause some problems for translators.

Yeah, the error messages that list a bunch of different relkinds in text
form are going to be a hassle no matter what.  Most of the ways you might
think of to change that will violate our translatability rules.

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] Re: [BUGS] BUG #14758: Segfault with logical replication on a function index

[Peter Eisentraut]

On 8/1/17 16:29, Peter Eisentraut wrote:
> On 8/1/17 00:21, Noah Misch wrote:
>> On Mon, Jul 31, 2017 at 09:40:34AM +0900, Masahiko Sawada wrote:
>>> On Sat, Jul 29, 2017 at 4:35 AM, Scott Milliken  wrote:
 Thank you Masahiko! I've tested and confirmed that this patch fixes the
 problem.

>>>
>>> Thank you for the testing. This issue should be added to the open item
>>> since this cause of the server crash. I'll add it.
>>
>> [Action required within three days.  This is a generic notification.]
> 
> I'm looking into this now and will report back on Thursday.

This item has been closed.

-- 
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] pgbench: Skipping the creating primary keys after initialization

[Tom Lane]

Robert Haas  writes:
> On Wed, Aug 2, 2017 at 11:50 AM, Tom Lane  wrote:
>> Well, I'm imagining that "-i" would essentially become a short form
>> of "-b initialize", as already happened for -S and -N, where the script
>> looks something like ...

> I imagine that would be useful for some use cases, but it's a heck of
> a lot more work than just writing --no-indexes-please.

Of course.  It's also a heck of a lot more flexible.  Adding on another
ad-hoc option that does the minimum possible amount of work needed to
address one specific problem is always going to be less work; but after
we repeat that process five or ten times, we're going to have a mess.

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] pgbench: Skipping the creating primary keys after initialization

[Robert Haas]

On Wed, Aug 2, 2017 at 11:50 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Aug 2, 2017 at 10:17 AM, Tom Lane  wrote:
>>> Or in other words, this looks to me quite a bit like the hackery
>>> that resulted in pgbench's -S and -N options, before we figured out
>>> that making it scriptable was a better answer.
>
>> But it's not very clear to me how we could make this case scriptable,
>
> Well, I'm imagining that "-i" would essentially become a short form
> of "-b initialize", as already happened for -S and -N, where the script
> looks something like
>
> drop table if exists pgbench_branches;
> create table pgbench_branches (
>   bid int not null,bbalance int,filler char(88)
> );
> \load_data pgbench_branches [ other parameters to-be-determined ]
> alter table pgbench_branches add primary key (bid);
> ... repeat for other tables ...
>
> and we'd document that the same way we do for the existing built-in
> scripts.  Then, if there's something you don't like about it, you
> just paste the script into a file and edit to taste.

I imagine that would be useful for some use cases, but it's a heck of
a lot more work than just writing --no-indexes-please.

-- 
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] pgbench: Skipping the creating primary keys after initialization

[Tom Lane]

Robert Haas  writes:
> On Wed, Aug 2, 2017 at 10:17 AM, Tom Lane  wrote:
>> Or in other words, this looks to me quite a bit like the hackery
>> that resulted in pgbench's -S and -N options, before we figured out
>> that making it scriptable was a better answer.

> But it's not very clear to me how we could make this case scriptable,

Well, I'm imagining that "-i" would essentially become a short form
of "-b initialize", as already happened for -S and -N, where the script
looks something like

drop table if exists pgbench_branches;
create table pgbench_branches (
  bid int not null,bbalance int,filler char(88)
);
\load_data pgbench_branches [ other parameters to-be-determined ]
alter table pgbench_branches add primary key (bid);
... repeat for other tables ...

and we'd document that the same way we do for the existing built-in
scripts.  Then, if there's something you don't like about it, you
just paste the script into a file and edit to taste.

I'm sure there's complexities that would only become apparent when
someone tries to write the patch, but that seems to me like a better
foundation for this class of desires than extending the option set
with various one-off options having no discernible architecture.

> If you just want to create
> different/extra indexes, you can do that yourself.

Sure, but there's no end to the number of small variations on this
theme that somebody might want.  For example, we realized years ago
that the "filler" fields as-implemented don't really meet the intent
of the TPC-B spec (cf comment in the init() function).  If someone
comes along with a patch adding a "--really-tpc-b" option to change
the table declarations and/or data loading code to fix that, will we
take that patch?  What about one that wants all the id fields (not
just accounts.aid) to be bigint, or one that wants the balance fields
to be numeric?

You can say "let 'em set up the tables manually if they want that",
but I don't see why a nonstandard set of indexes is much different.

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] pgsql 10: hash indexes testing

[Robert Haas]

On Wed, Jul 12, 2017 at 1:10 AM, Amit Kapila  wrote:
>>> Yes, I also think the same idea can be used, in fact, I have mentioned
>>> it [1] as soon as you have committed that patch.  Do we want to do
>>> anything at this stage for PG-10?  I don't think we should attempt
>>> something this late unless people feel this is a show-stopper issue
>>> for usage of hash indexes.  If required, I think a separate function
>>> can be provided to allow users to perform squeeze operation.
>>
>> Sorry, I have no idea how critical this squeeze thing is for the
>> newfangled hash indexes, so I cannot comment on that.  Does this make
>> the indexes unusable in some way under some circumstances?
>
> It seems so.  Basically, in the case of a large number of duplicates,
> we hit the maximum number of overflow pages.  There is a theoretical
> possibility of hitting it but it could also happen that we are not
> free the existing unused overflow pages due to which it keeps on
> growing and hit the limit.  I have requested up thread to verify if
> that is happening in this case and I am still waiting for same.  The
> squeeze operation does free such unused overflow pages after cleaning
> them.  As this is a costly operation and needs a cleanup lock, so we
> currently perform it only during Vacuum and next split from the bucket
> which can have redundant overflow pages.

Oops.  It was rather short-sighted of us not to increase
HASH_MAX_BITMAPS when we bumped HASH_VERSION.  Actually removing that
limit is hard, but we could have easily bumped it for 128 to say 1024
without (I think) causing any problem, which would have given us quite
a bit of headroom here.  I suppose we could still try to jam that
change in before beta3 (bumping HASH_VERSION again) but that might be
asking for trouble.

-- 
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] Confusing error message in pgbench

[Fabien COELHO]

Hello Tatsuo-san,


I found an error message in pgbench is quite confusing.

pgbench -S -M extended -c 1 -T 30 test
query mode (-M) should be specified before any transaction scripts (-f or -b)

Since there's no -f or -b option is specified, users will be
confused.


Indeed.

Actually the error occurs because pgbench implicitly introduces a built 
in script for -S. To eliminate the confusion, I think the error message 
should be fixed like this:


The idea is that -S/-N documentations say that it is just a shortcut for 
-b, but the explanation (eg --help) is too far away.


query mode (-M) should be specified before transaction type (-S or -N) 
or any transaction scripts (-f or -b)


I would suggest to make it even shorter, see attached:

query mode (-M) should be specified before any transaction scripts (-f, 
-b, -S or -N).


I'm wondering whether it could/should be "any transaction script". My 
English level does not allow to decide.


--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4d364a1..9e41c07 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3898,7 +3898,7 @@ main(int argc, char **argv)
 benchmarking_option_set = true;
 if (num_scripts > 0)
 {
-	fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f or -b)\n");
+	fprintf(stderr, "query mode (-M) should be specified before any transaction scripts (-f, -b, -S or -N)\n");
 	exit(1);
 }
 for (querymode = 0; querymode < NUM_QUERYMODE; querymode++)

-- 
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] Macros bundling RELKIND_* conditions

[Robert Haas]

 On Mon, Jul 3, 2017 at 3:52 AM, Ashutosh Bapat
 wrote:
>> I noticed, that
>> after we introduced RELKIND_PARTITIONED_TABLE, we required to change a
>> number of conditions to include this relkind. We missed some places in
>> initial commits and fixed those later. I am wondering whether we
>> should creates macros clubbing relevant relkinds together based on the
>> purpose of the tests e.g. IS_RELKIND_HAS_STORAGE(). When a new relkind
>> is added, one can examine these macros to check whether the new
>> relkind fits in the given macro. If all those macros are placed
>> together, there is a high chance that we will not miss any place in
>> the initial commit itself.

I've thought about this kind of thing, too.  But the thing is that
most of these macros you're proposing to introduce only get used in
one place.

0001-RELKIND_HAS_VISIBILITY_MAP.patch - one place
0002-RELKIND_HAS_STORAGE.patch - one place
0003-RELKIND_HAS_XIDS-macro.patch - one place
0004-RELKIND_HAS_COMPOSITE_TYPE-macro.patch - one place
0005-RELKIND_CAN_HAVE_TOAST_TABLE-macro.patch - one place
0006-RELKIND_CAN_HAVE_COLUMN_COMMENT-macro.patch - one place
0007-RELKIND_CAN_HAVE_INDEX-macro.patch - two places
0008-RELKIND_CAN_HAVE_COLUMN_SECLABEL-macro.patch - one place
0009-RELKIND_CAN_HAVE_STATS-macro.patch - two places

I'm totally cool with doing this where we can use the macro in more
than one place, but otherwise I don't think it helps.

> With this approach the macro which tests relkinds and the macro which
> reports error are places together in pg_class.h. If somebody adds a
> new relkind, s/he will notice the comment there and update the macros
> below also keeping the error message in sync with the test. Please
> note that partitioned tables are not explicitly mentioned in the error
> messages when the corresponding test has RELKIND_PARTITIONED_TABLE. I
> think we don't need to differentiate between a regular table and
> partitioned table in those error messages; a "table" implies both a
> regular table and a partitioned table.

I'm honestly not sure this buys us anything, unless you can use those
macros in a lot more places.

> With this approach, if a developer may still fail to update the error
> message when the test is updated. We can further tighten this by
> following approach.
> 1. For every test declare an array of relkinds that the test accepts e.g.
> int relkinds_with_vm[] = {RELKIND_RELATION, RELKIND_MATVIEW,
> RELKIND_TOASTVALUE};
> 2. Write a function is_relkind_in_array(int *relkinds_array, int
> num_relkinds, int relkind) to check whether the given relkind is in
> the array.
> 3. Each test macro now calls this function passing appropriate array
> e.g. #define RELKIND_WITH_VISIBILITY_MAP(relkind) \
>  is_relkind_in_array(relkinds_with_vm,
> sizeof(relkinds_with_vm)/sizeof(relkinds_with_vm[0], (relkind))
> 4. Declare an array of relkinds and their readable strings e.g
> {{RELKIND_RELATION, "table"}, {RELKIND_MATVIEW, "materialized view"}}
> 5. Write a function to collect the readable strings for all relkinds a
> given array of relkinds say char *relkind_names(int *relkinds, int
> num_relkinds)
> 6. Declare error message macros to call this function by passing
> appropriate array.

I think this might cause some problems for translators.

-- 
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] [TRAP: FailedAssertion] causing server to crash

[Robert Haas]

On Fri, Jul 21, 2017 at 1:31 AM, Thomas Munro
 wrote:
> Thanks Neha.  It's be best to post the back trace and if possible
> print oldestXact and ShmemVariableCache->oldestXid from the stack
> frame for TruncateCLOG.
>
> The failing assertion in TruncateCLOG() has a comment that says
> "vac_truncate_clog already advanced oldestXid", but vac_truncate_clog
> calls SetTransactionIdLimit() to write ShmemVariableCache->oldestXid
> *after* it calls TruncateCLOG().  What am I missing here?

This problem was introduced by commit
ea42cc18c35381f639d45628d792e790ff39e271, so this should be added to
the PostgreSQL 10 open items list. That commit intended to introduce a
distinction between (1) the oldest XID that can be safely examined and
(2) the oldest XID that can't yet be safely reused.  These are the
same except when we're in the middle of truncating CLOG: (1) advances
before the truncation, and (2) advances afterwards. That's why
AdvanceOldestClogXid() happens before truncation proper and
SetTransactionIdLimit() happens afterwards, and changing the order
would, I think, be quite wrong.

AFAICS, that assertion is simply a holdover from an earlier version of
the patch that escaped review.  There's just no reason to suppose that
it's true.

> What actually prevents ShmemVariableCache->oldestXid from going
> backwards anyway?  Suppose there are two or more autovacuum processes
> that reach vac_truncate_clog() concurrently.  They do a scan of
> pg_database whose tuples they access without locking through a
> pointer-to-volatile because they expect concurrent in-place writers,
> come up with a value for frozenXID, and then arrive at
> SetTransactionIdLimit() in whatever order and clobber
> ShmemVariableCache->oldestXid.  What am I missing here?

Hmm, there could be a bug there, but I don't think it's *this* bug.

-- 
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] typo for using "OBJECT_TYPE" for "security label on domain" in "gram.y"

[Peter Eisentraut]

On 8/2/17 08:21, Robert Haas wrote:
> On Wed, Aug 2, 2017 at 6:04 AM, 高增琦  wrote:
>> Commit: 3f88672a4e4d8e648d24ccc65937da61c7660854 add "security label on
>> domain"
>> in "gram.y", and set "objtype" to "OBJECT_TYPE".
>>
>> Is this a typo?
> 
> Looks like it.

Fix committed to master.  I don't intend to backpatch it.

-- 
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] pgbench: Skipping the creating primary keys after initialization

[Robert Haas]

On Wed, Aug 2, 2017 at 10:17 AM, Tom Lane  wrote:
> Sure, but "no indexes at all" is hardly ever the real goal, is it?

Right.

> So the switch as proposed is only solving part of your problem.
> I'd rather see a solution that addresses a larger range of desires.

That's reasonable.

> Or in other words, this looks to me quite a bit like the hackery
> that resulted in pgbench's -S and -N options, before we figured out
> that making it scriptable was a better answer.

But it's not very clear to me how we could make this case scriptable,
and it would probably not be much different from just using the
proposed option and then running the script afterwards yourself via
psql.  The thing about -N and -S is that those scripts are being run
repeatedly, so pgbench has to be involved.  If you just want to create
different/extra indexes, you can do that yourself.

-- 
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] Unused variable scanned_tuples in LVRelStats

[Robert Haas]

On Tue, Jul 4, 2017 at 10:13 PM, Masahiko Sawada  wrote:
> scanned_tuples variable in LVRelStats is introduced by commit b4b6923e
> but it seems to me that it's actually not used. We store num_tuples
> into vacrelstats->scanned_tuples after scanned all blocks, and the
> comment mentioned that saving it in order to use later but we actually
> use num_tuples instead of vacrelstats->scanned_tuples from there. I
> think the since the name of scanned_tuples implies more clearer
> purpose than num_tuples it's better to use it instead of num_tuples,
> or we can remove scanned_tuples from LVRelStats.

I think we should only store stuff in LVRelStats if it needs to be
passed to some other function.  Data that's only used in
lazy_scan_heap() can just be kept in local variables.  We could rename
the local variable, though, since I agree with you that scanned_tuples
is clearer.

-- 
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] Domains and arrays and composites, oh my

[Robert Haas]

On Thu, Jul 13, 2017 at 3:42 PM, Tom Lane  wrote:
> Yeah, it does, although I'm not sure how intuitive it is that the
> parentheses are significant ...
>
> regression=# select fdc.* from fdc();
>   fdc
> ---
>  (1,2)
> (1 row)
>
> regression=# select (fdc).* from fdc();
>  r | i
> ---+---
>  1 | 2
> (1 row)

Not intuitive at all.

-- 
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] pgbench: Skipping the creating primary keys after initialization

[Tom Lane]

Robert Haas  writes:
> I've actually wanted this exact thing multiple times: most recently,
> to make a non-unique btree index instead of a unique one, and to make
> a hash index instead of a btree one.  I don't object to a modest
> effort at coming up with a more general mechanism here, but I also
> think the switch as proposed is something that would have met my real
> needs on multiple occasions.  I've probably had 10 different occasions
> when I wanted all of the standard pgbench initialization *except for*
> something different about the indexes.

Sure, but "no indexes at all" is hardly ever the real goal, is it?
So the switch as proposed is only solving part of your problem.
I'd rather see a solution that addresses a larger range of desires.

Or in other words, this looks to me quite a bit like the hackery
that resulted in pgbench's -S and -N options, before we figured out
that making it scriptable was a better answer.

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] reload-through-the-top-parent switch the partition table

[Robert Haas]

On Wed, Aug 2, 2017 at 1:01 AM, Rushabh Lathia  wrote:
> Looking at the dbObjectTypePriority comments that seems like data
> restoration
> will *absolutely always* follow all CREATE TABLE commands.

Hmm.  I wasn't very convinced by those comments, but Tom's commit
a1ef01fe163b304760088e3e30eb22036910a495 convinces me that it has to
work that way.  So I think we are OK on that score.

The patch itself looks just fine on a quick glance, modulo the lack of
documentation, but I think we need to bikeshed the name of the flag.
--reload-through-root is clear as daylight to me, but I'm not sure
users will agree.   The lack of the word "partition" is perhaps a
significant flaw, and pg_dump doesn't really reload anything; it just
dumps.

The best thing I can come up with after brief thought is
--partition-data-via-root, but maybe somebody else has a better idea?

-- 
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] pgbench: Skipping the creating primary keys after initialization

[Robert Haas]

On Wed, Aug 2, 2017 at 9:41 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Aug 1, 2017 at 9:49 PM, Masahiko Sawada  
>> wrote:
>>> I'd like to propose a new option -I for pgbench command which skips
>>> the creating primary keys after initialized tables.
>
>> I support adding an option for this, but I propose that we just make
>> it a long-form option, similar to --log-prefix or --index-tablespace.
>
> I think we could probably do without this ... if you want a non-default
> test setup, why do you need to use "pgbench -i" to create it?
>
> It's not that there's anything greatly wrong with this particular idea,
> it's just that pgbench has too many switches already, and omitting random
> subsets of the initialization actions doesn't seem like it contributes
> fundamental new benchmarking capability.
>
> I could get behind a proposal that generalized pgbench's "-i" behavior
> in some meaningful way.  I wonder whether it would be possible to convert
> that behavior into a script.  Some of what it does is just SQL commands
> with injected parameters, which pgbench does already.  There's also
> data-loading actions, which could be converted to backslash commands
> perhaps.  Then desires like this could be addressed by invoking a
> customized script instead of complicating pgbench's option set.

I've actually wanted this exact thing multiple times: most recently,
to make a non-unique btree index instead of a unique one, and to make
a hash index instead of a btree one.  I don't object to a modest
effort at coming up with a more general mechanism here, but I also
think the switch as proposed is something that would have met my real
needs on multiple occasions.  I've probably had 10 different occasions
when I wanted all of the standard pgbench initialization *except for*
something different about the indexes.

-- 
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] pgbench: Skipping the creating primary keys after initialization

[Tatsuo Ishii]

> I think we could probably do without this ... if you want a non-default
> test setup, why do you need to use "pgbench -i" to create it?
> 
> It's not that there's anything greatly wrong with this particular idea,
> it's just that pgbench has too many switches already, and omitting random
> subsets of the initialization actions doesn't seem like it contributes
> fundamental new benchmarking capability.
> 
> I could get behind a proposal that generalized pgbench's "-i" behavior
> in some meaningful way.  I wonder whether it would be possible to convert
> that behavior into a script.  Some of what it does is just SQL commands
> with injected parameters, which pgbench does already.  There's also
> data-loading actions, which could be converted to backslash commands
> perhaps.  Then desires like this could be addressed by invoking a
> customized script instead of complicating pgbench's option set.

+1.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] pgbench: Skipping the creating primary keys after initialization

[Tom Lane]

Robert Haas  writes:
> On Tue, Aug 1, 2017 at 9:49 PM, Masahiko Sawada  wrote:
>> I'd like to propose a new option -I for pgbench command which skips
>> the creating primary keys after initialized tables.

> I support adding an option for this, but I propose that we just make
> it a long-form option, similar to --log-prefix or --index-tablespace.

I think we could probably do without this ... if you want a non-default
test setup, why do you need to use "pgbench -i" to create it?

It's not that there's anything greatly wrong with this particular idea,
it's just that pgbench has too many switches already, and omitting random
subsets of the initialization actions doesn't seem like it contributes
fundamental new benchmarking capability.

I could get behind a proposal that generalized pgbench's "-i" behavior
in some meaningful way.  I wonder whether it would be possible to convert
that behavior into a script.  Some of what it does is just SQL commands
with injected parameters, which pgbench does already.  There's also
data-loading actions, which could be converted to backslash commands
perhaps.  Then desires like this could be addressed by invoking a
customized script instead of complicating pgbench's option set.

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] pgbench: Skipping the creating primary keys after initialization

[Masahiko Sawada]

On Wed, Aug 2, 2017 at 10:25 PM, Robert Haas  wrote:
> On Tue, Aug 1, 2017 at 9:49 PM, Masahiko Sawada  wrote:
>> I'd like to propose a new option -I for pgbench command which skips
>> the creating primary keys after initialized tables. This option is
>> useful for users who want to do bench marking with no index or indexes
>> other than btree primary index. If we initialize pgbench tables at a
>> large number scale factor the primary key index creation takes a long
>> time even if we're going to use other types of indexes. With this
>> option, the initialization time is reduced and you can create indexes
>> as you want.
>>
>> Feedback is very welcome. I'll add this patch to the next CF.
>
> I support adding an option for this, but I propose that we just make
> it a long-form option, similar to --log-prefix or --index-tablespace.
>

Yeah, that's better. I'll update the patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
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] pgbench: Skipping the creating primary keys after initialization

[Robert Haas]

On Tue, Aug 1, 2017 at 9:49 PM, Masahiko Sawada  wrote:
> I'd like to propose a new option -I for pgbench command which skips
> the creating primary keys after initialized tables. This option is
> useful for users who want to do bench marking with no index or indexes
> other than btree primary index. If we initialize pgbench tables at a
> large number scale factor the primary key index creation takes a long
> time even if we're going to use other types of indexes. With this
> option, the initialization time is reduced and you can create indexes
> as you want.
>
> Feedback is very welcome. I'll add this patch to the next CF.

I support adding an option for this, but I propose that we just make
it a long-form option, similar to --log-prefix or --index-tablespace.

-- 
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] Confusing error message in pgbench

[Robert Haas]

On Tue, Aug 1, 2017 at 10:03 PM, Tatsuo Ishii  wrote:
> I found an error message in pgbench is quite confusing.
>
> pgbench -S -M extended -c 1 -T 30 test
> query mode (-M) should be specified before any transaction scripts (-f or -b)
>
> Since there's no -f or -b option is specified, users will be
> confused. Actually the error occurs because pgbench implicitly
> introduces a built in script for -S. To eliminate the confusion, I
> think the error message should be fixed like this:
>
> query mode (-M) should be specified before transaction type (-S or -N) or any 
> transaction scripts (-f or -b)
>
> Patch attached.

Not really objecting, but an even better fix might be to remove the
restriction on the order in which the options can be specified.

-- 
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] typo for using "OBJECT_TYPE" for "security label on domain" in "gram.y"

[Robert Haas]

On Wed, Aug 2, 2017 at 6:04 AM, 高增琦  wrote:
> Commit: 3f88672a4e4d8e648d24ccc65937da61c7660854 add "security label on
> domain"
> in "gram.y", and set "objtype" to "OBJECT_TYPE".
>
> Is this a typo?

Looks like it.

-- 
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] Red-Black tree traversal tests

[Victor Drobny]

I forgot to attach the patch. Sorry.
Here it is.
--
--
Victor Drobny
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 3ce9904..b7ed0af 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -13,6 +13,7 @@ SUBDIRS = \
 		  test_extensions \
 		  test_parser \
 		  test_pg_dump \
+		  test_rbtree \
 		  test_rls_hooks \
 		  test_shm_mq \
 		  worker_spi
diff --git a/src/test/modules/test_rbtree/.gitignore b/src/test/modules/test_rbtree/.gitignore
new file mode 100644
index 000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/test_rbtree/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_rbtree/Makefile b/src/test/modules/test_rbtree/Makefile
new file mode 100644
index 000..11a10cf
--- /dev/null
+++ b/src/test/modules/test_rbtree/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_rbtree/Makefile
+
+MODULE_big = test_rbtree
+OBJS = test.o $(WIN32RES)
+PGFILEDESC = "test_rbtree - rbtree triversal testing"
+
+EXTENSION = test_rbtree
+DATA = test_rbtree--1.0.sql
+
+REGRESS = test_rbtree
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_rbtree
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_rbtree/README b/src/test/modules/test_rbtree/README
new file mode 100644
index 000..8f4287e
--- /dev/null
+++ b/src/test/modules/test_rbtree/README
@@ -0,0 +1,20 @@
+test_rbtree is a module tests for checking the correctness of all kinds of
+traversal of red-black tree. Right now rbtree in postgres has 4 kinds of
+traversals: Left-Current-Right, Right-Current-Left, Current-Left-Right and
+Left-Right-Current.
+
+This extention has 4 functions. Each function checks one traversal.
+The checking the correctness of first two types are based on the fact that
+red-black tree is a binary search tree, so the elements should be iterated in
+increasing(for Left-Current-Right) or decreasing(for Right-Current-Left)
+order.
+In order to verify last two strategies, we will check the sequence if it is
+correct or not. For given pre- or post- order traversing of binary search tree
+it is always possible to say is it correct or not and to rebuild original tree.
+The idea is based on the fact that in such traversal sequence is always
+possible to determine current node, left subtree and right subtree.
+
+Also, this module is checking the correctness of the find, delete and leftmost
+operation.
+
+These tests are performed on red-black trees that store integers.
\ No newline at end of file
diff --git a/src/test/modules/test_rbtree/expected/test_rbtree.out b/src/test/modules/test_rbtree/expected/test_rbtree.out
new file mode 100644
index 000..cd4435b
--- /dev/null
+++ b/src/test/modules/test_rbtree/expected/test_rbtree.out
@@ -0,0 +1,43 @@
+CREATE EXTENSION test_rbtree;
+SELECT testleftright();
+ testleftright 
+---
+ 
+(1 row)
+
+SELECT testrightleft();
+ testrightleft 
+---
+ 
+(1 row)
+
+SELECT testdirect();
+ testdirect 
+
+ 
+(1 row)
+
+SELECT testinverted();
+ testinverted 
+--
+ 
+(1 row)
+
+SELECT testfind();
+ testfind 
+--
+ 
+(1 row)
+
+SELECT testleftmost();
+ testleftmost 
+--
+ 
+(1 row)
+
+SELECT testdelete();
+ testdelete 
+
+ 
+(1 row)
+
diff --git a/src/test/modules/test_rbtree/int_rbtree.h b/src/test/modules/test_rbtree/int_rbtree.h
new file mode 100644
index 000..d153616
--- /dev/null
+++ b/src/test/modules/test_rbtree/int_rbtree.h
@@ -0,0 +1,49 @@
+/*--
+ *
+ * int_rbtree.h
+ *		Definitions for integer red-black tree
+ *
+ * Copyright (c) 2013-2017, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_rbtree/int_rbtree.h
+ *
+ * -
+ */
+
+#ifndef INT_RBTREE_H
+#define INT_RBTREE_H
+
+#include "lib/rbtree.h"
+
+typedef struct IntRBTreeNode
+{
+	RBNode		rbnode;
+	int			key;
+
+} IntRBTreeNode;
+
+static int
+cmp(const RBNode *a, const RBNode *b, void *arg)
+{
+	const IntRBTreeNode *ea = (const IntRBTreeNode *) a;
+	const IntRBTreeNode *eb = (const IntRBTreeNode *) b;
+
+	return ea->key - eb->key;
+}
+
+static RBNode *
+alloc(void *arg)
+{
+	IntRBTreeNode *ea;
+	ea = malloc(sizeof(IntRBTreeNode));
+	return (RBNode *) ea;
+}
+
+static void
+fr(RBNode * node, void *arg)
+{
+	free(node);
+}
+
+#endif // INT_RBTREE_H
diff --git a/src/test/modules/test_rbtree/sql/test_rbtree.sql b/src/test/modules/test_rbtree/sql/test_rbtree.sql
new file mode 100644
index 000..3bedff2
--- /dev/null
+++ b/src/test/modules/test_rbtree/sql/test_rbtree.sql
@@ -0,0 +1,9 @@
+CREATE EXTENSION 

Re: [HACKERS] Red-Black tree traversal tests

[Victor Drobny]

Hello,

Thank you for the reviewing.
If it's not too much trouble perhaps you could write a few more test 
so
we would have 100% test coverage for rbtree, could modify it safely 
and

be sure that it actually works when someone will need the rest of its
functionality?


Done. Now all of the functions in rbtree.c are covered.


Also I would recommend to add your patch to the nearest commitfest [1].
Otherwise there is a good chance that everyone will forget about it
quite soon.

[1]: https://commitfest.postgresql.org/14/


Done. Here is the link: https://commitfest.postgresql.org/14/1225/

Thank you for attention!

--
--
Victor Drobny
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Proposal for CSN based snapshots

[Alexander Kuzmenkov]

What problem exactly you are seeing in the clog, is it the contention
around CLOGControlLock or generally accessing CLOG is slower.  If
former, then we already have a patch [1] to address it.
It's the contention around CLogControlLock. Thank you for the pointer, 
next time I'll try it with the group clog update patch.



--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] foreign table creation and NOT VALID check constraints

[Robert Haas]

On Wed, Aug 2, 2017 at 3:46 AM, Ashutosh Bapat
 wrote:
> If the user has specified "not valid" for a constraint on the foreign
> table, there is high chance that s/he is aware of the fact that the
> remote table that the foreign table points to has some rows which will
> violet the constraint. So, +1.

+1 from me, too.

-- 
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] A bug in mapping attributes in ATExecAttachPartition()

[Robert Haas]

On Tue, Aug 1, 2017 at 9:44 PM, Amit Langote
 wrote:
> I too dislike the shape of attachRel.  How about we rename attachRel to
> attachrel?  So, attachrel_children, attachrel_constr, etc.  It's still
> long though... :)

OK, I can live with that, I guess.

-- 
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] UPDATE of partition key

[Amit Khandekar]

On 2 August 2017 at 14:38, Amit Langote  wrote:
> On 2017/07/29 2:45, Amit Khandekar wrote:
>> On 28 July 2017 at 20:10, Robert Haas  wrote:
>>> On Wed, Jul 26, 2017 at 2:13 AM, Amit Langote wrote:
 I checked that we get the same result relation order with both the
 patches, but I would like to highlight a notable difference here between
 the approaches taken by our patches.  In my patch, I have now taught
 RelationGetPartitionDispatchInfo() to lock *only* the partitioned tables
 in the tree, because we need to look at its partition descriptor to
 collect partition OIDs and bounds.  We can defer locking (and opening the
 relation descriptor of) leaf partitions to a point where planner has
 determined that the partition will be accessed after all (not pruned),
 which will be done in a separate patch of course.
>>>
>>> That's very desirable, but I believe it introduces a deadlock risk
>>> which Amit's patch avoids.  A transaction using the code you've
>>> written here is eventually going to lock all partitions, BUT it's
>>> going to move the partitioned ones to the front of the locking order
>>> vs. what find_all_inheritors would do.  So, when multi-level
>>> partitioning is in use, I think it could happen that some other
>>> transaction is accessing the table using a different code path that
>>> uses the find_all_inheritors order without modification.  If those
>>> locks conflict (e.g. query vs. DROP) then there's a deadlock risk.
>>
>> Yes, I agree. Even with single-level partitioning, the leaf partitions
>> ordered by find_all_inheritors() is by oid values, so that's also
>> going to be differently ordered.
>
> We do require to lock the parent first in any case.  Doesn't that prevent
> deadlocks by imparting an implicit order on locking by operations whose
> locks conflict.

Yes may be, but I am not too sure at this point. find_all_inheritors()
locks only the children, and the parent lock is already locked
separately. find_all_inheritors() does not necessitate to lock the
children with the same lockmode as the parent.

> Having said that, I think it would be desirable for all code paths to
> manipulate partitions in the same order.  For partitioned tables, I think
> we can make it the partition bound order by replacing all calls to
> find_all_inheritors and find_inheritance_children on partitioned table
> parents with something else that reads partition OIDs from the relcache
> (PartitionDesc) and traverses the partition tree breadth-first manner.
>
>>> Unfortunately I don't see any easy way around that problem, but maybe
>>> somebody else has an idea.
>>
>> One approach I had considered was to have find_inheritance_children()
>> itself lock the children in bound order, so that everyone will have
>> bound-ordered oids, but that would be too expensive since it requires
>> opening all partitioned tables to initialize partition descriptors. In
>> find_inheritance_children(), we get all oids without opening any
>> tables. But now that I think more of it, it's only the partitioned
>> tables that we have to open, not the leaf partitions; and furthermore,
>> I didn't see calls to find_inheritance_children() and
>> find_all_inheritors() in performance-critical code, except in
>> expand_inherited_rtentry(). All of them are in DDL commands; but yes,
>> that can change in the future.
>
> This approach more or less amounts to calling the new
> RelationGetPartitionDispatchInfo() (per my proposed patch, a version of
> which I posted upthread.)  Maybe we can add a wrapper on top, say,
> get_all_partition_oids() which throws away other things that
> RelationGetPartitionDispatchInfo() returned.  In addition it locks all the
> partitions that are returned, unlike only the partitioned ones, which is
> what RelationGetPartitionDispatchInfo() has been taught to do.

So there are three different task items here :
1. Arrange the oids in consistent order everywhere.
2. Prepare the Partition Dispatch Info data structure in the planner
as against during execution.
3. For update tuple routing, assume that the result rels are ordered
consistently to make the searching efficient.

#3 depends on #1. So for that, I have come up with a minimum set of
changes to have expand_inherited_rtentry() generate the rels in bound
order. When we do #2 , it may be possible that we may need to re-do my
changes in expand_inherited_rtentry(), but those are minimum. We may
even end up having the walker function being used at multiple places,
but right now it is not certain.

So, I think we can continue the discussion about #1 and #2 in a separate thread.

>
>> Regarding dynamically locking specific partitions as and when needed,
>> I think this method inherently has the issue of deadlock because the
>> order would be random. So it feels like there is no way around other
>> than to lock all partitions beforehand.
>
> I'm not sure why the order has to be random.  If 

Re: [HACKERS] POC: Cache data in GetSnapshotData()

[Mithun Cy]

On Wed, Aug 2, 2017 at 3:42 PM, Mithun Cy  wrote:
Sorry, there was an unnecessary header included in proc.c which should
be removed adding the corrected patch.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


Cache_data_in_GetSnapshotData_POC_04.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] POC: Cache data in GetSnapshotData()

[Mithun Cy]

I have made few more changes with the new patch.

1. Ran pgindent.
2. Instead of an atomic state variable to make only one process cache
the snapshot in shared memory, I have used conditional try lwlock.
With this, we have a small and reliable code.
3. Performance benchmarking

Machine - cthulhu
==
[mithun.cy@cthulhu bin]$ lscpu
Architecture:  x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):128
On-line CPU(s) list:   0-127
Thread(s) per core:2
Core(s) per socket:8
Socket(s): 8
NUMA node(s):  8
Vendor ID: GenuineIntel
CPU family:6
Model: 47
Model name:Intel(R) Xeon(R) CPU E7- 8830  @ 2.13GHz
Stepping:  2
CPU MHz:   1197.000
BogoMIPS:  4266.63
Virtualization:VT-x
L1d cache: 32K
L1i cache: 32K
L2 cache:  256K
L3 cache:  24576K
NUMA node0 CPU(s): 0,65-71,96-103
NUMA node1 CPU(s): 72-79,104-111
NUMA node2 CPU(s): 80-87,112-119
NUMA node3 CPU(s): 88-95,120-127
NUMA node4 CPU(s): 1-8,33-40
NUMA node5 CPU(s): 9-16,41-48
NUMA node6 CPU(s): 17-24,49-56
NUMA node7 CPU(s): 25-32,57-64

Server configuration:
./postgres -c shared_buffers=8GB -N 300 -c min_wal_size=15GB -c
max_wal_size=20GB -c checkpoint_timeout=900 -c
maintenance_work_mem=1GB -c checkpoint_completion_target=0.9 -c
wal_buffers=256MB &

pgbench configuration:
scale_factor = 300
./pgbench -c $threads -j $threads -T $time_for_reading -M prepared -S  postgres

The machine has 64 cores with this patch I can see server starts
improvement after 64 clients. I have tested up to 256 clients. Which
shows performance improvement nearly max 39%.

Alternatively, I thought instead of storing the snapshot in a shared
memory each backend can hold on to its previously computed snapshot
until next commit/rollback happens in the system. We can have a global
counter value associated with the snapshot when ever it is computed.
Upon any new end of the transaction, the global counter will be
incremented. So when a process wants a new snapshot it can compare
these 2 values to check if it can use previously computed snapshot.
This makes code significantly simple. With the first approach, one
process has to compute and store the snapshot for every end of the
transaction and others can reuse the cached the snapshot.  In the
second approach, every process has to re-compute the snapshot. So I am
keeping with the same approach.

On Mon, Jul 10, 2017 at 10:13 AM, Mithun Cy  wrote:
> On Fri, Apr 8, 2016 at 12:13 PM, Robert Haas  wrote:
>> I think that we really shouldn't do anything about this patch until
>> after the CLOG stuff is settled, which it isn't yet.  So I'm going to
>> mark this Returned with Feedback; let's reconsider it for 9.7.
>
> I am updating a rebased patch have tried to benchmark again could see
> good improvement in the pgbench read-only case at very high clients on
> our cthulhu (8 nodes, 128 hyper thread machines) and power2 (4 nodes,
> 192 hyper threads) machine. There is some issue with base code
> benchmarking which is somehow not consistent so once I could figure
> out what is the issue with that I will update
>
> --
> Thanks and Regards
> Mithun C Y
> EnterpriseDB: http://www.enterprisedb.com



-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


cache_the_snapshot_performance.ods
Description: application/vnd.oasis.opendocument.spreadsheet


Cache_data_in_GetSnapshotData_POC_03.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] Parallel Hash take II

[Thomas Munro]

On Tue, Aug 1, 2017 at 1:11 PM, Andres Freund  wrote:
> WRT the main patch:

Thanks for the review.  I will respond soon, but for now I just wanted
to post a rebased version (no changes) because v16 no longer applies.

-- 
Thomas Munro
http://www.enterprisedb.com


parallel-hash-v17.patchset.tgz
Description: GNU Zip compressed 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] INSERT ON CONFLICT and partitioned tables

[Jeevan Ladhe]

I applied the patch on latest master sources and the patch applies cleanly.
The documentation is built without errors.

We do not support following syntax for 'do nothing':

postgres=# insert into parted_conflict_test values (1, 'a') on conflict (b)
do nothing;
ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT
specification

This limitation is because we do not support unique index on partitioned
table.
But, in that sense the following snippet of the documentation seems
misleading:

+   will cause an error if the conflict target is specified (see
+for more details).  That means it's not
+   possible to specify DO UPDATE as the alternative
+   action, because it requires the conflict target to be specified.
+   On the other hand, specifying DO NOTHING as the
+   alternative action works fine.
May be the last sentence can be rephrased as below:

"On the other hand, specifying DO NOTHING without target
as
an alternative action works fine."

Other than this patch looks good to me.

Regards,
Jeevan Ladhe



On Wed, Aug 2, 2017 at 10:26 AM, Amit Langote  wrote:

> Starting a new thread for a patch I posted earlier [1] to handle ON
> CONFLICT DO NOTHING when inserting into a partitioned table.  It's
> intended for PG 11 and so registered in the upcoming CF.
>
> Summary of the previous discussion and the patch for anyone interested:
>
> Currently, if an INSERT statement for a partitioned table mentions the ON
> CONFLICT clause, we error out immediately.  It was implemented that way,
> because it was thought that it could not be handled with zero support for
> defining indexes on partitioned tables.  Peter Geoghegan pointed out [2]
> that it's too restrictive a view.
>
> He pointed out that planner doesn't *always* expect indexes to be present
> on the table when ON CONFLICT is specified.  They must be present though
> if DO UPDATE action is requested, because one would need to also specify
> the exact columns on which conflict will be checked and those must covered
> by the appropriate indexes.  So, if the table is partitioned and DO UPDATE
> is specified, lack of indexes will result in an error saying that a
> suitable index is absent.  DO UPDATE action cannot be supported until we
> implement the feature to define indexes on partitioned tables.
>
> OTOH, the DO NOTHING case should go through the planner without error,
> because neither any columns need to be specified nor any indexes need to
> be present covering them.  So, DO NOTHING on partitioned tables might work
> after all.  Conflict can only be determined using indexes, which
> partitioned tables don't allow, so how?  Leaf partitions into which tuples
> are ultimately stored can have indexes defined on them, which can be used
> to check for the conflict.
>
> The patch's job is simple:
>
> - Remove the check in the parser that causes an error the moment the
>   ON CONFLICT clause is found.
>
> - Fix leaf partition ResultRelInfo initialization code so that the call
>   ExecOpenIndices() specifies 'true' for speculative, so that the
>   information necessary for conflict checking will be initialized in the
>   leaf partition's ResultRelInfo
>
> Thanks,
> Amit
>
> [1]
> https://www.postgresql.org/message-id/62be3d7a-08f6-5dcb-
> f5c8-a5b764ca96df%40lab.ntt.co.jp
>
> [2]
> https://www.postgresql.org/message-id/CAH2-Wzm10T%2B_PWVM4XO5zaknVbAXkOH9-
> JW3gRVPm1njLHck_w%40mail.gmail.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

[HACKERS] typo for using "OBJECT_TYPE" for "security label on domain" in "gram.y"

[高增琦]

Commit: 3f88672a4e4d8e648d24ccc65937da61c7660854 add "security label on
domain"
in "gram.y", and set "objtype" to "OBJECT_TYPE".

Is this a typo?

-- 
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com

Re: [HACKERS] How to run PG TAP tests on windows?

[Abbas Butt]

On Tue, Aug 1, 2017 at 7:35 PM, Michael Paquier 
wrote:

> On Tue, Aug 1, 2017 at 10:24 AM, Abbas Butt 
> wrote:
> > Can anyone point out to a tutorial or a list of steps required to run PG
> TAP
> > tests on windows?
>
> Only MSVC has a special handling:
> https://www.postgresql.org/docs/devel/static/install-windows-full.html#
> idm46046082578368
> Using vcregress.bat, you are looking mainly for the subcommands
> bincheck and recoverycheck.
>

Thanks Michael.



> --
> Michael
>



-- 
-- 
*Abbas*
Architect

Ph: 92.334.5100153
Skype ID: gabbasb
www.enterprisedb.co m


*Follow us on Twitter*
@EnterpriseDB

Visit EnterpriseDB for tutorials, webinars, whitepapers
 and more