Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Fri, Aug 18, 2017 at 12:25 AM, Robert Haas  wrote:

> On Thu, Aug 17, 2017 at 6:24 AM, Jeevan Ladhe
>  wrote:
> > I have addressed following comments in V25 patch[1].
>
> Committed 0001.  Since that code seems to be changing about every 10
> minutes, it seems best to get this refactoring out of the way before
> it changes again.


Thanks Robert for taking care of this.

Regards,
Jeevan Ladhe

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Michael Paquier]

On Tue, Aug 15, 2017 at 4:05 PM, Michael Paquier
 wrote:
> On Tue, Aug 15, 2017 at 8:28 AM, Bossart, Nathan  wrote:
>> I’ve rebased this patch with master to create v7, which is attached.
>
> Thanks for the rebased patch. I am switching into review mode actively
> now, so I'll look at it soon.

Another pass for this patch.

+   /*
+* We have already checked the column list in vacuum(...),
+* but the columns may have disappeared since then.  If
+* this happens, emit a nice warning message and skip the
+* undefined column.
+*/
I think that this would be reworded. "nice" is cute is this context.
Why not just saying something like:
"Do not issue an ERROR if a column is missing but use a WARNING
instead. At the beginning of the VACUUM run, the code already checked
for undefined columns and informed about an ERROR, but we want the
processing to move on for existing columns."

+   /*
+* Now dedupe the list to avoid any redundant work (e.g. user specifies
+* the same relation twice).  We also take care of combining any
+* separate column lists for duplicate relations.
+*
+* We do this after resolving the OIDs so that we do not miss entries
+* that have unequal RangeVars but resolve to the same set of OIDs.
+* For example, "foo" and "public.foo" could be the same relation.
+*/
+   relations = dedupe_relations(relations);
This has been introduced in v5. If we would want to put some effort
for that, I think that it could be a separate patch and a separate
discussion. This patch does not make things worse than they are, see
HEAD for example with the same column specified twice:
=# create table aa as select generate_series(1, 1) as a;
SELECT 1
=# vacuum (analyze) aa (a, a);
ERROR:  23505: duplicate key value violates unique constraint
"pg_statistic_relid_att_inh_index"
DETAIL:  Key (starelid, staattnum, stainherit)=(16385, 1, f) already exists.
SCHEMA NAME:  pg_catalog
TABLE NAME:  pg_statistic
CONSTRAINT NAME:  pg_statistic_relid_att_inh_index
LOCATION:  _bt_check_unique, nbtinsert.c:434

And actually, your patch does not seem to work, and makes things worse:
=# analyze aa (a, a);
ERROR:  XX000: tuple already updated by self
LOCATION:  simple_heap_update, heapam.c:4482

+/*
+ * This is used to keep track of a relation and an optional list of
+ * column names, as may be specified in VACUUM and ANALYZE.
+ */
+typedef struct RelationAndColumns
+{
+   NodeTag  type;
+   RangeVar*relation;  /* single table to process */
+   List*va_cols;   /* list of column names, or NIL for all */
+   List*oids;  /* corresponding OIDs (filled in by
[auto]vacuum.c) */
+} RelationAndColumns;
This name is a bit awkward. Say VacuumRelation? I also don't
understand why there are multiple OIDs here. There should be only one,
referring to the relation of this RangeVar. Even for inherited
relations what should be done is to add one entry RelationAndColumns
(or VacuumRelation) for each child relation.

+   /*
+* Check that all specified columns exist so that we can fast-fail
+* commands with multiple tables.  If the column disappears before we
+* actually process it, we will emit a WARNING and skip it later on.
+*/
+   foreach(relation, relations)
+   check_columns_exist(lfirst(relation));
The full list could be processed in there.

+static void
+check_columns_exist(RelationAndColumns *relation)
[...]
+   Relation rel;
+   ListCell *lc;
+
+   rel = try_relation_open(lfirst_oid(oid), NoLock);
+   if (!rel)
This really meritates a comment. In short: why is it fine to not take
a lock here? The answer I think is that even if the relation does not
exist vacuum would do nothing, but this should be written out.

+   foreach(relation, relations)
+   {
+   if (((RelationAndColumns *) lfirst(relation))->va_cols != NIL &&
+   !(options & VACOPT_ANALYZE))
Saving the data in a variable makes for a better style and easier
debugging. When doing a bitwise operation, please use as well != 0 or
== 0 as you are looking here for a boolean result.

+   relinfo = makeNode(RelationAndColumns);
+   relinfo->oids = list_make1_oid(HeapTupleGetOid(tuple));
+   *vacrels = lappend(*vacrels, relinfo);
Assigning variables even for nothing is good practice for readability,
and shows the intention behind the code.

- * relid, if not InvalidOid, indicate the relation to process; otherwise,
- * the RangeVar is used.  (The latter must always be passed, because it's
- * used for error messages.)
+ * If we intend to process all relations, the 'relations' argument may be
+ * NIL.
This comment actually applies to RelationAndColumns. If the OID is
invalid, then RangeVar is used, and should always be set. You are
breaking that promise actually for autovacuum. The comment here should
say that if relations is NIL all the relations of the 

Re: [HACKERS] expanding inheritance in partition bound order

[Amit Langote]

On 2017/08/18 4:54, Robert Haas wrote:
> On Thu, Aug 17, 2017 at 8:39 AM, Ashutosh Bapat
>  wrote:
>> [2] had a patch with some changes to the original patch you posted. I
>> didn't describe those changes in my mail, since they rearranged the
>> comments. Those changes are not part of this patch and you haven't
>> comments about those changes as well. If you have intentionally
>> excluded those changes, it's fine. In case, you haven't reviewed them,
>> please see if they are good to be incorporated.
> 
> I took a quick look at your version but I think I like Amit's fine the
> way it is, so committed that and back-patched it to v10.

Thanks for committing.

> I find 0002 pretty ugly as things stand.  We get a bunch of tuple maps
> that we don't really need, only to turn around and free them.  We get
> a bunch of tuple slots that we don't need, only to turn around and
> drop them.  We don't really need the PartitionDispatch objects either,
> except for the OIDs they contain.  There's a lot of extra stuff being
> computed here that is really irrelevant for this purpose.  I think we
> should try to clean that up somehow.

One way to do that might be to reverse the order of the remaining patches
and put the patch to refactor RelationGetPartitionDispatchInfo() first.
With that refactoring, PartitionDispatch itself has become much simpler in
that it does not contain a relcache reference to be closed eventually by
the caller, the tuple map, and the tuple table slot.  Since those things
are required for tuple-routing, the refactoring makes
ExecSetupPartitionTupleRouting itself create them from the (minimal)
information returned by RelationGetPartitionDispatchInfo and ultimately
destroy when done using it.  I kept the indexes field in
PartitionDispatchData though, because it's essentially free to create
while we are walking the partition tree in
RelationGetPartitionDispatchInfo() and it seems undesirable to make the
caller compute that information (indexes) by traversing the partition tree
all over again, if it doesn't otherwise have to.  I am still considering
some counter-arguments raised by Amit Khandekar about this last assertion.

Thoughts?

Thanks,
Amit
From b86fd0e920e4bc49b17a2dba9e848420ec99c22b Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Jul 2017 18:59:57 +0900
Subject: [PATCH 1/2] Decouple RelationGetPartitionDispatchInfo() from executor

Currently it and the structure it generates viz. PartitionDispatch
objects are too coupled with the executor's tuple-routing code.  In
particular, it's pretty undesirable that it makes it the responsibility
of the caller to release some resources, such as relcache references
and tuple table slots.  That makes it harder to use in places other
than where it's currently being used.

After this refactoring, ExecSetupPartitionTupleRouting() now needs to
do some of the work that was previously done in
RelationGetPartitionDispatchInfo() and expand_inherited_rtentry() no
longer needs to do some things that it used to.
---
 src/backend/catalog/partition.c| 309 +
 src/backend/commands/copy.c|  35 ++--
 src/backend/executor/execMain.c| 145 ++--
 src/backend/executor/nodeModifyTable.c |  29 ++--
 src/include/catalog/partition.h|  52 +++---
 src/include/executor/executor.h|   4 +-
 src/include/nodes/execnodes.h  |  53 +-
 7 files changed, 391 insertions(+), 236 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 96a64ce6b2..7618e4cb31 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -105,6 +105,24 @@ typedef struct PartitionRangeBound
boollower;  /* this is the lower (vs upper) 
bound */
 } PartitionRangeBound;
 
+/*---
+ * PartitionDispatchData - information of partitions of one partitioned table
+ *in a partition tree
+ *
+ * partkey Partition key of the table
+ * partdescPartition descriptor of the table
+ * indexes Array with partdesc->nparts members (for details on 
what the
+ * individual value represents, see the comments in
+ * RelationGetPartitionDispatchInfo())
+ *---
+ */
+typedef struct PartitionDispatchData
+{
+   PartitionKeypartkey;/* Points into the table's relcache 
entry */
+   PartitionDesc   partdesc;   /* Ditto */
+   int*indexes;
+} PartitionDispatchData;
+
 static int32 qsort_partition_list_value_cmp(const void *a, const void *b,
   void *arg);
 static int32 qsort_partition_rbound_cmp(const void *a, const void *b,
@@ -981,181 +999,165 @@ get_partition_qual_relid(Oid relid)
 }
 
 /*
- * Append OIDs 

Re: [HACKERS] expanding inheritance in partition bound order

[Ashutosh Bapat]

On Fri, Aug 18, 2017 at 10:32 AM, Amit Khandekar  wrote:
>
> I think in the final changes after applying all 3 patches, the
> redundant tuple slot is no longer present. But ...
>> We don't really need the PartitionDispatch objects either,
>> except for the OIDs they contain.  There's a lot of extra stuff being
>> computed here that is really irrelevant for this purpose.  I think we
>> should try to clean that up somehow.
> ... I am of the same opinion. That's why - as I mentioned upthread - I
> was thinking why not have a separate light-weight function to just
> generate oids, and keep RelationGetPartitionDispatchInfo() intact, to
> be used only for tuple routing.
>
> But, I haven't yet checked Ashuthosh's requirements, which suggest
> that it does not help to even get the oid list.
>

0004 patch in partition-wise join patchset has code to expand
partition hierarchy. That patch is expanding inheritance hierarchy in
depth first manner. Robert commented that instead of depth first
manner, it will be better if we expand it in partitioned tables first
manner. With the latest changes in your patch-set I don't see the
reason for expanding in partitioned tables first order. Can you please
elaborate if we still need to expand in partitioned table first
manner? May be we should just address the expansion issue in 0004
instead of dividing it in two patches.

-- 
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

[HACKERS] Re: Stats for triggers on partitioned tables not shown in EXPLAIN ANALYZE

[Noah Misch]

On Wed, Aug 16, 2017 at 05:14:25PM +0900, Amit Langote wrote:
> On 2017/08/15 21:20, Etsuro Fujita wrote:
> > I noticed that runtime stats for BEFORE ROW INSERT triggers on leaf
> > partitions of partitioned tables aren't reported in EXPLAIN ANALYZE. Here
> > is an example:

> > So here is a
> > patch for fixing both issues.

> Adding this to the PG 10 open items list.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
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.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] Subscription code improvements

[Masahiko Sawada]

On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada  wrote:
> On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut
>  wrote:
>> On 8/8/17 05:58, Masahiko Sawada wrote:
>>> Are you planning to work on remaining patches 0005 and 0006 that
>>> improve the subscription codes in PG11 cycle? If not, I will take over
>>> them and work on the next CF.
>>
>> Based on your assessment, the remaining patches were not required bug
>> fixes.  So I think preparing them for the next commit fest would be great.
>>
>
> Thank you for the comment.
>
> After more thought, since 0001 and 0003 patches on the first mail also
> improve the subscription codes and are worth to be considered, I
> picked total 4 patches up and updated them. I'm planning to work on
> these patches in the next CF.
>

Added this item to the next commit fest.

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] expanding inheritance in partition bound order

[Amit Khandekar]

On 18 August 2017 at 01:24, Robert Haas  wrote:
> On Thu, Aug 17, 2017 at 8:39 AM, Ashutosh Bapat
>  wrote:
>> [2] had a patch with some changes to the original patch you posted. I
>> didn't describe those changes in my mail, since they rearranged the
>> comments. Those changes are not part of this patch and you haven't
>> comments about those changes as well. If you have intentionally
>> excluded those changes, it's fine. In case, you haven't reviewed them,
>> please see if they are good to be incorporated.
>
> I took a quick look at your version but I think I like Amit's fine the
> way it is, so committed that and back-patched it to v10.
>
> I find 0002 pretty ugly as things stand.  We get a bunch of tuple maps
> that we don't really need, only to turn around and free them.  We get
> a bunch of tuple slots that we don't need, only to turn around and
> drop them.

I think in the final changes after applying all 3 patches, the
redundant tuple slot is no longer present. But ...
> We don't really need the PartitionDispatch objects either,
> except for the OIDs they contain.  There's a lot of extra stuff being
> computed here that is really irrelevant for this purpose.  I think we
> should try to clean that up somehow.
... I am of the same opinion. That's why - as I mentioned upthread - I
was thinking why not have a separate light-weight function to just
generate oids, and keep RelationGetPartitionDispatchInfo() intact, to
be used only for tuple routing.

But, I haven't yet checked Ashuthosh's requirements, which suggest
that it does not help to even get the oid list.

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



-- 
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] expanding inheritance in partition bound order

[Ashutosh Bapat]

On Fri, Aug 18, 2017 at 10:12 AM, Amit Langote
 wrote
>
>>> 0002: Teach expand_inherited_rtentry to use partition bound order
>>
>> 0004 in [1] expands a multi-level partition hierarchy into similar
>> inheritance hierarchy. That patch doesn't need all OIDs in one go. It
>> will have to handle the partition hierarchy level by level, so most of
>> the code added by this patch will need to be changed by that patch. Is
>> there a way we can somehow change this patch so that the delta in 0004
>> is reduced? That may need rethinking about using
>> RelationGetPartitionDispatchInfo().
>
> Regarding that, I have a question:
>
> Does the multi-level partition-wise join planning logic depend on the
> inheritance itself to be expanded in a multi-level aware manner.  That is,
> expanding the partitioned table inheritance in multi-level aware manner in
> expan_inherited_rtentry()?

Yes, it needs AppendRelInfos to retain the parent-child relationship.
Please refer [1], [2], [3] for details.

>
> Wouldn't it suffice to just have the resulting Append paths be nested per
> multi-level partitioning hierarchy?

We are joining RelOptInfos, so those need to be nested. For those to
be nested, we need AppendRelInfos to preserve parent-child
relationship. Nesting paths doesn't help. Append paths actually should
be flattened out to avoid any extra time consumed in nested Append
node.


[1] 
https://www.postgresql.org/message-id/cafjfprcemmx26653xfayvc5kvqcrzckscvfqzdbxv%3dkb8ak...@mail.gmail.com
[2] 
https://www.postgresql.org/message-id/CAFjFpRefs5ZMnxQ2vP9v5zOtWtNPuiMYc01sb1SWjCOB1CT=u...@mail.gmail.com
[3] 
https://www.postgresql.org/message-id/CAFjFpRd6Kzx6Xn%3D7vdwwnh6rEw2VEgo--iPdhV%2BFb7bHfPzsbw%40mail.gmail.com

-- 
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] expanding inheritance in partition bound order

[Amit Langote]

Hi Ashutosh,

On 2017/08/17 21:39, Ashutosh Bapat wrote:
> On Thu, Aug 17, 2017 at 12:59 PM, Amit Langote
>  wrote:
>>
>> Attached rest of the patches.  0001 has changes per Ashutosh's review
>> comments [2]:
>>
>> 0001: Relieve RelationGetPartitionDispatchInfo() of doing any locking
> 
> [2] had a patch with some changes to the original patch you posted. I
> didn't describe those changes in my mail, since they rearranged the
> comments. Those changes are not part of this patch and you haven't
> comments about those changes as well. If you have intentionally
> excluded those changes, it's fine. In case, you haven't reviewed them,
> please see if they are good to be incorporated.

Sorry, I thought the ones you mentioned in the email were the only changes
you made to the original patch.  I noted only those and included them when
editing the relevant commit in my local repository in an interactive
rebase session.  I didn't actually take your patch and try to merge it
with the commit in my local repository.  IMHO, simply commenting in the
email which parts of the patch you would like to see changed would be
helpful. Then we can discuss those changes and proceed with them (or not)
per the result of that discussion.

>> 0002: Teach expand_inherited_rtentry to use partition bound order
> 
> 0004 in [1] expands a multi-level partition hierarchy into similar
> inheritance hierarchy. That patch doesn't need all OIDs in one go. It
> will have to handle the partition hierarchy level by level, so most of
> the code added by this patch will need to be changed by that patch. Is
> there a way we can somehow change this patch so that the delta in 0004
> is reduced? That may need rethinking about using
> RelationGetPartitionDispatchInfo().

Regarding that, I have a question:

Does the multi-level partition-wise join planning logic depend on the
inheritance itself to be expanded in a multi-level aware manner.  That is,
expanding the partitioned table inheritance in multi-level aware manner in
expan_inherited_rtentry()?

Wouldn't it suffice to just have the resulting Append paths be nested per
multi-level partitioning hierarchy?  Creating such nested Append paths
doesn't necessarily require that the inheritance be expanded that way in
the first place (as I am finding out when working on another patch).

Thanks,
Amit



-- 
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: Improve bitmap costing for lossy pages

[Robert Haas]

On Thu, Aug 17, 2017 at 12:06 AM, Dilip Kumar  wrote:
> I have attempted a very simple POC with this approach just to see how
> many lossy pages we can save if we lossify all the pages falling in
> the same chunk first, before moving to the next page.  I have taken
> some data on TPCH scale 20 with different work_mem (only calculated
> lossy pages did not test performance).  I did not see a significant
> reduction in lossy pages.  (POC patch attached with the mail in case
> someone is interested to test or more experiment).

That's not an impressive savings.  Maybe this approach is a dud, and
we should go back to just tackling the planner end of 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] Hash Functions

[Robert Haas]

On Wed, Aug 16, 2017 at 5:34 PM, Robert Haas  wrote:
> Attached is a quick sketch of how this could perhaps be done (ignoring
> for the moment the relatively-boring opclass pushups).

Here it is with some relatively-boring opclass pushups added.  I just
did the int4 bit; the same thing will need to be done, uh, 35 more
times.  But you get the gist.  No, not that kind of gist.

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


add-optional-second-hash-proc-v1.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] Re: ICU collation variant keywords and pg_collation entries (Was: [BUGS] Crash report for some ICU-52 (debian8) COLLATE and work_mem values)

[Noah Misch]

On Thu, Aug 17, 2017 at 09:22:07PM -0400, Peter Eisentraut wrote:
> On 8/14/17 12:23, Peter Eisentraut wrote:
> > On 8/13/17 15:39, Noah Misch 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:
> >> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> > 
> > I think there are up to three separate issues in play:
> > 
> > - what to do about some preloaded collations disappearing between versions
> > 
> > - whether to preload keyword variants
> > 
> > - whether to canonicalize some things during CREATE COLLATION
> > 
> > I responded to all these subplots now, but the discussion is ongoing.  I
> > will set the next check-in to Thursday.
> 
> I haven't read anything since that has provided any more clarity about
> what needs changing here.  I will entertain concrete proposals about the
> specific points above (considering any other issues under discussion to
> be PG11 material), but in the absence of that, I don't plan any work on
> this right now.

I think you're contending that, as formulated, this is not a valid v10 open
item.  Are you?


-- 
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] postgres_fdw bug in 9.6

[Etsuro Fujita]

On 2017/08/18 8:16, Michael Paquier wrote:

On Fri, Aug 18, 2017 at 3:59 AM, Tom Lane  wrote:

Robert Haas  writes:

On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita
 wrote:

I rebased the patch to HEAD.  PFA a new version of the patch.



Tom, you were instrumental in identifying what was going wrong here
initially.  Any chance you'd be willing to have a look at the patch?


I will, but probably not for a week or so.  Going eclipse-chasing.


Good luck:
https://xkcd.com/1876/
And enjoy:
https://xkcd.com/1877/


Enjoy!

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] [PATCH] Push limit to sort through a subquery

[Robert Haas]

On Thu, Aug 17, 2017 at 11:36 AM, Douglas Doole  wrote:

> I completely agree. The further a limit can be pushed down, the better.
>
> The patch looks good to me.
>

It seems like a somewhat ad-hoc approach; it supposes that we can take any
query produced by deparseSelectStmtForRel() and stick a LIMIT clause onto
the very end and all will be well.  Maybe that's not a problematic
assumption, not sure.  The grammar happens to allow both FOR UPDATE LIMIT n
and LIMIT n FOR UPDATE even though only the latter syntax is documented.

Regarding the other patch on this thread, you mentioned upthread that "If
it is possible to get more than one SubqueryScanState and/or ResultState
between the limit and sort, then the first block of code could be placed in
a while loop."  I think that's not possible for a ResultState, but I think
it *is* possible for a SubqueryScanState.

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

Re: [HACKERS] Re: ICU collation variant keywords and pg_collation entries (Was: [BUGS] Crash report for some ICU-52 (debian8) COLLATE and work_mem values)

[Peter Eisentraut]

On 8/14/17 12:23, Peter Eisentraut wrote:
> On 8/13/17 15:39, Noah Misch 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:
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 
> I think there are up to three separate issues in play:
> 
> - what to do about some preloaded collations disappearing between versions
> 
> - whether to preload keyword variants
> 
> - whether to canonicalize some things during CREATE COLLATION
> 
> I responded to all these subplots now, but the discussion is ongoing.  I
> will set the next check-in to Thursday.

I haven't read anything since that has provided any more clarity about
what needs changing here.  I will entertain concrete proposals about the
specific points above (considering any other issues under discussion to
be PG11 material), but in the absence of that, I don't plan any work on
this right now.

-- 
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] Inadequate infrastructure for NextValueExpr

[Andrew Gierth]

> "Thomas" == Thomas Munro  writes:

 >> [...]
 >> T_NamedTuplestoreScan can be produced by outfuncs.c with tagname
 >> NAMEDTUPLESTORESCAN but that tagname is not recognized by readfuncs.c
 >> [...]
 >> 
 >> That revealed a defect in commit
 >> 18ce3a4ab22d2984f8540ab480979c851dae5338 which I think should be
 >> corrected with something like the attached, though I'm not sure if
 >> it's possible to reach it.

 Thomas> Adding Kevin and Andrew G.  Thoughts on whether this is a
 Thomas> defect that should be corrected with something like
 Thomas> read-namedtuplestorescan.patch?

It's a defect that should probably be corrected for consistency, though
at present it looks like it's not actually possible to reach the code.
The patch looks good to me though I've not tested it.

Kevin, you want to take it? Or shall I deal with it?

-- 
Andrew (irc:RhodiumToad)


-- 
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] postgres_fdw bug in 9.6

[Michael Paquier]

On Fri, Aug 18, 2017 at 3:59 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita
>>  wrote:
>>> I rebased the patch to HEAD.  PFA a new version of the patch.
>
>> Tom, you were instrumental in identifying what was going wrong here
>> initially.  Any chance you'd be willing to have a look at the patch?
>
> I will, but probably not for a week or so.  Going eclipse-chasing.

Good luck:
https://xkcd.com/1876/
And enjoy:
https://xkcd.com/1877/
-- 
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] Inadequate infrastructure for NextValueExpr

[Thomas Munro]

On Fri, Jul 14, 2017 at 1:46 PM, Thomas Munro
 wrote:
> [...]
> T_NamedTuplestoreScan can be produced by outfuncs.c with tagname
> NAMEDTUPLESTORESCAN but that tagname is not recognized by readfuncs.c
> [...]
>
> That revealed a defect in commit
> 18ce3a4ab22d2984f8540ab480979c851dae5338 which I think should be
> corrected with something like the attached, though I'm not sure if
> it's possible to reach it.

Adding Kevin and Andrew G.  Thoughts on whether this is a defect that
should be corrected with something like
read-namedtuplestorescan.patch?

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


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

Re: [HACKERS] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Tom Lane]

I wrote:
> Ah-hah, I see my dromedary box is one of the ones failing, so I'll
> have a look there.  I can't reproduce it on my other machines.

OK, so this is a whole lot more broken than I thought :-(.

Bear in mind that the plan for this (omitting uninteresting detail) is

 Nested Loop Left Join
   ->  Values Scan on "*VALUES*"
   ->  Finalize GroupAggregate
 ->  Gather Merge
   ->  Partial GroupAggregate
 ->  Sort
   ->  Parallel Seq Scan on tenk1

What seems to be happening is that:

1. On the first pass, the parallel seqscan work gets doled out to several
workers, plus the leader, as expected.

2. When the nestloop rescans its right input, ExecReScanGatherMerge
supposes that this is good enough to handle rescanning its subnodes:

ExecReScan(node->ps.lefttree);

Leaving aside the question of why that doesn't look like nearly every
other child rescan call, what happens is that that invokes ExecReScanAgg,
which does the more usual thing:

if (outerPlan->chgParam == NULL)
ExecReScan(outerPlan);

and so that invokes ExecReScanSort, and then behold what ExecReScanSort
thinks it can optimize away:

 * If subnode is to be rescanned then we forget previous sort results; we
 * have to re-read the subplan and re-sort.  Also must re-sort if the
 * bounded-sort parameters changed or we didn't select randomAccess.
 *
 * Otherwise we can just rewind and rescan the sorted output.

So we never get to ExecReScanSeqScan, and thus not to heap_rescan,
with the effect that parallel_scan->phs_nallocated never gets reset.

3. On the next pass, we fire up all the workers as expected, but they all
perceive phs_nallocated >= rs_nblocks and conclude they have nothing to
do.  Meanwhile, in the leader, nodeSort just re-emits the sorted data it
had last time.  Net effect is that the GatherMerge node returns only the
fraction of the data that was scanned by the leader in the first pass.

4. The fact that the test succeeds on many machines implies that the
leader process is usually doing *all* of the work.  This is in itself not
very good.  Even on the machines where it fails, the fact that the tuple
counts are usually a pretty large fraction of the expected values
indicates that the leader usually did most of the work.  We need to take
a closer look at why that is.

However, the bottom line here is that parallel scan is completely broken
for rescans, and it's not (solely) the fault of nodeGatherMerge; rather,
the issue is that nobody bothered to wire up parallelism to the rescan
parameterization mechanism.  I imagine that related bugs can be
demonstrated in 9.6 with little effort.

I think that the correct fix probably involves marking each parallel scan
plan node as dependent on a pseudo executor parameter, which the parent
Gather or GatherMerge node would flag as being changed on each rescan.
This would cue the plan layers in between that they cannot optimize on the
assumption that the leader's instance of the parallel scan will produce
exactly the same rows as it did last time, even when "nothing else
changed".  The "wtParam" pseudo parameter that's used for communication
between RecursiveUnion and its descendant WorkTableScan node is a good
model for what needs to happen.

A possibly-simpler fix would be to abandon the idea that the leader
should do any of the work, but I imagine that will be unpopular.

As I mentioned, I'm outta here for the next week.  I'd be willing to
work on, or review, a patch for this when I get back.

regards, tom lane

PS: while I was trying to decipher this, I found three or four other
minor bugs or near-bugs in nodeGatherMerge.c.  But none of them seem to be
triggering in this example.  I plan to do a round of code-review-ish fixes
there when I get back.


-- 
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] SCRAM salt length

[Joe Conway]

On 08/17/2017 01:50 PM, Peter Eisentraut wrote:
> On 8/17/17 12:10, Heikki Linnakangas wrote:
>> On 08/17/2017 05:23 PM, Peter Eisentraut wrote:
>>> On 8/17/17 09:21, Heikki Linnakangas wrote:
 The RFC doesn't say anything about salt
 length, but the one example in it uses a 16 byte string as the salt.
 That's more or less equal to the current default of 12 raw bytes, after
 base64-encoding.
>>>
>>> The example is
>>>
>>> S: r=rOprNGfwEbeRWgbNEkqO%hvYDpWUa2RaTCAfuxFIlj)hNlF$k0,
>>>s=W22ZaJ0SNY7soEsUEjb6gQ==,i=4096
>>>
>>> That salt is 24 characters and 16 raw bytes.
>> 
>> Ah, I see, that's from the SCRAM-SHA-256 spec. I was looking at the 
>> example in the original SCRAM-SHA-1 spec:
>> 
>> S: r=fyko+d2lbbFgONRv9qkxdawL3rfcNHYJY1ZVvWVs7j,s=QSXCR+Q6sek8bf92,
>>i=4096
> 
> Hence my original inquiry: "I suspect that this length was chosen based
> on the example in RFC 5802 (SCRAM-SHA-1) section 5.  But the analogous
> example in RFC 7677 (SCRAM-SHA-256) section 3 uses a length of 16.
> Should we use that instead?"

Unless there is some significant downside to using 16 byte salt, that
would be my vote.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] SCRAM salt length

[Peter Eisentraut]

On 8/17/17 12:10, Heikki Linnakangas wrote:
> On 08/17/2017 05:23 PM, Peter Eisentraut wrote:
>> On 8/17/17 09:21, Heikki Linnakangas wrote:
>>> The RFC doesn't say anything about salt
>>> length, but the one example in it uses a 16 byte string as the salt.
>>> That's more or less equal to the current default of 12 raw bytes, after
>>> base64-encoding.
>>
>> The example is
>>
>> S: r=rOprNGfwEbeRWgbNEkqO%hvYDpWUa2RaTCAfuxFIlj)hNlF$k0,
>>s=W22ZaJ0SNY7soEsUEjb6gQ==,i=4096
>>
>> That salt is 24 characters and 16 raw bytes.
> 
> Ah, I see, that's from the SCRAM-SHA-256 spec. I was looking at the 
> example in the original SCRAM-SHA-1 spec:
> 
> S: r=fyko+d2lbbFgONRv9qkxdawL3rfcNHYJY1ZVvWVs7j,s=QSXCR+Q6sek8bf92,
>i=4096

Hence my original inquiry: "I suspect that this length was chosen based
on the example in RFC 5802 (SCRAM-SHA-1) section 5.  But the analogous
example in RFC 7677 (SCRAM-SHA-256) section 3 uses a length of 16.
Should we use that instead?"

-- 
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] expanding inheritance in partition bound order

[Robert Haas]

On Thu, Aug 17, 2017 at 8:39 AM, Ashutosh Bapat
 wrote:
> [2] had a patch with some changes to the original patch you posted. I
> didn't describe those changes in my mail, since they rearranged the
> comments. Those changes are not part of this patch and you haven't
> comments about those changes as well. If you have intentionally
> excluded those changes, it's fine. In case, you haven't reviewed them,
> please see if they are good to be incorporated.

I took a quick look at your version but I think I like Amit's fine the
way it is, so committed that and back-patched it to v10.

I find 0002 pretty ugly as things stand.  We get a bunch of tuple maps
that we don't really need, only to turn around and free them.  We get
a bunch of tuple slots that we don't need, only to turn around and
drop them.  We don't really need the PartitionDispatch objects either,
except for the OIDs they contain.  There's a lot of extra stuff being
computed here that is really irrelevant for this purpose.  I think we
should try to clean that up somehow.

-- 
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] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Aug 17, 2017 at 6:24 AM, Jeevan Ladhe
 wrote:
> I have addressed following comments in V25 patch[1].

Committed 0001.  Since that code seems to be changing about every 10
minutes, it seems best to get this refactoring out of the way before
it changes again.

-- 
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] postgres_fdw bug in 9.6

[Tom Lane]

Robert Haas  writes:
> On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita
>  wrote:
>> I rebased the patch to HEAD.  PFA a new version of the patch.

> Tom, you were instrumental in identifying what was going wrong here
> initially.  Any chance you'd be willing to have a look at the patch?

I will, but probably not for a week or so.  Going eclipse-chasing.

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] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Tom Lane]

Robert Haas  writes:
> On Thu, Aug 17, 2017 at 2:06 PM, Tom Lane  wrote:
>> Nope, spoke too soon.  See buildfarm.

> Whoa, that's not good.

Ah-hah, I see my dromedary box is one of the ones failing, so I'll
have a look there.  I can't reproduce it on my other machines.

I'm a bit suspicious that it's got something to do with getting
a different number of workers during restart.  Whether that's
the issue or not, though, it sure seems like a rescan leaks an
unpleasantly large amount of memory.  I wonder if we shouldn't
refactor this so that the per-reader structs can be reused.

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

[HACKERS] Update low-level backup documentation to match actual behavior

[David Steele]

As discussed in [1] our low-level backup documentation does not quite
match the actual behavior of the functions on primary vs. standby.
Since it appears we have decided that the remaining behavioral
differences after 52f8a59dd953c68 are bugs in the documentation, the
attached is a first pass at bringing the documentation up to date.

The biggest change is to recognize that exclusive backups can only be
run on a primary and to adjust the text accordingly.  Also, I did not
mention the wait_for_archive param in the exclusive instructions since
they are deprecated.

This patch should be sufficient for 10/11 but will need some minor
changes for 9.6 to remove the reference to wait_for_archive.  Note that
this patch ignores Michael's patch [2] to create WAL history files on a
standby as this will likely only be applied to master.

In addition, I have formatted the text to produce minimal diffs for
review, but it could be tightened up before commit.

-- 
-David
da...@pgmasters.net

[1]
https://www.postgresql.org/message-id/20170814152816.GF4628%40tamriel.snowman.net
[2]
https://www.postgresql.org/message-id/CAB7nPqQvVpMsqJExSVXHUwpXFRwojsb-jb4BYnxEQbjJLfw-yQ%40mail.gmail.com
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 0e7c6e2051..cfffea3919 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -890,7 +890,10 @@ SELECT pg_start_backup('label', false, false);
 SELECT * FROM pg_stop_backup(false, true);
 
  This terminates the backup mode and performs an automatic switch to
- the next WAL segment.  The reason for the switch is to arrange for
+ the next WAL segment when run on a primary.  On a standby you can call
+ pg_switch_wal on the primary to perform a manual
+ switch.
+ The reason for the switch is to arrange for
  the last WAL segment file written during the backup interval to be
  ready to archive.
 
@@ -908,9 +911,12 @@ SELECT * FROM pg_stop_backup(false, true);
  Once the WAL segment files active during the backup are archived, you are
  done.  The file identified by pg_stop_backup's first return
  value is the last segment that is required to form a complete set of
- backup files.  If archive_mode is enabled,
+ backup files.  On a primary, if archive_mode is enabled and 
the
+ wait_for_archive parameter is true,
  pg_stop_backup does not return until the last segment has
  been archived.
+ On a standby, archive_mode must be always in order
+ for pg_stop_backup to wait.
  Archiving of these files happens automatically since you have
  already configured archive_command. In most cases this
  happens quickly, but you are advised to monitor your archive
@@ -940,11 +946,12 @@ SELECT * FROM pg_stop_backup(false, true);
 


-Making an exclusive low level backup
+Making an exclusive low level backup on a primary
 
  The process for an exclusive backup is mostly the same as for a
  non-exclusive one, but it differs in a few key steps. It does not allow
- more than one concurrent backup to run, and there can be some issues on
+ more than one concurrent backup to run, must be run on a primary, and 
there
+ can be some issues on
  the server if it crashes during the backup. Prior to PostgreSQL 9.6, this
  was the only low-level method available, but it is now recommended that
  all users upgrade their scripts to use non-exclusive backups if possible.
@@ -1012,15 +1019,10 @@ SELECT pg_start_backup('label', true);
 
 SELECT pg_stop_backup();
 
- This function, when called on a primary, terminates the backup mode and
+ This function terminates the backup mode and
  performs an automatic switch to the next WAL segment. The reason for the
  switch is to arrange for the last WAL segment written during the backup
- interval to be ready to archive.  When called on a standby, this function
- only terminates backup mode.  A subsequent WAL segment switch will be
- needed in order to ensure that all WAL files needed to restore the backup
- can be archived; if the primary does not have sufficient write activity
- to trigger one, pg_switch_wal should be executed on
- the primary.
+ interval to be ready to archive.
 


diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index b43ec30a4e..28eda97273 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18606,7 +18606,8 @@ postgres=# select pg_start_backup('label_goes_here');

 

-The function also creates a backup history file in the write-ahead log
+When executed on a primary, the function also creates a backup history file
+in the write-ahead log
 archive area. The history file includes the label given to
 pg_start_backup, the starting and ending write-ahead log 
locations for
 the backup, and the starting and ending times of the backup.  The return

-- 
Sent via pgsql-hackers mailing 

Re: [HACKERS] postgres_fdw bug in 9.6

[Robert Haas]

On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita
 wrote:
> On 2017/04/04 18:01, Etsuro Fujita wrote:
>> I rebased the patch also.  Please find attached an updated version of the
>> patch.
>
> I rebased the patch to HEAD.  PFA a new version of the patch.

Tom, you were instrumental in identifying what was going wrong here
initially.  Any chance you'd be willing to have a look at the patch?

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


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

Re: [HACKERS] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Robert Haas]

On Thu, Aug 17, 2017 at 2:06 PM, Tom Lane  wrote:
> I wrote:
>> Pushed, with minor tidying of the test case.  I think we can now
>> close this open item.
>
> Nope, spoke too soon.  See buildfarm.
>
> (Man, am I glad I insisted on a test case.)

Whoa, that's not good.

-- 
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] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Tom Lane]

I wrote:
> Pushed, with minor tidying of the test case.  I think we can now
> close this open item.

Nope, spoke too soon.  See buildfarm.

(Man, am I glad I insisted on a test 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] distinct estimate of a hard-coded VALUES list

[Jeff Janes]

On Wed, Aug 16, 2017 at 12:40 PM, Tom Lane  wrote:

> Jeff Janes  writes:
> > This patch still applies, and I think the argument for it is still valid.
> > So I'm going to make a commit-fest entry for it.  Is there additional
> > evidence we should gather?
>
> I think we had consensus to apply this at the start of the next
> development cycle; I just forgot to do it for v10.  Hence, pushed
> into v11.
>

Thanks,

Jeff

Re: [HACKERS] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Tom Lane]

Amit Kapila  writes:
>> This will be fixed by the patch [1] (execrescan_gathermerge_v2.patch)
>> I posted sometime back.  The test case is slightly different, but may
>> I can re post the patch with your test case.

> I have kept the fix as it is but changed the test to match your test.

Pushed, with minor tidying of the test case.  I think we can now
close this open item.

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] pl/perl extension fails on Windows

[Tom Lane]

I wrote:
> Short of declaring this version of Perl unsupported, the only answer
> I can think of is to put a kluge into the MSVC build scripts along
> the lines of "if it's 32-bit Windows, and the Perl version is before X,
> assume we need _USE_32BIT_TIME_T even if $Config{ccflags} doesn't
> say so".  It would be nice to have some hard evidence about what X
> should be, but we don't know when ActiveState fixed this.  (I poked
> around in their bugzilla, without success.)

Ah-hah: it wasn't ActiveState that fixed this at all; it was upstream
Perl.  The stanza that Ashutosh found about defining _USE_32BIT_TIME_T
originated in Perl 5.13.4; older Perls simply don't provide it, no
matter how they were built.

We can now isolate the exact reason we're having trouble on baiji:
it's building Postgres with MSVC 2005, which by default will think
time_t is 64 bits, but it must be using a copy of Perl that was
built with an older Microsoft compiler that doesn't think that.
(Dave's "perl -V" output says ccversion='12.00.8804', but I don't
know how to translate that to the marketing version.)  And since it's
pre-5.13.4, Perl itself doesn't know it should advertise this fact.

So it's now looking to me like we should do the above with X = 5.13.4.
That won't be a perfect solution, but it's about the best we can
readily do.  Realistically, nobody out in the wider world is likely
to care about building current PG releases against such old Perl
versions on Windows; if we satisfy our older buildfarm critters,
it's enough for me.

regards, tom lane


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

Re: [HACKERS] SCRAM salt length

[Heikki Linnakangas]

On 08/17/2017 05:23 PM, Peter Eisentraut wrote:

On 8/17/17 09:21, Heikki Linnakangas wrote:

The RFC doesn't say anything about salt
length, but the one example in it uses a 16 byte string as the salt.
That's more or less equal to the current default of 12 raw bytes, after
base64-encoding.


The example is

S: r=rOprNGfwEbeRWgbNEkqO%hvYDpWUa2RaTCAfuxFIlj)hNlF$k0,
   s=W22ZaJ0SNY7soEsUEjb6gQ==,i=4096

That salt is 24 characters and 16 raw bytes.


Ah, I see, that's from the SCRAM-SHA-256 spec. I was looking at the 
example in the original SCRAM-SHA-1 spec:


S: r=fyko+d2lbbFgONRv9qkxdawL3rfcNHYJY1ZVvWVs7j,s=QSXCR+Q6sek8bf92,
  i=4096

- Heikki


--
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] Extra Vietnamese unaccent rules

[Dang Minh Huong]

Thanks!


On 2017/08/17 11:56, Tom Lane wrote:

Michael Paquier  writes:

On Thu, Aug 17, 2017 at 6:01 AM, Tom Lane  wrote:

I'm not really qualified to review the Python coding
style, but I did fix a typo in a comment.

No pythonist here, but a large confusing "if" condition without any
comments is better if split up and explained with comments if that can
help in clarifying what the code is doing in any language, so thanks
for keeping the code intact.

Certainly agreed on splitting up the logic into multiple statements.
I just meant that I don't know enough Python to know if there are
better ways to do these tests.  (It probably doesn't matter, since
performance of this script is not an issue, and it's not likely to
undergo a lot of further development either.)

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] [PATCH] Push limit to sort through a subquery

[Douglas Doole]

I completely agree. The further a limit can be pushed down, the better.

The patch looks good to me.

On Thu, Aug 17, 2017 at 8:27 AM Konstantin Knizhnik <
k.knizh...@postgrespro.ru> wrote:

>
>
> On 29.04.2017 00:13, Douglas Doole wrote:
>
> If you add this to the commitfest app, more people might look at it when
>> the next commitfest opens.
>
>
> I have added it. https://commitfest.postgresql.org/14/1119/
>
> Also, it might help if you can provide a query/ies with numbers where this
>> optimization shows improvement.
>>
>
> I can't provide the real queries where we encountered the problem because
> they are internal. However I showed a simplified version of the queries in
> my first post.
>
> On our queries, the change made quite a difference - execution time
> dropped from 31.4 seconds to 7.2 seconds. Explain analyze also shows that
> memory use dropped significantly and we didn't have to spill the sort to
> disk
>
> From:
>
> -> Sort (cost=989.95..1013.27 rows=9326 width=30)
> (node_startup_time/loop=31328.891, node_total_time/loop: 31329.756
> rows=2001 loops=1) Buffers: temp read=772 written=11201 lsm_bufmgr
> hits=3392 Sort Key: *** Sort Method: external merge Sort Space Used: 89592
> Sort Space Type: Disk
>
> To:
>
> -> Sort (cost=989.95..1013.27 rows=9326 width=30)
> (node_startup_time/loop=7123.275, node_total_time/loop: 7123.504 rows=2001
> loops=1) Buffers: lsm_bufmgr hits=3387 Sort Key: *** Sort Method: top-N
> heapsort Sort Space Used: 3256 Sort Space Type: Memory
>
>
> Attached please find yet another small patch which pushes down LIMIT to
> ForeignScan.
> I should notice that currently Postgres optimizer is using "Merge Append"
> and fetches from remote nodes only required number of tuples.
> So even without LIMIT push down, postgres_fdw will not pull the whole
> table from remote host.
> postgres_fdw is using cursor for fetching data from remote. Default fetch
> size is 100, so even without limit remote query will fetch no more  than
> 100 rows at remote site.
>
> Assume the following example:
>
> postgres=# create extension postgres_fdw;
> postgres=# create server shard1  FOREIGN DATA WRAPPER postgres_fdw
> options(dbname 'postgres', host 'localhost', port '5432');
> postgres=# create server shard2  FOREIGN DATA WRAPPER postgres_fdw
> options(dbname 'postgres', host 'localhost', port '5432');
> postgres=# CREATE USER MAPPING for $user SERVER shard1 options (user
> '$user');
> postgres=# CREATE USER MAPPING for $user SERVER shard1 options (user
> '$user');
> postgres=# CREATE TABLE t(u integer primary key, v integer);
> postgres=# CREATE TABLE t1(u integer primary key, v integer);
> postgres=# CREATE TABLE t2(u integer primary key, v integer);
> postgres=# insert into t1 values (generate_series(1,10),
> random()*10);
> postgres=# insert into t2 values (generate_series(1,10),
> random()*10);
> postgres=# CREATE FOREIGN TABLE t_fdw1() inherits (t) server shard1
> options(table_name 't1');
> postgres=# CREATE FOREIGN TABLE t_fdw2() inherits (t) server shard2
> options(table_name 't2');
>
>
> postgres=# explain analyze select * from t order by u limit 1;
>   QUERY
> PLAN
>
> ---
>  Limit  (cost=200.15..200.20 rows=1 width=8) (actual time=2.010..2.010
> rows=1 loops=1)
>->  Merge Append  (cost=200.15..449.39 rows=5121 width=8) (actual
> time=2.009..2.009 rows=1 loops=1)
>  Sort Key: t.u
>  ->  Index Scan using t_pkey on t  (cost=0.12..8.14 rows=1
> width=8) (actual time=0.005..0.005 rows=0 loops=1)
>  ->  Foreign Scan on t_fdw2  (cost=100.00..193.92 rows=2560
> width=8) (actual time=1.074..1.074 rows=1 loops=1)
>  ->  Foreign Scan on t_fdw1  (cost=100.00..193.92 rows=2560
> width=8) (actual time=0.928..0.928 rows=1 loops=1)
>  Planning time: 0.769 ms
>  Execution time: 6.837 ms
> (8 rows)
>
> As you can see foreign scan fetches only one row from each remote node.
>
> But still pushing down limit can have positive effect on performance,
> especially if SORT can be replaced with TOP-N.
> I got the following results (time in seconds):
>
> Query
> original
> limit push down
> select * from t order by u limit 1
> 2.276
> 1.777
> select * from t order by v limit 1
> 100 42
>
> There is index for "u", so fetching records with smallest "u" values can
> be done without sorting, so times are similar.
> But in case of sorting by "v", pushing down limit allows to use TOP-1
> instead of global sort and it reduces query execution time more than 2
> times.
>
> --
>
> Konstantin Knizhnik
> 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] pl/perl extension fails on Windows

[Tom Lane]

I wrote:
> Dave Page  writes:
>> C:\Perl\bin>perl -MConfig -e "print $Config{ccflags}"
>> -nologo -GF -W3 -MD -Zi -DNDEBUG -O1 -DWIN32 -D_CONSOLE -DNO_STRICT
>> -DHAVE_DES_FCRYPT -DNO_HASH_SEED -DUSE_SITECUSTOMIZE
>> -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO
>> -DPERL_MSVCRT_READFIX

> So it is indeed not advertising anything about _USE_32BIT_TIME_T ...
> but then how do other Perl extensions work?  Strange.

So we're between a rock and a hard place here.  ActiveState Perl 5.8.8
requires us to use _USE_32BIT_TIME_T, and doesn't appear to display that
requirement in any standard way.  Newer versions of Perl require us not
to use that symbol unless they say so.

Short of declaring this version of Perl unsupported, the only answer
I can think of is to put a kluge into the MSVC build scripts along
the lines of "if it's 32-bit Windows, and the Perl version is before X,
assume we need _USE_32BIT_TIME_T even if $Config{ccflags} doesn't
say so".  It would be nice to have some hard evidence about what X
should be, but we don't know when ActiveState fixed this.  (I poked
around in their bugzilla, without success.)

In the interests of getting the buildfarm green again before I disappear
on vacation, I propose to do the above with X = 5.10.0.  Anybody
have a better idea?

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] [PATCH] Push limit to sort through a subquery

[Konstantin Knizhnik]

On 29.04.2017 00:13, Douglas Doole wrote:


If you add this to the commitfest app, more people might look at
it when the next commitfest opens.


I have added it. https://commitfest.postgresql.org/14/1119/

Also, it might help if you can provide a query/ies with numbers
where this optimization shows improvement.


I can't provide the real queries where we encountered the problem 
because they are internal. However I showed a simplified version of 
the queries in my first post.


On our queries, the change made quite a difference - execution time 
dropped from 31.4 seconds to 7.2 seconds. Explain analyze also shows 
that memory use dropped significantly and we didn't have to spill the 
sort to disk


From:

-> Sort (cost=989.95..1013.27 rows=9326 width=30) 
(node_startup_time/loop=31328.891, node_total_time/loop: 31329.756 
rows=2001 loops=1) Buffers: temp read=772 written=11201 lsm_bufmgr 
hits=3392 Sort Key: *** Sort Method: external merge Sort Space Used: 
89592 Sort Space Type: Disk


To:

-> Sort (cost=989.95..1013.27 rows=9326 width=30) 
(node_startup_time/loop=7123.275, node_total_time/loop: 7123.504 
rows=2001 loops=1) Buffers: lsm_bufmgr hits=3387 Sort Key: *** Sort 
Method: top-N heapsort Sort Space Used: 3256 Sort Space Type: Memory


Attached please find yet another small patch which pushes down LIMIT to 
ForeignScan.
I should notice that currently Postgres optimizer is using "Merge 
Append" and fetches from remote nodes only required number of tuples.
So even without LIMIT push down, postgres_fdw will not pull the whole 
table from remote host.
postgres_fdw is using cursor for fetching data from remote. Default 
fetch size is 100, so even without limit remote query will fetch no 
more  than 100 rows at remote site.


Assume the following example:

postgres=# create extension postgres_fdw;
postgres=# create server shard1  FOREIGN DATA WRAPPER postgres_fdw 
options(dbname 'postgres', host 'localhost', port '5432');
postgres=# create server shard2  FOREIGN DATA WRAPPER postgres_fdw 
options(dbname 'postgres', host 'localhost', port '5432');
postgres=# CREATE USER MAPPING for $user SERVER shard1 options (user 
'$user');
postgres=# CREATE USER MAPPING for $user SERVER shard1 options (user 
'$user');

postgres=# CREATE TABLE t(u integer primary key, v integer);
postgres=# CREATE TABLE t1(u integer primary key, v integer);
postgres=# CREATE TABLE t2(u integer primary key, v integer);
postgres=# insert into t1 values (generate_series(1,10), 
random()*10);
postgres=# insert into t2 values (generate_series(1,10), 
random()*10);
postgres=# CREATE FOREIGN TABLE t_fdw1() inherits (t) server shard1 
options(table_name 't1');
postgres=# CREATE FOREIGN TABLE t_fdw2() inherits (t) server shard2 
options(table_name 't2');



postgres=# explain analyze select * from t order by u limit 1;
  QUERY PLAN
---
 Limit  (cost=200.15..200.20 rows=1 width=8) (actual time=2.010..2.010 
rows=1 loops=1)
   ->  Merge Append  (cost=200.15..449.39 rows=5121 width=8) (actual 
time=2.009..2.009 rows=1 loops=1)

 Sort Key: t.u
 ->  Index Scan using t_pkey on t  (cost=0.12..8.14 rows=1 
width=8) (actual time=0.005..0.005 rows=0 loops=1)
 ->  Foreign Scan on t_fdw2  (cost=100.00..193.92 rows=2560 
width=8) (actual time=1.074..1.074rows=1 loops=1)
 ->  Foreign Scan on t_fdw1  (cost=100.00..193.92 rows=2560 
width=8) (actual time=0.928..0.928rows=1 loops=1)

 Planning time: 0.769 ms
 Execution time: 6.837 ms
(8 rows)

As you can see foreign scan fetches only one row from each remote node.

But still pushing down limit can have positive effect on performance, 
especially if SORT can be replaced with TOP-N.

I got the following results (time in seconds):

Query
original
limit push down
select * from t order by u limit 1
2.276
1.777
select * from t order by v limit 1
100 42


There is index for "u", so fetching records with smallest "u" values can 
be done without sorting, so times are similar.
But in case of sorting by "v", pushing down limit allows to use TOP-1 
instead of global sort and it reduces query execution time more than 2 
times.


--

Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 080cb0a..e3847ce 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2949,7 +2949,8 @@ create_cursor(ForeignScanState *node)
 	initStringInfo();
 	appendStringInfo(, "DECLARE c%u CURSOR FOR\n%s",
 	 fsstate->cursor_number, fsstate->query);
-
+	if (node->limit > 0)
+		appendStringInfo(, " LIMIT %lld", (long long)node->limit);
 	/*
 	 * Notice that we pass NULL for paramTypes, thus 

Re: [HACKERS] WIP: Aggregation push-down

[Antonin Houska]

Antonin Houska  wrote:

> Antonin Houska  wrote:
> 
> > This is a new version of the patch I presented in [1].
> 
> Rebased.
> 
> cat .git/refs/heads/master 
> b9a3ef55b253d885081c2d0e9dc45802cab71c7b

This is another version of the patch.

Besides other changes, it enables the aggregation push-down for postgres_fdw,
although only for aggregates whose transient aggregation state is equal to the
output type. For other aggregates (like avg()) the remote nodes will have to
return the transient state value in an appropriate form (maybe bytea type),
which does not depend on PG version.

shard.tgz demonstrates the typical postgres_fdw use case. One query shows base
scans of base relation's partitions being pushed to shard nodes, the other
pushes down a join and performs aggregation of the join result on the remote
node. Of course, the query can only references one particular partition, until
the "partition-wise join" [1] patch gets committed and merged with this my
patch.

One thing I'm not sure about is whether the patch should remove
GetForeignUpperPaths function from FdwRoutine, which it essentially makes
obsolete. Or should it only be deprecated so far? I understand that
deprecation is important for "ordinary SQL users", but FdwRoutine is an
interface for extension developers, so the policy might be different.

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

Any feedback is appreciated.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at



agg_pushdown_v3.tgz
Description: GNU Zip compressed data


shard.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] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Tom Lane]

Amit Kapila  writes:
> I think the another patch posted above to add a new guc for
> enable_gather is still relevant and important.

FWIW, I'm pretty much -1 on that.  I don't see that it has any
real-world use; somebody who wants to turn that off would likely
really want to turn off parallelism altogether.  We have
mucho knobs in that area already.

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] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Amit Kapila]

On Thu, Aug 17, 2017 at 8:08 PM, Amit Kapila  wrote:
> On Thu, Aug 17, 2017 at 7:49 PM, Tom Lane  wrote:
>> Amit Kapila  writes:
>>> On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane  wrote:
 I should think it wouldn't be that expensive to create a test
 case, if you already have test cases that invoke GatherMerge.
 Adding a right join against a VALUES clause with a small number of
 entries, and a non-mergeable/hashable join clause, ought to do it.
>>
>>> I have done some experiments based on this idea to generate a test,
>>> but I think it is not as straightforward as it appears.
>>
>> I did this (the first 4 SETs duplicate what's already used in
>> select_parallel.sql):
>>
>> regression=# set parallel_setup_cost=0;
>> SET
>> regression=# set parallel_tuple_cost=0;
>> SET
>> regression=# set min_parallel_table_scan_size=0;
>> SET
>> regression=# set max_parallel_workers_per_gather=4;
>> SET
>> regression=# set enable_hashagg TO 0;
>> SET
>> regression=# set enable_material TO 0;
>> SET
>> regression=# explain select * from (select string4, count((unique2))
>> from tenk1 group by string4 order by string4) ss right join
>> (values(1),(2)) v(x) on true;
>> QUERY PLAN
>> --
>>  Nested Loop Left Join  (cost=524.15..1086.77 rows=8 width=76)
>>->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=4)
>>->  Finalize GroupAggregate  (cost=524.15..543.29 rows=4 width=72)
>>  Group Key: tenk1.string4
>>  ->  Gather Merge  (cost=524.15..543.17 rows=16 width=72)
>>Workers Planned: 4
>>->  Partial GroupAggregate  (cost=524.10..542.89 rows=4 
>> width=72)
>>  Group Key: tenk1.string4
>>  ->  Sort  (cost=524.10..530.35 rows=2500 width=68)
>>Sort Key: tenk1.string4
>>->  Parallel Seq Scan on tenk1  
>> (cost=0.00..383.00 rows=2500 width=68)
>> (11 rows)
>>
>> regression=# select * from (select string4, count((unique2))
>> from tenk1 group by string4 order by string4) ss right join
>> (values(1),(2)) v(x) on true;
>> server closed the connection unexpectedly
>>
>>
>> So, not only is it not that hard to reach ExecReScanGatherMerge,
>> but there is indeed a bug to fix there somewhere.  The stack
>> trace indicates that the failure occurs in a later execution
>> of ExecGatherMerge:
>>
>
> This will be fixed by the patch [1] (execrescan_gathermerge_v2.patch)
> I posted sometime back.  The test case is slightly different, but may
> I can re post the patch with your test case.
>

I have kept the fix as it is but changed the test to match your test.
I think the another patch posted above to add a new guc for
enable_gather is still relevant and important.


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


execrescan_gathermerge_v3.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] pl/perl extension fails on Windows

[Tom Lane]

Dave Page  writes:
> Didn't realise I needed the -MConfig bit (told you my perl-fu was weak :-)
> ):

> C:\Perl\bin>perl -MConfig -e "print $Config{ccflags}"
> -nologo -GF -W3 -MD -Zi -DNDEBUG -O1 -DWIN32 -D_CONSOLE -DNO_STRICT
> -DHAVE_DES_FCRYPT -DNO_HASH_SEED -DUSE_SITECUSTOMIZE
> -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO
> -DPERL_MSVCRT_READFIX

So it is indeed not advertising anything about _USE_32BIT_TIME_T ...
but then how do other Perl extensions work?  Strange.

I wonder whether slightly-newer versions of ActiveState Perl work.

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] Add support for tuple routing to foreign partitions

[David Fetter]

On Thu, Aug 17, 2017 at 05:27:05PM +0900, Etsuro Fujita wrote:
> On 2017/07/11 6:56, Robert Haas wrote:
> >On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
> > wrote:
> >>So, I dropped the COPY part.
> >
> >Ouch.  I think we should try to figure out how the COPY part will be
> >handled before we commit to a design.
> 
> I spent some time on this.  To handle that, I'd like to propose doing
> something similar to \copy (frontend copy): submit a COPY query "COPY ...
> FROM STDIN" to the remote server and route data from a file to the remote
> server.  For that, I'd like to add new FDW APIs called during CopyFrom that
> allow us to copy to foreign tables:
> 
> * BeginForeignCopyIn: this would be called after creating a ResultRelInfo
> for the target table (or each leaf partition of the target partitioned
> table) if it's a foreign table, and perform any initialization needed before
> the remote copy can start.  In the postgres_fdw case, I think this function
> would be a good place to send "COPY ... FROM STDIN" to the remote server.
> 
> * ExecForeignCopyInOneRow: this would be called instead of heap_insert if
> the target is a foreign table, and route the tuple read from the file by
> NextCopyFrom to the remote server.  In the postgres_fdw case, I think this
> function would convert the tuple to text format for portability, and then
> send the data to the remote server using PQputCopyData.
> 
> * EndForeignCopyIn: this would be called at the bottom of CopyFrom, and
> release resources such as connections to the remote server.  In the
> postgres_fdw case, this function would do PQputCopyEnd to terminate data
> transfer.

These primitives look good.  I know it seems unlikely at first blush,
but do we know of bulk load APIs for non-PostgreSQL data stores that
this would be unable to serve?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] changed column-count breaks pdf build

[Peter Eisentraut]

On 8/17/17 09:34, Erik Rijkers wrote:
> The feature matrix table in high-availability.sgml had a column added so 
> also increase the column-count (patch attached).

fixed, thanks

-- 
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] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Amit Kapila]

On Thu, Aug 17, 2017 at 7:49 PM, Tom Lane  wrote:
> Amit Kapila  writes:
>> On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane  wrote:
>>> I should think it wouldn't be that expensive to create a test
>>> case, if you already have test cases that invoke GatherMerge.
>>> Adding a right join against a VALUES clause with a small number of
>>> entries, and a non-mergeable/hashable join clause, ought to do it.
>
>> I have done some experiments based on this idea to generate a test,
>> but I think it is not as straightforward as it appears.
>
> I did this (the first 4 SETs duplicate what's already used in
> select_parallel.sql):
>
> regression=# set parallel_setup_cost=0;
> SET
> regression=# set parallel_tuple_cost=0;
> SET
> regression=# set min_parallel_table_scan_size=0;
> SET
> regression=# set max_parallel_workers_per_gather=4;
> SET
> regression=# set enable_hashagg TO 0;
> SET
> regression=# set enable_material TO 0;
> SET
> regression=# explain select * from (select string4, count((unique2))
> from tenk1 group by string4 order by string4) ss right join
> (values(1),(2)) v(x) on true;
> QUERY PLAN
> --
>  Nested Loop Left Join  (cost=524.15..1086.77 rows=8 width=76)
>->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=4)
>->  Finalize GroupAggregate  (cost=524.15..543.29 rows=4 width=72)
>  Group Key: tenk1.string4
>  ->  Gather Merge  (cost=524.15..543.17 rows=16 width=72)
>Workers Planned: 4
>->  Partial GroupAggregate  (cost=524.10..542.89 rows=4 
> width=72)
>  Group Key: tenk1.string4
>  ->  Sort  (cost=524.10..530.35 rows=2500 width=68)
>Sort Key: tenk1.string4
>->  Parallel Seq Scan on tenk1  (cost=0.00..383.00 
> rows=2500 width=68)
> (11 rows)
>
> regression=# select * from (select string4, count((unique2))
> from tenk1 group by string4 order by string4) ss right join
> (values(1),(2)) v(x) on true;
> server closed the connection unexpectedly
>
>
> So, not only is it not that hard to reach ExecReScanGatherMerge,
> but there is indeed a bug to fix there somewhere.  The stack
> trace indicates that the failure occurs in a later execution
> of ExecGatherMerge:
>

This will be fixed by the patch [1] (execrescan_gathermerge_v2.patch)
I posted sometime back.  The test case is slightly different, but may
I can re post the patch with your test case.

[1] - 
https://www.postgresql.org/message-id/CAA4eK1JkByysFJNh9M349u_nNjqETuEnY_y1VUc_kJiU0bxtaQ%40mail.gmail.com

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


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

Re: [HACKERS] SCRAM salt length

[Michael Paquier]

On Thu, Aug 17, 2017 at 10:21 PM, Heikki Linnakangas  wrote:
> On 08/17/2017 05:42 AM, Michael Paquier wrote:
>> That's now or never.
>
> Not really. That constant is just the default to use when creating new
> password verifiers, but the code can handle any salt length, and different
> verifiers can have different lengths.

Indeed, fuzzy memory here. I thought that parse_scram_verifier()
checked the salt length with the default value, but that's not the
case. Perhaps at some point in the development there was a check of
this kind..
-- 
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] SCRAM salt length

[Peter Eisentraut]

On 8/17/17 09:21, Heikki Linnakangas wrote:
> The RFC doesn't say anything about salt 
> length, but the one example in it uses a 16 byte string as the salt. 
> That's more or less equal to the current default of 12 raw bytes, after 
> base64-encoding.

The example is

   S: r=rOprNGfwEbeRWgbNEkqO%hvYDpWUa2RaTCAfuxFIlj)hNlF$k0,
  s=W22ZaJ0SNY7soEsUEjb6gQ==,i=4096

That salt is 24 characters and 16 raw bytes.

-- 
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] pl/perl extension fails on Windows

[Dave Page]

On Thu, Aug 17, 2017 at 2:58 PM, Tom Lane  wrote:

> Dave Page  writes:
> > It's ActiveState Perl 5.8.8. Printing $Config{ccflags} doesn't seem to do
> > anything, but perl -V output is below:
>
> That's weird ... you get nothing from
>
> perl -MConfig -e 'print $Config{ccflags}'
>

Didn't realise I needed the -MConfig bit (told you my perl-fu was weak :-)
):

C:\Perl\bin>perl -MConfig -e "print $Config{ccflags}"
-nologo -GF -W3 -MD -Zi -DNDEBUG -O1 -DWIN32 -D_CONSOLE -DNO_STRICT
-DHAVE_DES_FCRYPT -DNO_HASH_SEED -DUSE_SITECUSTOMIZE
-DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO
-DPERL_MSVCRT_READFIX

-- 
Dave Page
PostgreSQL Core Team
http://www.postgresql.org/

Re: [HACKERS] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Tom Lane]

Amit Kapila  writes:
> On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane  wrote:
>> I should think it wouldn't be that expensive to create a test
>> case, if you already have test cases that invoke GatherMerge.
>> Adding a right join against a VALUES clause with a small number of
>> entries, and a non-mergeable/hashable join clause, ought to do it.

> I have done some experiments based on this idea to generate a test,
> but I think it is not as straightforward as it appears.

I did this (the first 4 SETs duplicate what's already used in
select_parallel.sql):

regression=# set parallel_setup_cost=0;
SET
regression=# set parallel_tuple_cost=0;
SET
regression=# set min_parallel_table_scan_size=0;
SET
regression=# set max_parallel_workers_per_gather=4;
SET
regression=# set enable_hashagg TO 0;
SET
regression=# set enable_material TO 0;
SET
regression=# explain select * from (select string4, count((unique2))
from tenk1 group by string4 order by string4) ss right join
(values(1),(2)) v(x) on true;
QUERY PLAN  
  
--
 Nested Loop Left Join  (cost=524.15..1086.77 rows=8 width=76)
   ->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=4)
   ->  Finalize GroupAggregate  (cost=524.15..543.29 rows=4 width=72)
 Group Key: tenk1.string4
 ->  Gather Merge  (cost=524.15..543.17 rows=16 width=72)
   Workers Planned: 4
   ->  Partial GroupAggregate  (cost=524.10..542.89 rows=4 width=72)
 Group Key: tenk1.string4
 ->  Sort  (cost=524.10..530.35 rows=2500 width=68)
   Sort Key: tenk1.string4
   ->  Parallel Seq Scan on tenk1  (cost=0.00..383.00 
rows=2500 width=68)
(11 rows)

regression=# select * from (select string4, count((unique2))
from tenk1 group by string4 order by string4) ss right join
(values(1),(2)) v(x) on true;
server closed the connection unexpectedly


So, not only is it not that hard to reach ExecReScanGatherMerge,
but there is indeed a bug to fix there somewhere.  The stack
trace indicates that the failure occurs in a later execution
of ExecGatherMerge:

Program terminated with signal 11, Segmentation fault.
#0  0x0064b4e4 in swap_nodes (heap=0x15a9440) at binaryheap.c:223
223 heap->bh_nodes[a] = heap->bh_nodes[b];
(gdb) bt
#0  0x0064b4e4 in swap_nodes (heap=0x15a9440) at binaryheap.c:223
#1  binaryheap_remove_first (heap=0x15a9440) at binaryheap.c:189
#2  0x00634196 in gather_merge_getnext (pstate=)
at nodeGatherMerge.c:479
#3  ExecGatherMerge (pstate=) at nodeGatherMerge.c:241
#4  0x006251fe in ExecProcNode (aggstate=0x157a6d0)
at ../../../src/include/executor/executor.h:249
#5  fetch_input_tuple (aggstate=0x157a6d0) at nodeAgg.c:688
#6  0x00629264 in agg_retrieve_direct (pstate=)
at nodeAgg.c:2313
#7  ExecAgg (pstate=) at nodeAgg.c:2124
#8  0x006396ef in ExecProcNode (pstate=0x1579d98)
at ../../../src/include/executor/executor.h:249
#9  ExecNestLoop (pstate=0x1579d98) at nodeNestloop.c:160
#10 0x0061bc3f in ExecProcNode (queryDesc=0x14d5570, 
direction=, count=0, execute_once=-104 '\230')
at ../../../src/include/executor/executor.h:249
#11 ExecutePlan (queryDesc=0x14d5570, direction=, 
count=0, execute_once=-104 '\230') at execMain.c:1693
#12 standard_ExecutorRun (queryDesc=0x14d5570, 
direction=, count=0, execute_once=-104 '\230')
at execMain.c:362


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] SCRAM salt length

[Robert Haas]

On Thu, Aug 17, 2017 at 9:21 AM, Heikki Linnakangas  wrote:
> Different thing. That was the nonce length, now we're talking about salt
> length.

Actually that commit (0557a5dc2cf845639d384801b6861ebbd35dc7ee) changed both:

-#define SCRAM_RAW_NONCE_LEN 10
+#define SCRAM_RAW_NONCE_LEN 18

 /* length of salt when generating new verifiers */
-#define SCRAM_DEFAULT_SALT_LEN  10
+#define SCRAM_DEFAULT_SALT_LEN  12

I don't think I understand exactly how they're different; especially,
I don't quite understand how the nonce is used.

-- 
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] pl/perl extension fails on Windows

[Tom Lane]

Dave Page  writes:
> It's ActiveState Perl 5.8.8. Printing $Config{ccflags} doesn't seem to do
> anything, but perl -V output is below:

That's weird ... you get nothing from

perl -MConfig -e 'print $Config{ccflags}'

?

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

[HACKERS] changed column-count breaks pdf build

[Erik Rijkers]

The feature matrix table in high-availability.sgml had a column added so 
also increase the column-count (patch attached).


thanks,

Erik Rijkers--- doc/src/sgml/high-availability.sgml.orig	2017-08-17 15:04:32.535819637 +0200
+++ doc/src/sgml/high-availability.sgml	2017-08-17 15:04:46.528122345 +0200
@@ -301,7 +301,7 @@
 
  
   High Availability, Load Balancing, and Replication Feature Matrix
-  
+  

 
  Feature

-- 
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] SCRAM salt length

[Heikki Linnakangas]

On 08/17/2017 04:04 PM, Robert Haas wrote:

On Wed, Aug 16, 2017 at 10:42 PM, Michael Paquier
 wrote:

In the initial discussions there was as well a mention about using 16 bytes.
https://www.postgresql.org/message-id/507550bd.2030...@vmware.com
As we are using SCRAM-SHA-256, let's bump it up and be consistent.
That's now or never.


This was discussed and changed once before at
https://www.postgresql.org/message-id/df8c6e27-4d8e-5281-96e5-131a4e638...@8kdata.com


Different thing. That was the nonce length, now we're talking about salt 
length.


I think 2^96 is large enough. The RFC doesn't say anything about salt 
length, but the one example in it uses a 16 byte string as the salt. 
That's more or less equal to the current default of 12 raw bytes, after 
base64-encoding.


On 08/17/2017 05:42 AM, Michael Paquier wrote:
> That's now or never.

Not really. That constant is just the default to use when creating new 
password verifiers, but the code can handle any salt length, and 
different verifiers can have different lengths.


- Heikki


--
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] SCRAM salt length

[Robert Haas]

On Wed, Aug 16, 2017 at 10:42 PM, Michael Paquier
 wrote:
> In the initial discussions there was as well a mention about using 16 bytes.
> https://www.postgresql.org/message-id/507550bd.2030...@vmware.com
> As we are using SCRAM-SHA-256, let's bump it up and be consistent.
> That's now or never.

This was discussed and changed once before at
https://www.postgresql.org/message-id/df8c6e27-4d8e-5281-96e5-131a4e638...@8kdata.com

-- 
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] Adding support for Default partition in partitioning

[Thom Brown]

On 17 August 2017 at 10:59, Jeevan Ladhe  wrote:
> Hi,
>
> On Tue, Aug 15, 2017 at 7:20 PM, Robert Haas  wrote:
>>
>> On Wed, Jul 26, 2017 at 8:14 AM, Jeevan Ladhe
>>  wrote:
>> > I have rebased the patches on the latest commit.
>>
>> This needs another rebase.
>
>
> I have rebased the patch and addressed your and Ashutosh comments on last
> set of patches.
>
> The current set of patches contains 6 patches as below:
>
> 0001:
> Refactoring existing ATExecAttachPartition  code so that it can be used for
> default partitioning as well
>
> 0002:
> This patch teaches the partitioning code to handle the NIL returned by
> get_qual_for_list().
> This is needed because a default partition will not have any constraints in
> case
> it is the only partition of its parent.
>
> 0003:
> Support for default partition with the restriction of preventing addition of
> any
> new partition after default partition. This is a merge of 0003 and 0004 in
> V24 series.
>
> 0004:
> Extend default partitioning support to allow addition of new partitions
> after
> default partition is created/attached. This patch is a merge of patches
> 0005 and 0006 in V24 series to simplify the review process. The
> commit message has more details regarding what all is included.
>
> 0005:
> This patch introduces code to check if the scanning of default partition
> child
> can be skipped if it's constraints are proven.
>
> 0006:
> Documentation.
>
>
> PFA, and let me know in case of any comments.

Thanks.  Applies fine, and I've been exercising the patch and it is
doing everything it's supposed to do.

I am, however, curious to know why the planner can't optimise the following:

SELECT * FROM mystuff WHERE mystuff = (1::int,'JP'::text,'blue'::text);

This exhaustively checks all partitions, but if I change it to:

SELECT * FROM mystuff WHERE (id, country, content) =
(1::int,'JP'::text,'blue'::text);

It works fine.

The former filters like so: ((mystuff_default_1.*)::mystuff = ROW(1,
'JP'::text, 'blue'::text))

Shouldn't it instead do:

((mystuff_default_1.id, mystuff_default_1.country,
mystuff_default_1.content)::mystuff = ROW(1, 'JP'::text,
'blue'::text))

So it's not really to do with this patch; it's just something I
noticed while testing.

Thom


-- 
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] expanding inheritance in partition bound order

[Ashutosh Bapat]

On Thu, Aug 17, 2017 at 12:59 PM, Amit Langote
 wrote:
>
> Attached rest of the patches.  0001 has changes per Ashutosh's review
> comments [2]:
>
> 0001: Relieve RelationGetPartitionDispatchInfo() of doing any locking

[2] had a patch with some changes to the original patch you posted. I
didn't describe those changes in my mail, since they rearranged the
comments. Those changes are not part of this patch and you haven't
comments about those changes as well. If you have intentionally
excluded those changes, it's fine. In case, you haven't reviewed them,
please see if they are good to be incorporated.

>
> 0002: Teach expand_inherited_rtentry to use partition bound order

0004 in [1] expands a multi-level partition hierarchy into similar
inheritance hierarchy. That patch doesn't need all OIDs in one go. It
will have to handle the partition hierarchy level by level, so most of
the code added by this patch will need to be changed by that patch. Is
there a way we can somehow change this patch so that the delta in 0004
is reduced? That may need rethinking about using
RelationGetPartitionDispatchInfo().

[1] 
https://www.postgresql.org/message-id/cafjfprfkr7igcgbbwh1pq__w-xpy1o79y-qxcmjc6fizlqf...@mail.gmail.com
[2] 
https://www.postgresql.org/message-id/CAFjFpRdXn7w7nVKv4qN9fa%2BtzRi_mJFNCsBWy%3Dbd0SLbYczUfA%40mail.gmail.com


-- 
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] expanding inheritance in partition bound order

[Amit Khandekar]

On 17 August 2017 at 06:39, Amit Langote  wrote:
> Hi Amit,
>
> Thanks for the comments.
>
> On 2017/08/16 20:30, Amit Khandekar wrote:
>> On 16 August 2017 at 11:06, Amit Langote  
>> wrote:
>>
>>> Attached updated patches.
>>
>> Thanks Amit for the patches.
>>
>> I too agree with the overall approach taken for keeping the locking
>> order consistent: it's best to do the locking with the existing
>> find_all_inheritors() since it is much cheaper than to lock them in
>> partition-bound order, the later being expensive since it requires
>> opening partitioned tables.
>
> Yeah.  Per the Robert's design idea, we will always do the *locking* in
> the order determined by find_all_inheritors/find_inheritance_children.
>
>>> I haven't yet done anything about changing the timing of opening and
>>> locking leaf partitions, because it will require some more thinking about
>>> the required planner changes.  But the above set of patches will get us
>>> far enough to get leaf partition sub-plans appear in the partition bound
>>> order (same order as what partition tuple-routing uses in the executor).
>>
>> So, I believe none of the changes done in pg_inherits.c are essential
>> for expanding the inheritence tree in bound order, right ?
>
> Right.
>
> The changes to pg_inherits.c are only about recognizing partitioned tables
> in an inheritance hierarchy and putting them ahead in the returned list.
> Now that I think of it, the title of the patch (teach pg_inherits.c about
> "partitioning") sounds a bit confusing.  In particular, the patch does not
> teach it things like partition bound order, just that some tables in the
> hierarchy could be partitioned tables.
>
>> I am not
>> sure whether we are planning to commit these two things together or
>> incrementally :
>> 1. Expand in bound order
>> 2. Allow for locking only the partitioned tables first.
>>
>> For #1, the changes in pg_inherits.c are not required (viz, keeping
>> partitioned tables at the head of the list, adding inhchildparted
>> column, etc).
>
> Yes.  Changes to pg_inherits.c can be committed completely independently
> of either 1 or 2, although then there would be nobody using that capability.
>
> About 2: I think the capability to lock only the partitioned tables in
> expand_inherited_rtentry() will only be used once we have the patch to do
> the necessary planner restructuring that will allow us to defer child
> table locking to some place that is not expand_inherited_rtentry().  I am
> working on that patch now and should be able to show something soon.
>
>> If we are going to do #2 together with #1, then in the patch set there
>> is no one using the capability introduced by #2. That is, there are no
>> callers of find_all_inheritors() that make use of the new
>> num_partitioned_children parameter. Also, there is no boolean
>> parameter  for find_all_inheritors() to be used to lock only the
>> partitioned tables.
>>
>> I feel we should think about
>> 0002-Teach-pg_inherits.c-a-bit-about-partitioning.patch later, and
>> first get the review done for the other patches.
>
> I think that makes sense.
>
>> I see that RelationGetPartitionDispatchInfo() now returns quite a
>> small subset of what it used to return, which is good. But I feel for
>> expand_inherited_rtentry(), RelationGetPartitionDispatchInfo() is
>> still a bit heavy. We only require the oids, so the
>> PartitionedTableInfo data structure (including the pd->indexes array)
>> gets discarded.
>
> Maybe we could make the output argument optional, but I don't see much
> point in being too conservative here.  Building the indexes array does not
> cost us that much and if a not-too-distant-in-future patch could use that
> information somehow, it could do so for free.

Ok, so these changes are mostly kept keeping in mind some future
use-cases. Otherwise, I was thinking we could just keep a light-weight
function to generate the oids, and keep the current
RelationGetPartitionDispatchInfo() intact.

Anyways, some more comments :

In ExecSetupPartitionTupleRouting(), not sure why ptrinfos array is an
array of pointers. Why can't it be an array of
PartitionTupleRoutingInfo structure  rather than pointer to that
structure ?

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
+ * Close all the leaf partitions and their indices.
*
Above comment needs to be shifted a bit down to the subsequent "for"
loop where it's actually applicable.

* node->mt_partition_dispatch_info[0] corresponds to the root partitioned
* table, for which we didn't create tupslot.
Above : node->mt_partition_dispatch_info[0] => node->mt_ptrinfos[0]

/*
 * XXX- do we need a pinning mechanism for partition descriptors
 * so that there references can be managed independently of
 * the parent relcache entry? Like PinPartitionDesc(partdesc)?
 */
pd->partdesc = partdesc;

Any idea if the above can be handled ? I am not too sure.


>
> Thanks,
> Amit
>

Re: [HACKERS] Garbled comment in postgresGetForeignJoinPaths

[Etsuro Fujita]

On 2017/08/17 1:31, Tom Lane wrote:

postgres_fdw.c around line 4500:

 /*
  * If there is a possibility that EvalPlanQual will be executed, we need
  * to be able to reconstruct the row using scans of the base relations.
  * GetExistingLocalJoinPath will find a suitable path for this purpose in
  * the path list of the joinrel, if one exists.  We must be careful to
  * call it before adding any ForeignPath, since the ForeignPath might
  * dominate the only suitable local path available.  We also do it before
-->  * reconstruct the row for EvalPlanQual(). Find an alternative local path
  * calling foreign_join_ok(), since that function updates fpinfo and marks
  * it as pushable if the join is found to be pushable.
  */

Should the marked line simply be deleted?  If not, what correction is
appropriate?


In relation to this, I updated the patch for addressing the 
GetExistingLocalJoinPath issue [1].


Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/6008ca34-906f-e61d-478b-8f85fde2c090%40lab.ntt.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] postgres_fdw bug in 9.6

[Etsuro Fujita]

On 2017/04/04 18:01, Etsuro Fujita wrote:
I rebased the patch also.  Please find attached an updated version of 
the patch.


I rebased the patch to HEAD.  PFA a new version of the patch.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 1701,1722  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = 
t2.c1) ORDER BY t1.c3, t
 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS 
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) 
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, 
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN 
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST, 
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
!->  Merge Join
   Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!  Merge Cond: (t1.c1 = t2.c1)
!  ->  Sort
 Output: t1.c1, t1.c3, t1.*
!Sort Key: t1.c1
!->  Foreign Scan on public.ft1 t1
!  Output: t1.c1, t1.c3, t1.*
!  Remote SQL: SELECT "C 1", c2, c3, c4, c5, 
c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!  ->  Sort
 Output: t2.c1, t2.*
!Sort Key: t2.c1
!->  Foreign Scan on public.ft2 t2
!  Output: t2.c1, t2.*
!  Remote SQL: SELECT "C 1", c2, c3, c4, c5, 
c6, c7, c8 FROM "S 1"."T 1"
! (23 rows)
  
  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY 
t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
   c1  | c1  
--- 1701,1716 
 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS 
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) 
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, 
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN 
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST, 
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
!->  Nested Loop
   Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!  Join Filter: (t1.c1 = t2.c1)
!  ->  Foreign Scan on public.ft1 t1
 Output: t1.c1, t1.c3, t1.*
!Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, 
c8 FROM "S 1"."T 1" FOR UPDATE
!  ->  Foreign Scan on public.ft2 t2
 Output: t2.c1, t2.*
!Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, 
c8 FROM "S 1"."T 1"
! (17 rows)
  
  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY 
t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
   c1  | c1  
***
*** 1745,1766  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = 
t2.c1) ORDER BY t1.c3, t
 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS 
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) 
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, 
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN 
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST, 
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1 FOR UPDATE OF r2
!->  Merge Join
   Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!  Merge Cond: (t1.c1 = t2.c1)
!  ->  Sort
 Output: t1.c1, t1.c3, t1.*
!Sort Key: t1.c1
!->  Foreign Scan on public.ft1 t1
!  Output: t1.c1, t1.c3, t1.*
!  Remote SQL: SELECT "C 1", c2, c3, c4, c5, 
c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!  ->  Sort
 Output: t2.c1, t2.*
!Sort Key: t2.c1
!->  Foreign Scan on public.ft2 t2
!  Output: t2.c1, t2.*
!  Remote SQL: SELECT "C 1", c2, c3, c4, c5, 
c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
! (23 rows)
  
  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/08/17 20:37, Ashutosh Bapat wrote:

On Thu, Aug 17, 2017 at 1:57 PM, Etsuro Fujita
 wrote:

I spent some time on this.  To handle that, I'd like to propose doing
something similar to \copy (frontend copy): submit a COPY query "COPY ...
FROM STDIN" to the remote server and route data from a file to the remote
server.  For that, I'd like to add new FDW APIs called during CopyFrom that
allow us to copy to foreign tables:


The description seems to support only COPY to a foreign table from a
file, but probably we need the support other way round as well. This
looks like a feature (support copy to and from a foreign table) to be
handled by itself.


Agreed.  I'll consider how to handle copy-from-a-foreign-table as well.

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] Add support for tuple routing to foreign partitions

[Ashutosh Bapat]

On Thu, Aug 17, 2017 at 1:57 PM, Etsuro Fujita
 wrote:
> On 2017/07/11 6:56, Robert Haas wrote:
>>
>> On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
>>  wrote:
>>>
>>> So, I dropped the COPY part.
>>
>>
>> Ouch.  I think we should try to figure out how the COPY part will be
>> handled before we commit to a design.
>
>
> I spent some time on this.  To handle that, I'd like to propose doing
> something similar to \copy (frontend copy): submit a COPY query "COPY ...
> FROM STDIN" to the remote server and route data from a file to the remote
> server.  For that, I'd like to add new FDW APIs called during CopyFrom that
> allow us to copy to foreign tables:

The description seems to support only COPY to a foreign table from a
file, but probably we need the support other way round as well. This
looks like a feature (support copy to and from a foreign table) to be
handled by itself.

-- 
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] Fix number skipping in to_number

[Oliver Ford]

On Thu, Aug 17, 2017 at 11:55 AM, Thomas Munro
 wrote:

> Hmm.  Just in case my macOS laptop (CC=Apple's clang,
> LANG=en_NZ.UTF-8) was unduly affected by cosmic rays I tried on a
> couple of nearby servers running FreeBSD 11.1 (CC=clang, LANG=)
> and CentOS 7.3 (CC=gcc, LANG=en_US.utf-8) and got the same result:
> int8 has lost some whitespace in to_char output.
>
> It looks a bit like it has lost a leading space for every ',' that was
> present in the format string but which didn't finish up getting output
> (because the number was too small).  It looks a bit like that doesn't
> happen for 'G', so let's compare the NUM_COMMA and NUM_G cases.  Ahh..
> I'm not sure, but I think you might have a close-brace in the wrong
> place here:
>
> @@ -4879,6 +4883,10 @@ NUM_processor(FormatNode *node, NUMDesc *Num,
> char *inout,
> continue;
> }
> }<--- this guy here might
> need to move after "noadd = true"?
> +   if
> (strncmp(Np->inout_p, Np->L_thousands_sep, separator_len) == 0)
> +   Np->inout_p +=
> separator_len - 1;
> +   else
> +   noadd = true;
> break;
>
> case NUM_G:
>
> With that change the test passes for me.  But why do we get different results?
>
> --
> Thomas Munro
> http://www.enterprisedb.com

Ok I've made that change in the attached v3. I'm not sure as I'm on
en_US.UTF-8 locale too. Maybe something Windows specific?


0001-apply-number-v3.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] Default Partition for Range

[Beena Emerson]

PFA the patch rebased over v25 patches of default list partition [1]

[1] 
https://www.postgresql.org/message-id/CAOgcT0NwqnavYtu-QM-DAZ6N%3DwTiqKgy83WwtO2x94LSLZ1-Sw%40mail.gmail.com




-- 

Beena Emerson

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


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

[Masahiko Sawada]

On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut
 wrote:
> On 8/8/17 05:58, Masahiko Sawada wrote:
>> Are you planning to work on remaining patches 0005 and 0006 that
>> improve the subscription codes in PG11 cycle? If not, I will take over
>> them and work on the next CF.
>
> Based on your assessment, the remaining patches were not required bug
> fixes.  So I think preparing them for the next commit fest would be great.
>

Thank you for the comment.

After more thought, since 0001 and 0003 patches on the first mail also
improve the subscription codes and are worth to be considered, I
picked total 4 patches up and updated them. I'm planning to work on
these patches in the next CF.

Regards,

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


0001-Improve-messaging-during-logical-replication-worker-_v2.patch
Description: Binary data


0002-Split-the-SetSubscriptionRelState-function-into-two_v2.patch
Description: Binary data


0003-Allow-syscache-access-to-subscriptions-in-database-l_v2.patch
Description: Binary data


0004-Improve-locking-for-subscriptions-and-subscribed-rel_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] Fix number skipping in to_number

[Thomas Munro]

On Thu, Aug 17, 2017 at 9:48 PM, Oliver Ford  wrote:
>> The tests you added pass for me but the int8 test now fails with the
>> following (this is from regression.diff after running
>> 'PG_REGRESS_DIFF_OPTS="-dU10" make check').  It looks like some new
>> whitespace is appearing on the right in the output of to_char().  I
>> didn't try to understand why.
>>
>> @@ -453,34 +453,34 @@
>>  --+--
>>   4567890123456789 | 4567890123456789
>>  (1 row)
>>
>>  -- TO_CHAR()
>>  --
>>  SELECT '' AS to_char_1, to_char(q1, '9G999G999G999G999G999'),
>> to_char(q2, '9,999,999,999,999,999')
>> FROM INT8_TBL;
>>   to_char_1 |to_char |to_char
>>  ---++
>> -   |123 |456
>> +   |123 |   456
>> |123 |  4,567,890,123,456,789
>> -   |  4,567,890,123,456,789 |123
>> +   |  4,567,890,123,456,789 |   123
>> |  4,567,890,123,456,789 |  4,567,890,123,456,789
>> |  4,567,890,123,456,789 | -4,567,890,123,456,789
>>  (5 rows)
>>
>>  SELECT '' AS to_char_2, to_char(q1, '9G999G999G999G999G999D999G999'),
>> to_char(q2, '9,999,999,999,999,999.999,999')
>> FROM INT8_TBL;
>>   to_char_2 |to_char |to_char
>>  
>> ---++
>> -   |123.000,000 |456.000,000
>> +   |123.000,000 |   456.000,000
>> |123.000,000 |  4,567,890,123,456,789.000,000
>> -   |  4,567,890,123,456,789.000,000 |123.000,000
>> +   |  4,567,890,123,456,789.000,000 |   123.000,000
>> |  4,567,890,123,456,789.000,000 |  4,567,890,123,456,789.000,000
>> |  4,567,890,123,456,789.000,000 | -4,567,890,123,456,789.000,000
>>  (5 rows)
>>
>>  SELECT '' AS to_char_3, to_char( (q1 * -1), 'PR'),
>> to_char( (q2 * -1), '.999PR')
>> FROM INT8_TBL;
>>   to_char_3 |  to_char   |to_char
>>  ---++
>> |  <123> |  <456.000>
>> |  <123> | <4567890123456789.000>
>>
>
> That's strange, I can't replicate that issue on my Windows build. I've
> tried with 'PG_REGRESS_DIFF_OPTS="-dU10" make check' and all the tests
> (including int8) pass fine. The spacing in the results is perfectly
> normal. I wonder if something else on your build is causing this? I've
> also tried several "make check" options for different locales
> mentioned in the docs and they pass fine.

Hmm.  Just in case my macOS laptop (CC=Apple's clang,
LANG=en_NZ.UTF-8) was unduly affected by cosmic rays I tried on a
couple of nearby servers running FreeBSD 11.1 (CC=clang, LANG=)
and CentOS 7.3 (CC=gcc, LANG=en_US.utf-8) and got the same result:
int8 has lost some whitespace in to_char output.

It looks a bit like it has lost a leading space for every ',' that was
present in the format string but which didn't finish up getting output
(because the number was too small).  It looks a bit like that doesn't
happen for 'G', so let's compare the NUM_COMMA and NUM_G cases.  Ahh..
I'm not sure, but I think you might have a close-brace in the wrong
place here:

@@ -4879,6 +4883,10 @@ NUM_processor(FormatNode *node, NUMDesc *Num,
char *inout,
continue;
}
}<--- this guy here might
need to move after "noadd = true"?
+   if
(strncmp(Np->inout_p, Np->L_thousands_sep, separator_len) == 0)
+   Np->inout_p +=
separator_len - 1;
+   else
+   noadd = true;
break;

case NUM_G:

With that change the test passes for me.  But why do we get different results?

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

>> > 0007:
> >> > This patch introduces code to check if the scanning of default
> partition
> >> > child
> >> > can be skipped if it's constraints are proven.
> >>
> >> If I understand correctly, this is actually a completely separate
> >> feature not intrinsically related to default partitioning.
> >
> > I don't see this as a new feature, since scanning the default partition
> > will be introduced by this series of patches only, and rather than a
> > feature this can be classified as a completeness of default skip
> > validation logic. Your thoughts?
>
> Currently, when a partitioned table is attached, we check whether all
> the scans can be checked but not whether scans on some partitions can
> be attached.  So there are two separate things:
>
> 1. When we introduce default partitioning, we need scan the default
> partition either when (a) any partition is attached or (b) any
> partition is created.
>
> 2. In any situation where scans are needed (scanning the partition
> when it's attached, scanning the default partition when some other
> partition is attached, scanning the default when a new partition is
> created), we can run predicate_implied_by for each partition to see
> whether the scan of that partition can be skipped.
>
> Those two changes are independent. We could do (1) without doing (2)
> or (2) without doing (1) or we could do both.  So they are separate
> features.
>
>
In my V25 series(patch 0005) I have only addressed half of (2) above
i.e. code to check whether the scan of a partition of default partition
can be skipped when other partition is being added. Amit Langote
has submitted[1] a separate patch(0003)  to address skipping the scan
of the children of relation when it's being attached as a partition.

[1]
https://www.postgresql.org/message-id/4cd13b03-846d-dc65-89de-1fd9743a3869%40lab.ntt.co.jp

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

Please find my feedback inlined.
I have addressed following comments in V25 patch[1].


> > 0002:
> > This patch teaches the partitioning code to handle the NIL returned by
> > get_qual_for_list().
> > This is needed because a default partition will not have any constraints
> in
> > case
> > it is the only partition of its parent.
>
> Perhaps it would be better to make validatePartConstraint() a no-op
> when the constraint is empty rather than putting the logic in the
> caller.  Otherwise, every place that calls validatePartConstraint()
> has to think about whether or not the constraint-is-NULL case needs to
> be handled.
>
> I have now added a check in ValidatePartConstraint(). This change is made
in 0001 patch.



> > 0003:
> > Support for default partition with the restriction of preventing
> addition of
> > any
> > new partition after default partition.
>
> This looks generally reasonable, but can't really be committed without
> the later patches, because it might break pg_dump, which won't know
> that the DEFAULT partition must be dumped last and might therefore get
> the dump ordering wrong, and of course also because it reverts commit
> c1e0e7e1d790bf18c913e6a452dea811e858b554.
>
>
Thanks Robert for looking into this. The purpose of having different
patches is
just to ease the review process and the basic patch of introducing the
default
partition support and extending support for addition of new partition
should go
together.


> > 0004:
> > Store the default partition OID in pg_partition_table, this will help us
> to
> > retrieve the OID of default relation when we don't have the relation
> cache
> > available. This was also suggested by Amit Langote here[1].
>
> I looked this over and I think this is the right approach.  An
> alternative way to avoid needing a relcache entry in
> heap_drop_with_catalog() would be for get_default_partition_oid() to
> call find_inheritance_children() here and then use a syscache lookup
> to get the partition bound for each one, but that's still going to
> cause some syscache bloat.
>

To safeguard future development from missing this and leaving it out of
sync, I
have added an Assert in RelationBuildPartitionDesc() to cross check the
partdefid.


>
> > 0005:
> > Extend default partitioning support to allow addition of new partitions.
>
> +   if (spec->is_default)
> +   {
> +   /* Default partition cannot be added if there already
> exists one. */
> +   if (partdesc->nparts > 0 &&
> partition_bound_has_default(boundinfo))
> +   {
> +   with = boundinfo->default_index;
> +   ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +errmsg("partition \"%s\"
> conflicts with existing default partition \"%s\"",
> +   relname,
> get_rel_name(partdesc->oids[with])),
> +parser_errposition(pstate,
> spec->location)));
> +   }
> +
> +   return;
> +   }
>
> I generally think it's good to structure the code so as to minimize
> the indentation level.  In this case, if you did if (partdesc->nparts
> == 0 || !partition_bound_has_default(boundinfo)) return; first, then
> the rest of it could be one level less indented.  Also, perhaps it
> would be clearer to test boundinfo == NULL rather than
> partdesc->nparts == 0, assuming they are equivalent.
>

Fixed.

> 0006:
> > Extend default partitioning validation code to reuse the refactored code
> in
> > patch 0001.
>
> I'm having a very hard time understanding what's going on with this
> patch.  It certainly doesn't seem to be just refactoring things to use
> the code from 0001.  For example:
>
> -   if (ExecCheck(partqualstate, econtext))
> +   if (!ExecCheck(partqualstate, econtext))
>
> It seems hard to believe that refactoring things to use the code from
> 0001 would involve inverting the value of this test.
>
> +* derived from the bounds(the partition constraint
> never evaluates to
> +* NULL, so negating it like this is safe).
>
> I don't see it being negated.
>
> I think this patch needs a better explanation of what it's trying to
> do, and better comments.  I gather that at least part of the point
> here is to skip validation scans on default partitions if the default
> partition has been constrained not to contain any values that would
> fall in the new partition, but neither the commit message for 0006 nor
> your description here make that very clear.
>

In V25 series, this is now part of patch 0004, and should avoid any
confusion as above. I have tried to add verbose comment in commit
message as well as I have improved the code comments in this code
area.

> [0008 documentation]
>
> -  attached is marked NO INHERIT, the command will
> fail;
> -  such a constraint must be recreated 

Re: [HACKERS] Remove 1MB size limit in tsvector

[Ildus Kurbangaliev]

On Thu, 10 Aug 2017 18:06:17 +0300
Alexander Korotkov  wrote:

> On Wed, Aug 9, 2017 at 7:38 PM, Robert Haas 
> wrote:
> 
> > On Tue, Aug 1, 2017 at 4:00 PM, Ildus K
> >  wrote:  
> > > It's a workaround. DatumGetTSVector and
> > > DatumGetTSVectorCopy will upgrade tsvector on the fly if it
> > > has old format.  
> >
> > Hmm, that seems like a real fix, not just a workaround.  If you can
> > transparently read the old format, there's no problem.  Not sure
> > about performance, though.
> >  
> 
> +1
> Ildus, I think we need to benchmark reading of the old format.  There
> would be tradeoff between performance of old format reading and
> amount of extra code needed.  Once we will have benchmarks we can
> consider whether this is the solution we would like to buy.

In my benchmarks when database fits into buffers (so it's measurement of
the time required for the tsvectors conversion) it gives me these
results:

Without conversion:

$ ./tsbench2 -database test1 -bench_time 300
2017/08/17 12:04:44 Number of connections:  4
2017/08/17 12:04:44 Database:  test1
2017/08/17 12:09:44 Processed: 51419

With conversion:

$ ./tsbench2 -database test1 -bench_time 300
2017/08/17 12:14:31 Number of connections:  4
2017/08/17 12:14:31 Database:  test1
2017/08/17 12:19:31 Processed: 43607

I ran a bunch of these tests, and these results are stable on my
machine. So in these specific tests performance regression about 15%.

Same time I think this could be the worst case, because usually data
is on disk and conversion will not affect so much to performance.

-- 
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
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] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

On Thu, Aug 17, 2017 at 3:41 PM, Jeevan Ladhe  wrote:

> Hi Ashutosh,
>
> Please find my feedback inlined.
>
> On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat <
> ashutosh.ba...@enterprisedb.com> wrote:
>
>> On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
>>  wrote:
>> > Hi,
>> >
>> > I have rebased the patches on the latest commit.
>> >
>>
>> Thanks for rebasing the patches. The patches apply and compile
>> cleanly. make check passes.
>>
>> Here are some review comments
>> 0001 patch
>> Most of this patch is same as 0002 patch posted in thread [1]. I have
>> extensively reviewed that patch for Amit Langote. Can you please compare
>> these
>> two patches and try to address those comments OR just use patch from that
>> thread? For example, canSkipPartConstraintValidation() is named as
>> PartConstraintImpliedByRelConstraint() in that patch. OR
>> +if (scanRel_constr == NULL)
>> +return false;
>> +
>> is not there in that patch since returning false is wrong when
>> partConstraint
>> is NULL. I think this patch needs those fixes. Also, this patch set would
>> need
>> a rebase when 0001 from that thread gets committed.
>>
>>
> I have renamed the canSkipPartConstraintValidation() to
> PartConstraintImpliedByRelConstraint() and made other changes applicable
> per
> Amit’s patch. This patch also refactors the scanning logic in
> ATExecAttachPartition()
> and adds it into a function ValidatePartitionConstraints(), hence I could
> not use
> Amit’s patch as it is. Please have a look into the new patch and let me
> know if it
> looks fine to you.
>
>
>> 0002 patch
>> +if (!and_args)
>> +result = NULL;
>> Add "NULL, if there are not partition constraints e.g. in case of default
>> partition as the only partition.".
>
>
> Added. Please check.
>
>
>> This patch avoids calling
>> validatePartitionConstraints() and hence canSkipPartConstraintValidation()
>> when
>> partConstraint is NULL, but patches in [1] introduce more callers of
>> canSkipPartConstraintValidation() which may pass NULL. So, it's better
>> that we
>> handle that case.
>>
>
> Following code added in patch 0001 now should take care of this.
> +   num_check = (constr != NULL) ? constr->num_check : 0;
>
>
>> 0003 patch
>> +parentRel = heap_open(parentOid, AccessExclusiveLock);
>> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
>> should not heap_open() the parent relation. But this patch still calls
>> heap_open() without giving any counter argument. Also I don't see
>> get_default_partition_oid() using Relation anywhere. If you remove that
>> heap_open() please remove following heap_close().
>> +heap_close(parentRel, NoLock);
>>
>
> As clarified earlier this was addressed in 0004 patch of V24 series. In
> current set of patches this is now addressed in patch 0003 itself.
>
>
>>
>> +/*
>> + * The default partition accepts any
>> non-specified
>> + * value, hence it should not get a mapped index
>> while
>> + * assigning those for non-null datums.
>> + */
>> Instead of "any non-specified value", you may want to use "any value not
>> specified in the lists of other partitions" or something like that.
>>
>
> Changed the comment.
>
>
>>
>> + * If this is a NULL, route it to the null-accepting partition.
>> + * Otherwise, route by searching the array of partition bounds.
>> You may want to write it as "If this is a null partition key, ..." to
>> clarify
>> what's NULL.
>>
>
> Changed the comment.
>
>
>>
>> + * cur_index < 0 means we could not find a non-default partition
>> of
>> + * this parent. cur_index >= 0 means we either found the leaf
>> + * partition, or the next parent to find a partition of.
>> + *
>> + * If we couldn't find a non-default partition check if the
>> default
>> + * partition exists, if it does, get its index.
>> In order to avoid repeating "we couldn't find a ..."; you may want to add
>> ",
>> try default partition if one exists." in the first sentence itself.
>>
>
> Sorry, but I am not really sure how this change would make the comment
> more readable than the current one.
>
>
>> get_default_partition_oid() is defined in this patch and then redefined in
>> 0004. Let's define it only once, mostly in or before 0003 patch.
>>
>
> get_default_partition_oid() is now defined only once in patch 0003.
>
>
>>
>> + * partition strategy. Assign the parent strategy to the default
>> s/parent/parent's/
>>
>
> Fixed.
>
>
>>
>> +-- attaching default partition overlaps if the default partition already
>> exists
>> +CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
>> +CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
>> +ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
>> +ERROR:  

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

Please find my feedback inlined.

On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
>  wrote:
> > Hi,
> >
> > I have rebased the patches on the latest commit.
> >
>
> Thanks for rebasing the patches. The patches apply and compile
> cleanly. make check passes.
>
> Here are some review comments
> 0001 patch
> Most of this patch is same as 0002 patch posted in thread [1]. I have
> extensively reviewed that patch for Amit Langote. Can you please compare
> these
> two patches and try to address those comments OR just use patch from that
> thread? For example, canSkipPartConstraintValidation() is named as
> PartConstraintImpliedByRelConstraint() in that patch. OR
> +if (scanRel_constr == NULL)
> +return false;
> +
> is not there in that patch since returning false is wrong when
> partConstraint
> is NULL. I think this patch needs those fixes. Also, this patch set would
> need
> a rebase when 0001 from that thread gets committed.
>
>
I have renamed the canSkipPartConstraintValidation() to
PartConstraintImpliedByRelConstraint() and made other changes applicable per
Amit’s patch. This patch also refactors the scanning logic in
ATExecAttachPartition()
and adds it into a function ValidatePartitionConstraints(), hence I could
not use
Amit’s patch as it is. Please have a look into the new patch and let me
know if it
looks fine to you.


> 0002 patch
> +if (!and_args)
> +result = NULL;
> Add "NULL, if there are not partition constraints e.g. in case of default
> partition as the only partition.".


Added. Please check.


> This patch avoids calling
> validatePartitionConstraints() and hence canSkipPartConstraintValidation()
> when
> partConstraint is NULL, but patches in [1] introduce more callers of
> canSkipPartConstraintValidation() which may pass NULL. So, it's better
> that we
> handle that case.
>

Following code added in patch 0001 now should take care of this.
+   num_check = (constr != NULL) ? constr->num_check : 0;


> 0003 patch
> +parentRel = heap_open(parentOid, AccessExclusiveLock);
> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
> should not heap_open() the parent relation. But this patch still calls
> heap_open() without giving any counter argument. Also I don't see
> get_default_partition_oid() using Relation anywhere. If you remove that
> heap_open() please remove following heap_close().
> +heap_close(parentRel, NoLock);
>

As clarified earlier this was addressed in 0004 patch of V24 series. In
current set of patches this is now addressed in patch 0003 itself.


>
> +/*
> + * The default partition accepts any non-specified
> + * value, hence it should not get a mapped index
> while
> + * assigning those for non-null datums.
> + */
> Instead of "any non-specified value", you may want to use "any value not
> specified in the lists of other partitions" or something like that.
>

Changed the comment.


>
> + * If this is a NULL, route it to the null-accepting partition.
> + * Otherwise, route by searching the array of partition bounds.
> You may want to write it as "If this is a null partition key, ..." to
> clarify
> what's NULL.
>

Changed the comment.


>
> + * cur_index < 0 means we could not find a non-default partition
> of
> + * this parent. cur_index >= 0 means we either found the leaf
> + * partition, or the next parent to find a partition of.
> + *
> + * If we couldn't find a non-default partition check if the
> default
> + * partition exists, if it does, get its index.
> In order to avoid repeating "we couldn't find a ..."; you may want to add
> ",
> try default partition if one exists." in the first sentence itself.
>

Sorry, but I am not really sure how this change would make the comment
more readable than the current one.


> get_default_partition_oid() is defined in this patch and then redefined in
> 0004. Let's define it only once, mostly in or before 0003 patch.
>

get_default_partition_oid() is now defined only once in patch 0003.


>
> + * partition strategy. Assign the parent strategy to the default
> s/parent/parent's/
>

Fixed.


>
> +-- attaching default partition overlaps if the default partition already
> exists
> +CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
> +CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
> +ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
> +ERROR:  cannot attach a new partition to table "list_parted" having a
> default partition
> For 0003 patch this testcase is same as the testcase in the next hunk; no
> new
> partition can be added after default partition. Please add this testcase in
> next set of patches.
>

Though the 

Re: [HACKERS] PATCH: multivariate histograms and MCV lists

[Adrien Nayrat]

On 08/14/2017 12:48 AM, Tomas Vondra wrote:
> Hi all,
> 
> For PostgreSQL 10 we managed to get the basic CREATE STATISTICS bits in
> (grammar, infrastructure, and two simple types of statistics). See:
> 
> https://commitfest.postgresql.org/13/852/
> 
> This patch presents a rebased version of the remaining parts, adding more
> complex statistic types (MCV lists and histograms), and hopefully some
> additional improvements.
> 
> The code was rebased on top of current master, and I've made various
> improvements to match how the committed parts were reworked. So the basic idea
> and shape remains the same, the tweaks are mostly small.
> 
> 
> regards
> 
> 
> 
> 

Hello,

There is no check of "statistics type/kind" in pg_stats_ext_mcvlist_items and
pg_histogram_buckets.

select stxname,stxkind from pg_statistic_ext ;
  stxname  | stxkind
---+-
 stts3 | {h}
 stts2 | {m}

So you can call :

SELECT * FROM pg_mcv_list_items((SELECT oid FROM pg_statistic_ext WHERE stxname
= 'stts3'));

SELECT * FROM pg_histogram_buckets((SELECT oid FROM pg_statistic_ext WHERE
stxname = 'stts2'), 0);

Both crashes.

Unfotunately, I don't have the knowledge to produce a patch :/

Small fix in documentation, patch attached.


Thanks!

-- 
Adrien NAYRAT

http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 3a86577b0a..a4ab48cc81 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6445,7 +6445,9 @@ SCRAM-SHA-256$iteration count:salt<
 An array containing codes for the enabled statistic types;
 valid values are:
 d for n-distinct statistics,
-f for functional dependency statistics
+f for functional dependency statistics,
+m for mcv statistics,
+h for histogram statistics
   
  
 
diff --git a/doc/src/sgml/planstats.sgml b/doc/src/sgml/planstats.sgml
index 8857fc7542..9faa7ee393 100644
--- a/doc/src/sgml/planstats.sgml
+++ b/doc/src/sgml/planstats.sgml
@@ -653,7 +653,7 @@ Statistics objects:
 pg_mcv_list_items set-returning function.
 
 
-SELECT * FROM pg_mcv_list_items((SELECT oid FROM pg_statistic_ext WHERE staname = 'stts2'));
+SELECT * FROM pg_mcv_list_items((SELECT oid FROM pg_statistic_ext WHERE stxname = 'stts2'));
  index | values  | nulls | frequency
 ---+-+---+---
  0 | {0,0}   | {f,f} |  0.01
@@ -783,7 +783,7 @@ EXPLAIN ANALYZE SELECT * FROM t WHERE a = 1 AND b = 1;
 using a function called pg_histogram_buckets.
 
 
-test=# SELECT * FROM pg_histogram_buckets((SELECT oid FROM pg_statistic_ext WHERE staname = 'stts3'), 0);
+test=# SELECT * FROM pg_histogram_buckets((SELECT oid FROM pg_statistic_ext WHERE stxname = 'stts3'), 0);
  index | minvals | maxvals | nullsonly | mininclusive | maxinclusive | frequency | density  | bucket_volume 
 ---+-+-+---+--+--+---+--+---
  0 | {0,0}   | {3,1}   | {f,f} | {t,t}| {f,f}|  0.01 | 1.68 |  0.005952


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [BUGS] [postgresql 10 beta3] unrecognized node type: 90

[Amit Kapila]

On Thu, Aug 17, 2017 at 10:07 AM, Amit Kapila  wrote:
> On Tue, Aug 15, 2017 at 7:16 PM, Tom Lane  wrote:
>>
>>> I believe that between this commit and the test-coverage commit from
>>> Andres, this open item is reasonably well addressed.  If someone
>>> thinks more needs to be done, please specify.  Thanks.
>>
>> How big a deal do we think test coverage is?  It looks like
>> ExecReScanGatherMerge is identical logic to ExecReScanGather,
>> which *is* covered according to coverage.postgresql.org, but
>> it wouldn't be too surprising if they diverge in future.
>>
>> I should think it wouldn't be that expensive to create a test
>> case, if you already have test cases that invoke GatherMerge.
>> Adding a right join against a VALUES clause with a small number of
>> entries, and a non-mergeable/hashable join clause, ought to do it.
>>
>
>
> Another way to make it parallel is, add a new guc enable_gather
> similar to enable_gathermerge and then set that to off, it will prefer
> GatherMerge in that case.  I think it is anyway good to have such a
> guc.  I will go and do it this way unless you have a better idea.
>

Going by above, I have created two separate patches.  First to
introduce a new guc enable_gather and second patch to test the rescan
behavior of gather merge.  I have found a problem in the rescan path
of gather merge which is that it is not initializing the gather merge
state which is required to initialize the heap for processing of
tuples.  I think this should have been caught earlier, but probably I
didn't notice it because in the previous tests left side would not
have passed enough rows to hit this case.  I have fixed it in the
attached patch (execrescan_gathermerge_v2).

> Note - enable_gathermerge is not present in postgresql.conf. I think
> we should add it in the postgresql.conf.sample file.
>

Thomas has already posted a patch to handle this problem.

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


guc_enable_gather_v1.patch
Description: Binary data


execrescan_gathermerge_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] Fix number skipping in to_number

[Oliver Ford]

> The tests you added pass for me but the int8 test now fails with the
> following (this is from regression.diff after running
> 'PG_REGRESS_DIFF_OPTS="-dU10" make check').  It looks like some new
> whitespace is appearing on the right in the output of to_char().  I
> didn't try to understand why.
>
> @@ -453,34 +453,34 @@
>  --+--
>   4567890123456789 | 4567890123456789
>  (1 row)
>
>  -- TO_CHAR()
>  --
>  SELECT '' AS to_char_1, to_char(q1, '9G999G999G999G999G999'),
> to_char(q2, '9,999,999,999,999,999')
> FROM INT8_TBL;
>   to_char_1 |to_char |to_char
>  ---++
> -   |123 |456
> +   |123 |   456
> |123 |  4,567,890,123,456,789
> -   |  4,567,890,123,456,789 |123
> +   |  4,567,890,123,456,789 |   123
> |  4,567,890,123,456,789 |  4,567,890,123,456,789
> |  4,567,890,123,456,789 | -4,567,890,123,456,789
>  (5 rows)
>
>  SELECT '' AS to_char_2, to_char(q1, '9G999G999G999G999G999D999G999'),
> to_char(q2, '9,999,999,999,999,999.999,999')
> FROM INT8_TBL;
>   to_char_2 |to_char |to_char
>  ---++
> -   |123.000,000 |456.000,000
> +   |123.000,000 |   456.000,000
> |123.000,000 |  4,567,890,123,456,789.000,000
> -   |  4,567,890,123,456,789.000,000 |123.000,000
> +   |  4,567,890,123,456,789.000,000 |   123.000,000
> |  4,567,890,123,456,789.000,000 |  4,567,890,123,456,789.000,000
> |  4,567,890,123,456,789.000,000 | -4,567,890,123,456,789.000,000
>  (5 rows)
>
>  SELECT '' AS to_char_3, to_char( (q1 * -1), 'PR'),
> to_char( (q2 * -1), '.999PR')
> FROM INT8_TBL;
>   to_char_3 |  to_char   |to_char
>  ---++
> |  <123> |  <456.000>
> |  <123> | <4567890123456789.000>
>

That's strange, I can't replicate that issue on my Windows build. I've
tried with 'PG_REGRESS_DIFF_OPTS="-dU10" make check' and all the tests
(including int8) pass fine. The spacing in the results is perfectly
normal. I wonder if something else on your build is causing this? I've
also tried several "make check" options for different locales
mentioned in the docs and they pass fine.

>
> One superficial comment after first glimpse at the patch:
>
> +if(!strncmp(Np->inout_p, Np->L_thousands_sep, separator_len))
>
> I believe the usual coding around here would be if (strncmp(...) == 0)
>

Yes you're right, that is the coding standard. I've changed it to that
in the attached v2.


0001-apply-number-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] recovery_target_time = 'now' is not an error but still impractical setting

[Simon Riggs]

On 15 August 2017 at 15:37, Piotr Stefaniak  wrote:

> One thing I tried was a combination of recovery_target_action =
> 'shutdown' and recovery_target_time = 'now'. The result is surprising

Indeed, special timestamp values were never considered in the design,
so I'm not surprised they don't work and have never been tested.

Your suggestion of "furthest" is already the default behaviour.

Why are you using 'now'? Why would you want to pick a randomly
selected end time?

recovery_target_time = 'allballs' sounds fun for recovering corrupted databases

-- 
Simon Riggshttp://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

[Dilip Kumar]

On Thu, Aug 17, 2017 at 2:09 PM, Dilip Kumar  wrote:
>
> Either we can pass "num_gene" to merge_clump or we can store num_gene
> in the root. And inside merge_clump we can check. Do you see some more
> complexity?
>
After putting some more thought I see one more problem but not sure
whether we can solve it easily. Now, if we skip generating the gather
path at top level node then our cost comparison while adding the
element to pool will not be correct as we are skipping some of the
paths (gather path).  And, it's very much possible that the path1 is
cheaper than path2 without adding gather on top of it but with gather,
path2 can be cheaper.  But, there is not an easy way to handle it
because without targetlist we can not calculate the cost of the
gather(which is the actual problem).

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] Refactoring identifier checks to consistently use strcmp

[Daniel Gustafsson]

> On 16 Aug 2017, at 17:51, Tom Lane  wrote:
> 
> Heikki Linnakangas  writes:
>> This no longer works:
> 
>> postgres=# CREATE TEXT SEARCH DICTIONARY public.simple_dict (
>> TEMPLATE = pg_catalog.simple,
>> "STOPWORds" = english
>> );
>> ERROR:  unrecognized simple dictionary parameter: "STOPWORds"
> 
>> In hindsight, perhaps we should always have been more strict about that 
>> to begin wtih, but let's not break backwards-compatibility without a 
>> better reason. I didn't thoroughly check all of the cases here, to see 
>> if there are more like this.
> 
> You have a point, but I'm not sure that this is such a bad compatibility
> break as to be a reason not to change things to be more consistent.

I agree with this, but I admittedly have no idea how common the above case
would be in the wild.

>> It'd be nice to have some kind of a rule on when pg_strcasecmp should be 
>> used and when strcmp() is enough. Currently, by looking at the code, I 
>> can't tell.
> 
> My thought is that if we are looking at words that have been through the
> parser, then it should *always* be plain strcmp; we should expect that
> the parser already did the appropriate case-folding.

+1

> pg_strcasecmp would be appropriate, perhaps, if we're dealing with stuff
> that somehow came in without going through the parser.

In that case it would be up to the consumer of the data to handle required
case-folding for the expected input, so pg_strcasecmp or strcmp depending on
situation.

cheers ./daniel


-- 
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 Append implementation

[Amit Khandekar]

On 16 August 2017 at 18:34, Robert Haas  wrote:
> Thanks for the benchmarking results!
>
> On Tue, Aug 15, 2017 at 11:35 PM, Rafia Sabih
>  wrote:
>> Q4 | 244 | 12 | PA and PWJ, time by only PWJ - 41
>
> 12 seconds instead of 244?  Whoa.  I find it curious that we picked a
> Parallel Append with a bunch of non-partial plans when we could've
> just as easily picked partial plans, or so it seems to me.  To put
> that another way, why did we end up with a bunch of Bitmap Heap Scans
> here instead of Parallel Bitmap Heap Scans?
>
>> Q7 | 134 | 88 | PA only
>> Q18 | 508 | 489 | PA only
>
> What's interesting in these results is that the join order changes
> quite a lot when PA is in the mix, and I don't really see why that
> should happen.

Yes, it seems hard to determine why exactly the join order changes.
Parallel Append is expected to give the benefit especially if there
are no partial subplans. But for all of the cases here, partial
subplans seem possible, and so even on HEAD it executed Partial
Append. So between a Parallel Append having partial subplans and a
Partial Append having partial subplans , the cost difference would not
be significant. Even if we assume that Parallel Append was chosen
because its cost turned out to be a bit cheaper, the actual
performance gain seems quite large as compared to the expected cost
difference. So it might be even possible that the performance gain
might be due to some other reasons. I will investigate this, and the
other queries.



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

[Dilip Kumar]

On Sat, Aug 12, 2017 at 6:48 PM, Amit Kapila  wrote:
> On Thu, Aug 10, 2017 at 1:07 AM, Robert Haas  wrote:
>> On Tue, Aug 8, 2017 at 3:50 AM, Amit Kapila  wrote:
>>> Right.
>>>
>
> I think skipping a generation of gather paths for scan node or top
> level join node generated via standard_join_search seems straight
> forward, but skipping for paths generated via geqo seems to be tricky
> (See use of generate_gather_paths in merge_clump).

Either we can pass "num_gene" to merge_clump or we can store num_gene
in the root. And inside merge_clump we can check. Do you see some more
complexity?

if (joinrel)

{
/* Create GatherPaths for any useful partial paths for rel */
if (old_clump->size + new_clump->size < num_gene)
  generate_gather_paths(root, joinrel);

}

  Assuming, we find
> some way to skip it for top level scan/join node, I don't think that
> will be sufficient, we have some special way to push target list below
> Gather node in apply_projection_to_path, we need to move that part as
> well in generate_gather_paths.
>

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] Default Partition for Range

[Beena Emerson]

Hello,

PFA the updated patch which returns NULL instead of true when the
default partition has no constraints and also have modified the output
as discussed above.

This applies over v24 patch [1] of default list partition. I will
rebase over the next version when it is updated.

[1] 
https://www.postgresql.org/message-id/CAOgcT0OVwDu%2BbeChWb5R5s6rfKLCiWcZT5617hqu7T3GdA1hAw%40mail.gmail.com
-- 

Beena Emerson

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


default_range_partition_v11.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] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/07/11 6:56, Robert Haas wrote:

On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
 wrote:

So, I dropped the COPY part.


Ouch.  I think we should try to figure out how the COPY part will be
handled before we commit to a design.


I spent some time on this.  To handle that, I'd like to propose doing 
something similar to \copy (frontend copy): submit a COPY query "COPY 
... FROM STDIN" to the remote server and route data from a file to the 
remote server.  For that, I'd like to add new FDW APIs called during 
CopyFrom that allow us to copy to foreign tables:


* BeginForeignCopyIn: this would be called after creating a 
ResultRelInfo for the target table (or each leaf partition of the target 
partitioned table) if it's a foreign table, and perform any 
initialization needed before the remote copy can start.  In the 
postgres_fdw case, I think this function would be a good place to send 
"COPY ... FROM STDIN" to the remote server.


* ExecForeignCopyInOneRow: this would be called instead of heap_insert 
if the target is a foreign table, and route the tuple read from the file 
by NextCopyFrom to the remote server.  In the postgres_fdw case, I think 
this function would convert the tuple to text format for portability, 
and then send the data to the remote server using PQputCopyData.


* EndForeignCopyIn: this would be called at the bottom of CopyFrom, and 
release resources such as connections to the remote server.  In the 
postgres_fdw case, this function would do PQputCopyEnd to terminate data 
transfer.


I think that would be much more efficient than INSERTing tuples into the 
remote server one by one.  What do you think about that?



I have to admit that I'm a little bit fuzzy about why foreign insert
routing requires all of these changes.  I think this patch would
benefit from being accompanied by several paragraphs of explanation
outlining the rationale for each part of the patch.


Will do.

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] HISTIGNORE for psql

[Pavel Stehule]

2017-08-17 9:23 GMT+02:00 Vesa-Matti J Kari :

>
> Hello,
>
> Bash has HISTIGNORE feature that allows you to exclude certain commands
> from the command history (e.g. shutdown, reboot, rm *).
>
> Would it make any sense to add such a feature to psql (e.g. to ignore
> DROP, DELETE commands)?
>

It is not bad idea.

Regards

Pavel


>
> Regards,
> vmk
> --
> 
>Tietotekniikkakeskus / Helsingin yliopisto
>  IT department / University of Helsinki
> 
>
>
> --
> 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] pl/perl extension fails on Windows

[Dave Page]

On Mon, Aug 14, 2017 at 9:37 PM, Tom Lane  wrote:

> I wrote:
> > Sandeep Thakkar  writes:
> >> We built the sources with this patch and were able to create the plperl
> >> extension on Windows 32bit and 64bit.
>
> > Excellent, thanks for testing.  I'll finish up the configure-script part
> > and push this shortly.
>
> So the early returns from the buildfarm are that this broke baiji,
> although a couple of other Windows critters seem to be OK with it.
>
> This presumably means that baiji's version of perl was built with
> _USE_32BIT_TIME_T, but $Config{ccflags} isn't admitting to that.
> I wonder what Perl version that is exactly, and what it reports for
> $Config{ccflags}, and whether there is some other place that we
> ought to be looking for the info.
>

It's ActiveState Perl 5.8.8. Printing $Config{ccflags} doesn't seem to do
anything, but perl -V output is below:

C:\Program Files\Microsoft Visual Studio 8\VC>c:\perl\bin\perl -V
Summary of my perl5 (revision 5 version 8 subversion 8) configuration:
  Platform:
osname=MSWin32, osvers=4.0, archname=MSWin32-x86-multi-thread
uname=''
config_args='undef'
hint=recommended, useposix=true, d_sigaction=undef
usethreads=define use5005threads=undef useithreads=define
usemultiplicity=de
fine
useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
use64bitint=undef use64bitall=undef uselongdouble=undef
usemymalloc=n, bincompat5005=undef
  Compiler:
cc='cl', ccflags ='-nologo -GF -W3 -MD -Zi -DNDEBUG -O1 -DWIN32
-D_CONSOLE -
DNO_STRICT -DHAVE_DES_FCRYPT -DNO_HASH_SEED -DUSE_SITECUSTOMIZE
-DPERL_IMPLICIT_
CONTEXT -DPERL_IMPLICIT_SYS -DUSE_PERLIO -DPERL_MSVCRT_READFIX',
optimize='-MD -Zi -DNDEBUG -O1',
cppflags='-DWIN32'
ccversion='12.00.8804', gccversion='', gccosandvers=''
intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=10
ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64',
lseeksi
ze=8
alignbytes=8, prototype=define
  Linker and Libraries:
ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf
 -libpath:"C:
\Perl\lib\CORE"  -machine:x86'
libpth=\lib
libs=  oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib
 comdlg32
.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib  netapi32.lib uuid.lib
ws2_
32.lib mpr.lib winmm.lib  version.lib odbc32.lib odbccp32.lib msvcrt.lib
perllibs=  oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib
 comd
lg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib  netapi32.lib
uuid.lib
ws2_32.lib mpr.lib winmm.lib  version.lib odbc32.lib odbccp32.lib msvcrt.lib
libc=msvcrt.lib, so=dll, useshrplib=yes, libperl=perl58.lib
gnulibc_version=''
  Dynamic Linking:
dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug
-opt:ref,icf  -
libpath:"C:\Perl\lib\CORE"  -machine:x86'


Characteristics of this binary (from libperl):
  Compile-time options: MULTIPLICITY PERL_IMPLICIT_CONTEXT
PERL_IMPLICIT_SYS PERL_MALLOC_WRAP
PL_OP_SLAB_ALLOC USE_ITHREADS USE_LARGE_FILES
USE_PERLIO USE_SITECUSTOMIZE
  Locally applied patches:
ActivePerl Build 820 [274739]
Iin_load_module moved for compatibility with build 806
PerlEx support in CGI::Carp
Less verbose ExtUtils::Install and Pod::Find
Patch for CAN-2005-0448 from Debian with modifications
Rearrange @INC so that 'site' is searched before 'perl'
Partly reverted 24733 to preserve binary compatibility
29930 win32.c typo in #define MULTIPLICITY
29868 win32_async_check() can still loop indefinitely
29690,29732 ANSIfy the PATH environment variable on Windows
29689 Add error handling to win32_ansipath
29675 Use short pathnames in $^X and @INC
29607,29676 allow blib.pm to be used for testing Win32 module
29605 Implement killpg() for MSWin32
29598 cwd() to return the short pathname
29597 let readdir() return the alternate filename
29590 Don't destroy the Unicode system environment on Perl startup
29528 get ext/Win32/Win32.xs to compile on cygwin
29509,29510,29511 Move Win32::* functions into Win32 module
29483 Move Win32 from win32/ext/Win32 to ext/Win32
29481 Makefile.PL changes to compile Win32.xs using cygwin
28671 Define PERL_NO_DEV_RANDOM on Windows
28376 Add error checks after execing PL_cshname or PL_sh_path
28305 Pod::Html should not convert "foo" into ``foo''
27833 Change anchor generation in Pod::Html for '=item item 2'
27832,27847 fix Pod::Html::depod() for multi-line strings
27719 Document the functions htmlify() and anchorify() in Pod::Html
27619 Bug in Term::ReadKey 

Re: [HACKERS] SCRAM salt length

[Simon Riggs]

On 16 August 2017 at 14:10, Peter Eisentraut
 wrote:
> The SCRAM salt length is currently set as
>
> /* length of salt when generating new verifiers */
> #define SCRAM_DEFAULT_SALT_LEN 12
>
> without further comment.
>
> I suspect that this length was chosen based on the example in RFC 5802
> (SCRAM-SHA-1) section 5.  But the analogous example in RFC 7677
> (SCRAM-SHA-256) section 3 uses a length of 16.  Should we use that instead?

16 preferred, IMHO

-- 
Simon Riggshttp://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] expanding inheritance in partition bound order

[Amit Langote]

On 2017/08/17 10:09, Amit Langote wrote:
> On 2017/08/16 20:30, Amit Khandekar wrote:
>> On 16 August 2017 at 11:06, Amit Langote  
>> wrote:
>> I am not
>> sure whether we are planning to commit these two things together or
>> incrementally :
>> 1. Expand in bound order
>> 2. Allow for locking only the partitioned tables first.
>>
>> For #1, the changes in pg_inherits.c are not required (viz, keeping
>> partitioned tables at the head of the list, adding inhchildparted
>> column, etc).
> 
> Yes.  Changes to pg_inherits.c can be committed completely independently
> of either 1 or 2, although then there would be nobody using that capability.
> 
> About 2: I think the capability to lock only the partitioned tables in
> expand_inherited_rtentry() will only be used once we have the patch to do
> the necessary planner restructuring that will allow us to defer child
> table locking to some place that is not expand_inherited_rtentry().  I am
> working on that patch now and should be able to show something soon.
> 
>> If we are going to do #2 together with #1, then in the patch set there
>> is no one using the capability introduced by #2. That is, there are no
>> callers of find_all_inheritors() that make use of the new
>> num_partitioned_children parameter. Also, there is no boolean
>> parameter  for find_all_inheritors() to be used to lock only the
>> partitioned tables.
>>
>> I feel we should think about
>> 0002-Teach-pg_inherits.c-a-bit-about-partitioning.patch later, and
>> first get the review done for the other patches.
> 
> I think that makes sense.

After thinking some more on this, I think Amit's suggestion to put this
patch at the end of the priority list is good (that is, the patch that
teaches pg_inherits infrastructure to list partitioned tables ahead in the
list.)  Its purpose is mainly to fulfill the requirement that partitioned
tables be able to be locked ahead of any leaf partitions in the list (per
the design idea Robert suggested [1]).  Patch which requires that
capability is not in the picture yet.  Perhaps, we could review and commit
this patch only when that future patch shows up.  So, I will hold that
patch for now.

Thoughts?

Attached rest of the patches.  0001 has changes per Ashutosh's review
comments [2]:

0001: Relieve RelationGetPartitionDispatchInfo() of doing any locking

0002: Teach expand_inherited_rtentry to use partition bound order

0003: Decouple RelationGetPartitionDispatchInfo() from executor

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmobwbh12OJerqAGyPEjb_%2B2y7T0nqRKTcjed6L4NTET6Fg%40mail.gmail.com

[2]
https://www.postgresql.org/message-id/CAFjFpRdXn7w7nVKv4qN9fa%2BtzRi_mJFNCsBWy%3Dbd0SLbYczUfA%40mail.gmail.com
From 365409b9d7cf723a65b832804bc5002d83ae15d5 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 16 Aug 2017 11:36:14 +0900
Subject: [PATCH 1/3] Relieve RelationGetPartitionDispatchInfo() of doing any
 locking

Anyone who wants to call RelationGetPartitionDispatchInfo() must first
acquire locks using find_all_inheritors.

Doing it this way gets rid of the possibility of a deadlock when partitions
are concurrently locked, because RelationGetPartitionDispatchInfo would lock
the partitions in one order and find_all_inheritors would in another.

Reported-by: Amit Khandekar, Robert Haas
Reports: 
https://postgr.es/m/CAJ3gD9fdjk2O8aPMXidCeYeB-mFB%3DwY9ZLfe8cQOfG4bTqVGyQ%40mail.gmail.com
 
https://postgr.es/m/CA%2BTgmobwbh12OJerqAGyPEjb_%2B2y7T0nqRKTcjed6L4NTET6Fg%40mail.gmail.com
---
 src/backend/catalog/partition.c | 55 ++---
 src/backend/executor/execMain.c | 10 +---
 src/include/catalog/partition.h |  3 +--
 3 files changed, 37 insertions(+), 31 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index c1a307c8d3..96a64ce6b2 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -999,12 +999,16 @@ get_partition_qual_relid(Oid relid)
  * RelationGetPartitionDispatchInfo
  * Returns information necessary to route tuples down a partition 
tree
  *
- * All the partitions will be locked with lockmode, unless it is NoLock.
- * A list of the OIDs of all the leaf partitions of rel is returned in
- * *leaf_part_oids.
+ * The number of elements in the returned array (that is, the number of
+ * PartitionDispatch objects for the partitioned tables in the partition tree)
+ * is returned in *num_parted and a list of the OIDs of all the leaf
+ * partitions of rel is returned in *leaf_part_oids.
+ *
+ * All the relations in the partition tree (including 'rel') must have been
+ * locked (using at least the AccessShareLock) by the caller.
  */
 PartitionDispatch *
-RelationGetPartitionDispatchInfo(Relation rel, int lockmode,
+RelationGetPartitionDispatchInfo(Relation rel,
 int 
*num_parted, List **leaf_part_oids)
 {
 

[HACKERS] HISTIGNORE for psql

[Vesa-Matti J Kari]

Hello,

Bash has HISTIGNORE feature that allows you to exclude certain commands
from the command history (e.g. shutdown, reboot, rm *).

Would it make any sense to add such a feature to psql (e.g. to ignore
DROP, DELETE commands)?

Regards,
vmk
-- 

   Tietotekniikkakeskus / Helsingin yliopisto
 IT department / University of Helsinki



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