Re: [HACKERS] MAIN, Uncompressed?

[Mark Kirkwood]

On 26/08/17 12:18, Simon Riggs wrote:


On 25 August 2017 at 20:53, Tom Lane  wrote:

Greg Stark  writes:

I think this is a particularly old piece of code and we're lucky the
default heuristics have served well for all this time because I doubt
many people fiddle with these storage attributes. The time may have
come to come up with a better UI for the storage attributes because
people are doing new things (like json) and wanting more control over
this heuristic.

Yeah, I could get behind a basic rethinking of the tuptoaster control
knobs.  I'm just not in love with Simon's specific proposal, especially
not if we can't think of a better name for it than "MAINU".

Extended/External would be just fine if you could set the toast
target, so I think a better suggestion would be to make "toast_target"
a per-attribute option .



+1, have thought about this myself previouslythank you for bringing 
it up!


regards

Mark


--
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] Pluggable storage

[Haribabu Kommi]

On Wed, Aug 23, 2017 at 11:59 PM, Amit Kapila 
wrote:

> On Wed, Aug 23, 2017 at 11:05 AM, Haribabu Kommi
>  wrote:
> >
> >
> > On Mon, Aug 21, 2017 at 7:25 PM, Amit Kapila 
> > wrote:
> >>
> >> On Mon, Aug 21, 2017 at 12:58 PM, Haribabu Kommi
> >>  wrote:
> >> >
> >> > On Sun, Aug 13, 2017 at 5:17 PM, Amit Kapila  >
> >> > wrote:
> >> >>
> >> >>
> >> >> Also, it is quite possible that some of the storage Am's don't even
> >> >> want to return bool as a parameter from HeapTupleSatisfies* API's.  I
> >> >> guess what we need here is to provide a way so that different storage
> >> >> am's can register their function pointer for an equivalent to
> >> >> satisfies function.  So, we need to change
> >> >> SnapshotData.SnapshotSatisfiesFunc in some way so that different
> >> >> handlers can register their function instead of using that directly.
> >> >> I think that should address the problem you are planning to solve by
> >> >> omitting buffer parameter.
> >> >
> >> >
> >> > Thanks for your suggestion. Yes, it is better to go in the direction
> of
> >> > SnapshotSatisfiesFunc.
> >> >
> >> > I verified the above idea of implementing the Tuple visibility
> functions
> >> > and assign them into the snapshotData structure based on the snapshot.
> >> >
> >> > The Tuple visibility functions that are specific to the relation are
> >> > available
> >> > with the RelationData structure and this structure may not be
> available,
> >> >
> >>
> >> Which functions are you referring here?  I don't see anything in
> >> tqual.h that uses RelationData.
> >
> >
> >
> > With storage API's, the tuple visibility functions are available with
> > RelationData
> > and those are needs used to update the SnapshotData structure
> > SnapshotSatisfiesFunc member.
> >
> > But the RelationData is not available everywhere, where the snapshot is
> > created,
> > but it is available every place where the tuple visibility is checked.
> So I
> > just changed
> > the way of checking the tuple visibility with the information of
> snapshot by
> > calling
> > the corresponding tuple visibility function from RelationData.
> >
> > If SnapshotData provides MVCC, then the MVCC specific tuple visibility
> > function from
> > RelationData is called. The SnapshotSatisfiesFunc member is changed to a
> > enum
> > that holds the tuple visibility type such as MVCC, DIRTY, SELF and etc.
> > Whenever
> > the visibility check is needed, the corresponding function is called.
> >
>
> It will be easy to understand and see if there is some better
> alternative once you have something in the form of a patch.


Sorry for the delay.

I will submit the new patch series with all comments given
in the upthread to the upcoming commitfest.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] visual studio 2017 build support

[Haribabu Kommi]

On Fri, Aug 25, 2017 at 11:27 PM, Christian Ullrich 
wrote:

> * On 2017-06-21 02:06, Haribabu Kommi wrote:
>
> Thanks for the review. Here I attached an updated patch with README update.
>>
>
> Hello,
>

Thanks for the review.


> the most recent update to VS 2017, version 15.3, now identifies as "14.11"
> rather than "14.10" in the output of nmake /?. Simply adding this value to
> the two places that check for 14.10 in your patch appears to work for me.
>

VS 2017 doesn't change the nmake version to 15, and it is updating with
every minor version, so I changed the check to accept
everything that is greater than 14.10 and eq 15, in case in future if VS
2017 changes the version number.


> In a newly created project, PlatformToolset is still "v141". ToolsVersion
> is "15.0" whereas your patch uses "14.1".
>
> ISTM that the ToolsVersion has been like this in all versions of VS 2017;
> in my collection of .vcxproj files the auto-generated PostgreSQL projects
> are the only ones using "14.1".
>

Updated the Tools version to 15.0 and kept the platform toolset as V141,
this because the toolset is version is still points
to V141, when I create a sample project with VS 2017 and the version number
is inline with nmake version also.


Regards,
Hari Babu
Fujitsu Australia


0001-vs-2017-support_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] MAIN, Uncompressed?

[Simon Riggs]

On 25 August 2017 at 20:53, Tom Lane  wrote:
> Greg Stark  writes:
>> I think this is a particularly old piece of code and we're lucky the
>> default heuristics have served well for all this time because I doubt
>> many people fiddle with these storage attributes. The time may have
>> come to come up with a better UI for the storage attributes because
>> people are doing new things (like json) and wanting more control over
>> this heuristic.
>
> Yeah, I could get behind a basic rethinking of the tuptoaster control
> knobs.  I'm just not in love with Simon's specific proposal, especially
> not if we can't think of a better name for it than "MAINU".

Extended/External would be just fine if you could set the toast
target, so I think a better suggestion would be to make "toast_target"
a per-attribute option .

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

[Peter Geoghegan]

On Tue, Aug 22, 2017 at 4:58 AM, Daniel Verite  wrote:
> For the record, attached are the collname that initdb now creates
> in pg_collation, when compiled successively with all current
> versions of ICU (49 to 59), versus what 10beta2 did.
>
> There are still a few names that get dropped along the ICU
> upgrade path, but now they look like isolated cases.

Even though ICU initdb collations are now as stable as possible, which
is great, I still think that Tom had it right about pg_upgrade: Long
term, it would be preferable if we also did a CREATE COLLATION when
initdb stable collations/base ICU locales go away for pg_upgrade. We
should do such a CREATE COLLATION if and only if that makes the
upgrade succeed where it would otherwise fail. This wouldn't be a
substitute for initdb collation name stability. It would work
alongside it.

This makes sense with ICU. The equivalent of a user-defined CREATE
COLLATION with an old country code may continue to work acceptably
because ICU/CLDR supports aliasing, and/or doesn't actually care that
a deleted country tag (e.g. the one for Serbia and Montenegro [1]) was
used. It'll still interpret Serbian as Serbian (sr-*), regardless of
what country code may also appear, even if the country code is not
just obsolete, but entirely bogus.

Events like the dissolution of countries are rare enough that that
extra assurance is just a nice-to-have, though.

[1] https://en.wikipedia.org/wiki/ISO_3166-2:CS
-- 
Peter Geoghegan


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

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

[Robert Haas]

On Thu, Aug 24, 2017 at 12:59 AM, Michael Paquier
 wrote:
> Robert, Amit and other folks working on extending the existing
> partitioning facility would be in better position to answer that, but
> I would think that we should have something as flexible as possible,
> and storing a list of relation OID in each VacuumRelation makes it
> harder to track the uniqueness of relations vacuumed. I agree that the
> concept of a partition with multiple parents induces a lot of
> problems. But the patch as proposed worries me as it complicates
> vacuum() with a double loop: one for each relation vacuumed, and one
> inside it with the list of OIDs. Modules calling vacuum() could also
> use flexibility, being able to analyze a custom list of columns for
> each relation has value as well.

So ... why have a double loop?  I mean, you could just expand this out
to one entry per relation actually being vacuumed, couldn't you?

What happens if you say VACUUM partitioned_table (a), some_partition (b)?

+oldcontext = MemoryContextSwitchTo(vac_context);
+foreach(lc, relations)
+temp_relations = lappend(temp_relations, copyObject(lfirst(lc)));
+MemoryContextSwitchTo(oldcontext);
+relations = temp_relations;

Can't we just copyObject() on the whole list?

-ListCell   *cur;
-

Why change this?  Generally, declaring a separate variable in an inner
scope seems like better style than reusing one that happens to be
lying around in the outer scope.

+VacuumRelation *relinfo = (VacuumRelation *) lfirst(lc);

Could use lfirst_node.

-- 
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] Variable substitution in psql backtick expansion

[Robert Haas]

On Fri, Aug 25, 2017 at 6:43 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Aug 25, 2017 at 6:09 PM, Tom Lane  wrote:
>>> ...  I'm still not sure that there's any use case for the
>>> string versions ("9.6.4" etc).
>
>> If somebody's doing comparisons, they probably want the numeric
>> version, but somebody might want to print the string version in an
>> error message e.g. \if  \echo this thing
>> doesn't work on :VERSION_NAME \quit \endif
>
> OK, but if human-friendly display is the use-case then it ought to
> duplicate what psql itself would print in, eg, the startup message about
> server version mismatch.  The v4 patch does not, in particular it neglects
> PQparameterStatus(pset.db, "server_version").  This would result in
> printing, eg, "11.0" when the user would likely rather see "11devel".

Oh.  Well, that seems suboptimal.

-- 
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] Variable substitution in psql backtick expansion

[Tom Lane]

Robert Haas  writes:
> On Fri, Aug 25, 2017 at 6:09 PM, Tom Lane  wrote:
>> ...  I'm still not sure that there's any use case for the
>> string versions ("9.6.4" etc).

> If somebody's doing comparisons, they probably want the numeric
> version, but somebody might want to print the string version in an
> error message e.g. \if  \echo this thing
> doesn't work on :VERSION_NAME \quit \endif

OK, but if human-friendly display is the use-case then it ought to
duplicate what psql itself would print in, eg, the startup message about
server version mismatch.  The v4 patch does not, in particular it neglects
PQparameterStatus(pset.db, "server_version").  This would result in
printing, eg, "11.0" when the user would likely rather see "11devel".

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] Variable substitution in psql backtick expansion

[Robert Haas]

On Fri, Aug 25, 2017 at 6:09 PM, Tom Lane  wrote:
> My question was more about how much of a use-case there is for these
> values if there's no expression language yet.  On reflection though,
> you can use either expr-in-backticks or a server query to make
> comparisons, so there's at least some use-case for the numeric
> versions today.  I'm still not sure that there's any use case for the
> string versions ("9.6.4" etc).

If somebody's doing comparisons, they probably want the numeric
version, but somebody might want to print the string version in an
error message e.g. \if  \echo this thing
doesn't work on :VERSION_NAME \quit \endif

>> - Is it a good idea/practical to prevent the new variables from being
>> modified by the user?
>
> We haven't done that for existing informational variables, only control
> variables that affect psql's behavior.  I think we should continue that
> policy for new informational variables.  If we make them read-only, we
> risk breaking scripts that are using those names for their own purposes.
> If we don't make them read-only, we risk breaking scripts that are using
> those names for their own purposes AND expecting them to provide the
> built-in values.  The latter risk seems strictly smaller, probably much
> smaller.

OK, got it.

>> - I think Pavel's upthread suggestion of prefixing the client
>> variables with "client" to match the way the server variables are
>> named is a good idea.
>
> Well, the issue is the precedent of VERSION for the long-form string
> spelling of psql's version.  But I agree that's not a very nice
> precedent.  No strong opinion here.

Hmm, well I think that's probably a pretty good reason to stick with
the names in the proposed patch.  VERSION seems like it was
shortsighted, but I think we're probably best off being consistent
with the precedent at this point.

-- 
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] Variable substitution in psql backtick expansion

[Tom Lane]

Robert Haas  writes:
> I am attempting to understand the status of this patch.  It looks like
> the patch that was the original subject of this thread was committed
> as f833c847b8fa4782efab45c8371d3cee64292d9b on April 1st by Tom, who
> was its author. Subsequently, a new patch not obviously related to the
> subject line was proposed by Fabien Coelho, and that patch was
> subsequently marked Ready for Committer by Pavel Stehule.  Meanwhile,
> objections were raised by Tom, who seems to think that we should make
> \if accept an expression language before we consider this change.

My question was more about how much of a use-case there is for these
values if there's no expression language yet.  On reflection though,
you can use either expr-in-backticks or a server query to make
comparisons, so there's at least some use-case for the numeric
versions today.  I'm still not sure that there's any use case for the
string versions ("9.6.4" etc).

> - Is it a good idea/practical to prevent the new variables from being
> modified by the user?

We haven't done that for existing informational variables, only control
variables that affect psql's behavior.  I think we should continue that
policy for new informational variables.  If we make them read-only, we
risk breaking scripts that are using those names for their own purposes.
If we don't make them read-only, we risk breaking scripts that are using
those names for their own purposes AND expecting them to provide the
built-in values.  The latter risk seems strictly smaller, probably much
smaller.

> - I think Pavel's upthread suggestion of prefixing the client
> variables with "client" to match the way the server variables are
> named is a good idea.

Well, the issue is the precedent of VERSION for the long-form string
spelling of psql's version.  But I agree that's not a very nice
precedent.  No strong opinion here.

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

[Tom Lane]

Robert Haas  writes:
> On Fri, Aug 25, 2017 at 5:12 PM, Tom Lane  wrote:
>> Here's a reviewed version of the second patch.  I fixed one bug
>> (wrong explain group nesting) and made some additional cosmetic
>> improvements beyond the struct relocation.

> The capitalization of TupleSortInstrumentation is inconsistent with
> TuplesortSpaceType, TuplesortMethod, and Tuplesortstate; I recommend
> we lower-case the S.

Sure, do it that way.

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

[Robert Haas]

On Fri, Aug 25, 2017 at 5:12 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Aug 25, 2017 at 2:54 PM, Tom Lane  wrote:
>>> Hmm, I'm not sure why SortInstrumentation belongs naturally to
>>> tuplesort.h but putting an array of them there would be a "gross
>>> abstraction violation".  Perhaps it would help to rename
>>> struct SharedSortInfo to SortInstrumentationArray, and change its
>>> field names to be less specific to the parallel-worker use case?
>
>> What other use case could there be?  I think an array of
>> SortInstrumentation objects intended to be stored in DSM is fairly
>> clearly a bit of executor-specific machinery and thus properly
>> declared along with the node that contains it.
>
> I'm not really convinced, but it's not worth arguing further.
>
> Here's a reviewed version of the second patch.  I fixed one bug
> (wrong explain group nesting) and made some additional cosmetic
> improvements beyond the struct relocation.

The capitalization of TupleSortInstrumentation is inconsistent with
TuplesortSpaceType, TuplesortMethod, and Tuplesortstate; I recommend
we lower-case the S.

- * Ordinary plan nodes won't do anything here, but parallel-aware plan
- * nodes may need to initialize shared state in the DSM before parallel
- * workers are available.  They can allocate the space they previously
+ * Most plan nodes won't do anything here, but plan nodes that allocated
+ * DSM may need to initialize shared state in the DSM before parallel
+ * workers are launched.  They can allocate the space they previously

On reading this again, it seems to me that the first instance of
"allocated" in this comment block needs to be changed to "estimated",
because otherwise saying they can allocated it later on is
unintelligible.

Other than that, LGTM.

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

[Tom Lane]

Robert Haas  writes:
> On Fri, Aug 25, 2017 at 2:54 PM, Tom Lane  wrote:
>> Hmm, I'm not sure why SortInstrumentation belongs naturally to
>> tuplesort.h but putting an array of them there would be a "gross
>> abstraction violation".  Perhaps it would help to rename
>> struct SharedSortInfo to SortInstrumentationArray, and change its
>> field names to be less specific to the parallel-worker use case?

> What other use case could there be?  I think an array of
> SortInstrumentation objects intended to be stored in DSM is fairly
> clearly a bit of executor-specific machinery and thus properly
> declared along with the node that contains it.

I'm not really convinced, but it's not worth arguing further.

Here's a reviewed version of the second patch.  I fixed one bug
(wrong explain group nesting) and made some additional cosmetic
improvements beyond the struct relocation.

regards, tom lane

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 953e74d..385b325 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2292,15 +2292,21 @@ show_tablesample(TableSampleClause *tsc, PlanState *planstate,
 static void
 show_sort_info(SortState *sortstate, ExplainState *es)
 {
-	if (es->analyze && sortstate->sort_Done &&
-		sortstate->tuplesortstate != NULL)
+	if (!es->analyze)
+		return;
+
+	if (sortstate->sort_Done && sortstate->tuplesortstate != NULL)
 	{
 		Tuplesortstate *state = (Tuplesortstate *) sortstate->tuplesortstate;
+		TupleSortInstrumentation stats;
 		const char *sortMethod;
 		const char *spaceType;
 		long		spaceUsed;
 
-		tuplesort_get_stats(state, , , );
+		tuplesort_get_stats(state, );
+		sortMethod = tuplesort_method_name(stats.sortMethod);
+		spaceType = tuplesort_space_type_name(stats.spaceType);
+		spaceUsed = stats.spaceUsed;
 
 		if (es->format == EXPLAIN_FORMAT_TEXT)
 		{
@@ -2315,6 +2321,51 @@ show_sort_info(SortState *sortstate, ExplainState *es)
 			ExplainPropertyText("Sort Space Type", spaceType, es);
 		}
 	}
+
+	if (sortstate->shared_info != NULL)
+	{
+		int			n;
+		bool		opened_group = false;
+
+		for (n = 0; n < sortstate->shared_info->num_workers; n++)
+		{
+			TupleSortInstrumentation *sinstrument;
+			const char *sortMethod;
+			const char *spaceType;
+			long		spaceUsed;
+
+			sinstrument = >shared_info->sinstrument[n];
+			if (sinstrument->sortMethod == SORT_TYPE_STILL_IN_PROGRESS)
+continue;		/* ignore any unfilled slots */
+			sortMethod = tuplesort_method_name(sinstrument->sortMethod);
+			spaceType = tuplesort_space_type_name(sinstrument->spaceType);
+			spaceUsed = sinstrument->spaceUsed;
+
+			if (es->format == EXPLAIN_FORMAT_TEXT)
+			{
+appendStringInfoSpaces(es->str, es->indent * 2);
+appendStringInfo(es->str,
+ "Worker %d:  Sort Method: %s  %s: %ldkB\n",
+ n, sortMethod, spaceType, spaceUsed);
+			}
+			else
+			{
+if (!opened_group)
+{
+	ExplainOpenGroup("Workers", "Workers", false, es);
+	opened_group = true;
+}
+ExplainOpenGroup("Worker", NULL, true, es);
+ExplainPropertyInteger("Worker Number", n, es);
+ExplainPropertyText("Sort Method", sortMethod, es);
+ExplainPropertyLong("Sort Space Used", spaceUsed, es);
+ExplainPropertyText("Sort Space Type", spaceType, es);
+ExplainCloseGroup("Worker", NULL, true, es);
+			}
+		}
+		if (opened_group)
+			ExplainCloseGroup("Workers", "Workers", false, es);
+	}
 }
 
 /*
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index ad9eba6..01316ff 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -28,9 +28,10 @@
 #include "executor/nodeBitmapHeapscan.h"
 #include "executor/nodeCustom.h"
 #include "executor/nodeForeignscan.h"
-#include "executor/nodeSeqscan.h"
 #include "executor/nodeIndexscan.h"
 #include "executor/nodeIndexonlyscan.h"
+#include "executor/nodeSeqscan.h"
+#include "executor/nodeSort.h"
 #include "executor/tqueue.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/planmain.h"
@@ -202,10 +203,10 @@ ExecSerializePlan(Plan *plan, EState *estate)
 }
 
 /*
- * Ordinary plan nodes won't do anything here, but parallel-aware plan nodes
- * may need some state which is shared across all parallel workers.  Before
- * we size the DSM, give them a chance to call shm_toc_estimate_chunk or
- * shm_toc_estimate_keys on >estimator.
+ * Parallel-aware plan nodes (and occasionally others) may need some state
+ * which is shared across all parallel workers.  Before we size the DSM, give
+ * them a chance to call shm_toc_estimate_chunk or shm_toc_estimate_keys on
+ * >estimator.
  *
  * While we're at it, count the number of PlanState nodes in the tree, so
  * we know how many SharedPlanStateInstrumentation structures we need.
@@ -219,38 +220,43 @@ ExecParallelEstimate(PlanState *planstate, ExecParallelEstimateContext *e)
 	/* Count this node. */
 	

Re: [HACKERS] Variable substitution in psql backtick expansion

[Robert Haas]

On Sun, May 21, 2017 at 11:37 AM, Fabien COELHO  wrote:
>> This patch has trivial implementation - and there are not any objection to
>> used new variable names.
>>
>> 1. there was not any problem with patching, compiling
>> 2. all regress tests passed
>> 3. no problems with doc build
>>
>> I'll mark this patch as ready for commiters
>
> Thanks for the review.

I am attempting to understand the status of this patch.  It looks like
the patch that was the original subject of this thread was committed
as f833c847b8fa4782efab45c8371d3cee64292d9b on April 1st by Tom, who
was its author. Subsequently, a new patch not obviously related to the
subject line was proposed by Fabien Coelho, and that patch was
subsequently marked Ready for Committer by Pavel Stehule.  Meanwhile,
objections were raised by Tom, who seems to think that we should make
\if accept an expression language before we consider this change.
AFAICT, there's no patch for that.

Personally, I have mixed feelings about Tom's objection.  On the one
hand, it seems a bit petty to reject this patch on the grounds that
the marginally-related feature of having \if accept an expression
doesn't exist yet.  It's hard enough to get things into PostgreSQL
already; we shouldn't make it harder without a very fine reason.  On
the other hand, given that there is no client-side expression language
yet, the only way to use this seems to be to send a query to the
server, and if you're going to do that, you may as well use select
current_setting('server_version_num') instead of referencing a psql
variable containing the same value.  Maybe the client version number
has some use, though; I think that's not otherwise available right
now.

Some comments on the patch itself:

- Documentation needs attention from a native English speaker.
- Is it a good idea/practical to prevent the new variables from being
modified by the user?
- I think Pavel's upthread suggestion of prefixing the client
variables with "client" to match the way the server variables are
named is a good idea.

I guess the bottom line is that I'm willing to fix this up and commit
it if we've got consensus on it, but I read this thread as Fabien and
Pavel voting +1 on this patch, Tom as voting -1, and a bunch of other
people (including me) not taking a strong position.  In my view,
that's not enough consensus to proceed.  Anybody else want to vote, or
change their vote?

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


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

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

[David Steele]

On 8/25/17 3:26 PM, Robert Haas wrote:

On Fri, Aug 25, 2017 at 3:21 PM, David Steele  wrote:

No problem.  I'll base it on your commit to capture any changes you made.


Thanks, but you incorporated everything I wanted in response to my
first review -- so I didn't tweak it any further.


Thank you for committing that.  I'll get the 9.6 patch to you early next 
week.


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


--
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] MAIN, Uncompressed?

[Tom Lane]

Greg Stark  writes:
> I think this is a particularly old piece of code and we're lucky the
> default heuristics have served well for all this time because I doubt
> many people fiddle with these storage attributes. The time may have
> come to come up with a better UI for the storage attributes because
> people are doing new things (like json) and wanting more control over
> this heuristic.

Yeah, I could get behind a basic rethinking of the tuptoaster control
knobs.  I'm just not in love with Simon's specific proposal, especially
not if we can't think of a better name for it than "MAINU".

> For what it's worth I think a good start would be to give people more
> visibility into what the tuptoaster heuristic is actually doing to
> their data and that will encourage people to give feedback about when
> they're surprised and are frustrated by the existing UI.

Hm, what might that look like exactly?  More pgstattuple functionality
perhaps?

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] MAIN, Uncompressed?

[Greg Stark]

On 25 August 2017 at 19:59, Simon Riggs  wrote:
>
> On 25 August 2017 at 14:08, Tom Lane  wrote:
>
> > Maybe, but the use case seems mighty narrow.
>
> JSON blobs between 2kB and 8160 bytes are very common.
>
> String length is maybe a poisson distribution, definitely not uniform.


But JSON blobs should be highly compressible. Even jsonb will be quite
compressible.

That said I always found remembering the mapping from these names to
various behaviours to be quite hard to use. I would have found it far
more useful to have two separate properties I could set "compress" and
"external" or perhaps even more useful would be to set some kind of
guideline size threshold for each (and perhaps a second size compress
threshold and external threshold for the whole tuple).

I think this is a particularly old piece of code and we're lucky the
default heuristics have served well for all this time because I doubt
many people fiddle with these storage attributes. The time may have
come to come up with a better UI for the storage attributes because
people are doing new things (like json) and wanting more control over
this heuristic.

For what it's worth I think a good start would be to give people more
visibility into what the tuptoaster heuristic is actually doing to
their data and that will encourage people to give feedback about when
they're surprised and are frustrated by the existing UI.


-- 
greg


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

[HACKERS] Release plans: near-term update releases, and 10.0

[Tom Lane]

There are a couple of embarrassing regressions in this month's minor
releases:

* parallel pg_restore crashes with -L (cf commit b1c2d76a2); affects
  9.3 and up

* walsender with a connected client won't shut down (cf commit b51c8efc6);
  affects 9.4 only

Given that these bugs were discovered so quickly after release, they're
probably a significant problem for many users.  Accordingly, the PG
release team has decided that we ought to do a new set of back-branch
releases ASAP to fix them.  We plan to wrap these releases Monday (8/28)
for announcement Thursday (8/31).

There are some other known bugs that we probably won't have time to
get fixed before next week, but those are much older issues and
accordingly seem less pressing.

In other news, the release team has also set planned dates for
proceeding to 10.0 official release:

10beta4: wrap 8/28 announce 8/31 (with the back-branch updates)
10rc1:   wrap 9/11 announce 9/14
10.0:wrap 9/25 announce 9/28

Those are subject to change if any really nasty bug emerges,
but right now we're feeling pretty good about the v10 branch.

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: add --if-exists to pg_recvlogical

[Robert Haas]

On Sun, May 21, 2017 at 3:04 PM, Rosser Schwarz
 wrote:
> While doing some scripting around pg_recvlogical at $work, I found a need
> for $subject. Attached, find a patch to that effect.
>
> I tried simply to mirror the logic used elsewhere. I don't think there's
> anything controversial here, but welcome any comments or suggestions.
>
> This applies and builds successfully against master, and behaves as designed
> (i.e., dropping or trying to stream from a nonexistent slot exits cleanly).
> It doesn't affect any other behavior I could observe.
>
> If accepted, this will likely need a pgindent run upon merging; I had to
> give up on the rabbit hole of getting that working locally.

Please add this to commitfest.postgresql.org

-- 
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] Crash on promotion when recovery.conf is renamed

[Robert Haas]

On Sat, Apr 8, 2017 at 10:05 AM, David Steele  wrote:
> On 3/28/17 1:21 AM, Tsunakawa, Takayuki wrote:
>> From: Michael Paquier [mailto:michael.paqu...@gmail.com]
>>> Okay. I got the message, and I agree with what you say here. You are right
>>> by the way, the error messages just use "two-phase file" and not "two-phase
>>> STATE file", missed that previously.
>>> --
>> Thanks, marked as ready for committer, as the code is good and Alexander 
>> reported the test success.
>
> This bug has been moved to CF 2017-07.

This bug fix has been pending in "Ready for Committer" state for about
4.5 months.  Three committers (Magnus, Heikki, Tom) have contributed
to the thread to date.  Maybe one of them would like to commit this?

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


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

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

[Robert Haas]

On Fri, Aug 25, 2017 at 3:21 PM, David Steele  wrote:
> No problem.  I'll base it on your commit to capture any changes you made.

Thanks, but you incorporated everything I wanted in response to my
first review -- so I didn't tweak it any further.

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


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

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

[David Steele]

On 8/25/17 3:13 PM, Robert Haas wrote:
> On Fri, Aug 25, 2017 at 3:10 PM, David Steele  wrote:
>>
>> Robert said he would commit this so I expect he'll do that if he doesn't
>> have any objections to the changes.
>>
>> Robert, if you would prefer me to submit this to the CF I'm happy to do so.
> 
> Ha, this note arrived just as I was working on getting this committed.
> I'll commit this to 11 and 10 presently; can you produce a version for
> 9.6?

No problem.  I'll base it on your commit to capture any changes you made.

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


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

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

[Robert Haas]

On Fri, Aug 25, 2017 at 3:10 PM, David Steele  wrote:
> On 8/24/17 7:36 PM, Michael Paquier wrote:
>>
>> True as well. The patch looks good to me. If a committer does not show
>> up soon, it may be better to register that in the CF and wait. I am
>> not sure that adding an open item is suited, as docs have the same
>> problem on 9.6.
>
> Robert said he would commit this so I expect he'll do that if he doesn't
> have any objections to the changes.
>
> Robert, if you would prefer me to submit this to the CF I'm happy to do so.

Ha, this note arrived just as I was working on getting this committed.
I'll commit this to 11 and 10 presently; can you produce a version for
9.6?

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


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

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

[David Steele]

On 8/24/17 7:36 PM, Michael Paquier wrote:
> 
> True as well. The patch looks good to me. If a committer does not show
> up soon, it may be better to register that in the CF and wait. I am
> not sure that adding an open item is suited, as docs have the same
> problem on 9.6.

Robert said he would commit this so I expect he'll do that if he doesn't
have any objections to the changes.

Robert, if you would prefer me to submit this to the CF I'm happy to do so.

Thanks,
-- 
-David
da...@pgmasters.net


-- 
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 Fri, Aug 25, 2017 at 2:54 PM, Tom Lane  wrote:
>> I think moving SharedSortInfo into tuplesort.h would be a gross
>> abstraction violation, but moving SortInstrumentation into tuplesort.h
>> seems like a modest improvement.
>
> Hmm, I'm not sure why SortInstrumentation belongs naturally to
> tuplesort.h but putting an array of them there would be a "gross
> abstraction violation".  Perhaps it would help to rename
> struct SharedSortInfo to SortInstrumentationArray, and change its
> field names to be less specific to the parallel-worker use case?

What other use case could there be?  I think an array of
SortInstrumentation objects intended to be stored in DSM is fairly
clearly a bit of executor-specific machinery and thus properly
declared along with the node that contains it.  Only nodeSort.c and
explain.c need to know anything about it; tuplesort.c is totally
ignorant of it and I see no reason why we'd ever want to change that.
Indeed, I don't quite see how we could do so without some kind of
abstraction violation.

SortInstrumentation is a diffferent kettle of fish.  I chose to make
that an executor-specific bit as well, but an advantage of your
proposed approach is that future additions to (or changes in) what
gets returned by tuplesort_get_stats() wouldn't require as much
tweaking elsewhere.  With your proposed change, nodeSort.c wouldn't
really care about the contents of that structure (as long as there are
no embedded pointers), so we could add new members or rename things
and only tuplesort.c and explain.c would need updating.

-- 
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] MAIN, Uncompressed?

[Simon Riggs]

On 25 August 2017 at 14:08, Tom Lane  wrote:
> Simon Riggs  writes:
>> On 25 August 2017 at 13:21, Tom Lane  wrote:
>>> If you know compression isn't useful, but you don't want to fail on
>>> wide values, then "external" should serve the purpose.
>
>> Well, almost. External toasts at 2048-ish bytes whereas Main toasts at
>> 8160 bytes.
>> The rows are typically near 4kB long, so if marked External they would
>> always be toasted.
>> It's desirable to have the full row in the heap block, rather than
>> have to access heap-toastindex-toastblocks in all cases.
>> The data is also incompressible, so Main just wastes time on insert.
>> Hence, we have a missing option.
>
> Maybe, but the use case seems mighty narrow.

JSON blobs between 2kB and 8160 bytes are very common.

String length is maybe a poisson distribution, definitely not uniform.

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

[Tom Lane]

Robert Haas  writes:
> On Fri, Aug 25, 2017 at 2:01 PM, Tom Lane  wrote:
>> I looked through this a little, and feel uncomfortable with the division
>> of typedefs between execnodes.h and tuplesort.h.  I'm inclined to push
>> struct SortInstrumentation, and maybe also SharedSortInfo, into
>> tuplesort.h.

> I think moving SharedSortInfo into tuplesort.h would be a gross
> abstraction violation, but moving SortInstrumentation into tuplesort.h
> seems like a modest improvement.

Hmm, I'm not sure why SortInstrumentation belongs naturally to
tuplesort.h but putting an array of them there would be a "gross
abstraction violation".  Perhaps it would help to rename
struct SharedSortInfo to SortInstrumentationArray, and change its
field names to be less specific to the parallel-worker use case?

>> (BTW, would it make sense to number the workers from 1 not 0 in the
>> EXPLAIN printout?)

> ... So I'm in favor of leaving it alone; I don't think that 0-based
> indexing is such an obscure convention that it will flummox users.

OK, I'm not particularly set on that.

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

[Robert Haas]

On Fri, Aug 25, 2017 at 2:01 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On another note, here's a second patch applying on top of my earlier
>> patch to push down Limit through Gather and Gather Merge.
>
> I looked through this a little, and feel uncomfortable with the division
> of typedefs between execnodes.h and tuplesort.h.  I'm inclined to push
> struct SortInstrumentation, and maybe also SharedSortInfo, into
> tuplesort.h.  Then tuplesort_get_stats could be given the signature
>
> void tuplesort_get_stats(Tuplesortstate *state,
>  SortInstrumentation *stats);
>
> which seems less messy.  Thoughts?

I think moving SharedSortInfo into tuplesort.h would be a gross
abstraction violation, but moving SortInstrumentation into tuplesort.h
seems like a modest improvement.

> I'm also pretty suspicious of the filter code you added to subselect.sql.
> What happens when there's more than one worker?

Uh, then somebody's changed the definition of force_parallel_mode in a
really strange way.

> (BTW, would it make sense to number the workers from 1 not 0 in the
> EXPLAIN printout?)

The main problem I see with that is that it might confuse somebody who
is debugging.  If you've got a backend and print out
ParallelWorkerNumber, you might expect that to match what you see in
the EXPLAIN output.  It also seems likely to me that other parts of
EXPLAIN or other parts of the system generally will eventually expose
parallel worker numbers in mildly user-visible ways (e.g. maybe it'll
show up in a DEBUG message someplace eventually), and if we insist on
sticking a + 1 into EXPLAIN's output in the existing places we'll have
to do it everywhere else, and somebody's eventually going to forget.
So I'm in favor of leaving it alone; I don't think that 0-based
indexing is such an obscure convention that it will flummox users.

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

[Robert Haas]

On Mon, Aug 21, 2017 at 2:10 AM, Amit Langote
 wrote:
> [ new patches ]

I am failing to understand the point of separating PartitionDispatch
into PartitionDispatch and PartitionTableInfo.  That seems like an
unnecessary multiplication of entities, as does the introduction of
PartitionKeyInfo.  I also think that replacing reldesc with reloid is
not really an improvement; any places that gets the relid has to go
open the relation to get the reldesc, whereas without that it has a
direct pointer to the information it needs.

I suggest that this patch just focus on removing the following things
from PartitionDispatchData: keystate, tupslot, tupmap.  Those things
are clearly executor-specific stuff that makes sense to move to a
different structure, what you're calling PartitionTupleRoutingInfo
(not sure that's the best name).  The other stuff all seems fine.
You're going to have to open the relation anyway, so keeping the
reldesc around seems like an optimization, if anything.  The
PartitionKey and PartitionDesc pointers may not really be needed --
they're just pointers into reldesc -- but they're trivial to compute,
so it doesn't hurt anything to have them either as a
micro-optimization for performance or even just for readability.

That just leaves indexes.  In a world where keystate, tupslot, and
tupmap are removed from the PartitionDispatchData, you must need
indexes or there would be no point in constructing a
PartitionDispatchData object in the first place; any application that
needs neither indexes nor the executor-specific stuff could just use
the Relation directly.

Regarding your XXX comments, note that if you've got a lock on a
relation, the pointers to the PartitionKey and PartitionDesc are
stable.  The PartitionKey can't change once it's established, and the
PartitionDesc can't change while we've got a lock on the relation
unless we change it ourselves (and any places that do should have
CheckTableNotInUse checks).  The keep_partkey and keep_partdesc
handling in relcache.c exists exactly so that we can guarantee that
the pointer won't go stale under us.  Now, if we *don't* have a lock
on the relation, then those pointers can easily be invalidated -- so
you can't hang onto a PartitionDispatch for longer than you hang onto
the lock on the Relation.  But that shouldn't be a problem.  I think
you only need to hang onto PartitionDispatch pointers for the lifetime
of a single query.  One can imagine optimizations where we try to
avoid rebuilding that for subsequent queries but I'm not sure there's
any demonstrated need for such a system at present.

-- 
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] ECPG: WHENEVER statement with DO CONTINUE action

[Tom Lane]

Christian Ullrich  writes:
> The buildfarm says that sorting is frequently done case-sensitively:

Given that it's Friday evening in Europe, I'm betting Michael is gone
for the day.  In the interests of getting the buildfarm back to green,
I'll see if I can fix this.

regards, tom lane


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

Re: [HACKERS] [PATCH] Push limit to sort through a subquery

[Tom Lane]

Robert Haas  writes:
> On another note, here's a second patch applying on top of my earlier
> patch to push down Limit through Gather and Gather Merge.

I looked through this a little, and feel uncomfortable with the division
of typedefs between execnodes.h and tuplesort.h.  I'm inclined to push
struct SortInstrumentation, and maybe also SharedSortInfo, into
tuplesort.h.  Then tuplesort_get_stats could be given the signature

void tuplesort_get_stats(Tuplesortstate *state,
 SortInstrumentation *stats);

which seems less messy.  Thoughts?

I'm also pretty suspicious of the filter code you added to subselect.sql.
What happens when there's more than one worker?

(BTW, would it make sense to number the workers from 1 not 0 in the
EXPLAIN printout?)

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

[Robert Haas]

On Fri, Aug 25, 2017 at 1:31 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> Awesome.  What's the business with enable_hashagg in the regression test 
>> stuff?
>
> Just relocating that "reset" so that it only affects the test case it's
> needed for.  (I think the 0-worker test was inserted in the wrong place.)

Oh, hmm.  Well, no objection to that, I guess.  Thanks for working on this.

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

[Tom Lane]

Robert Haas  writes:
> Awesome.  What's the business with enable_hashagg in the regression test 
> stuff?

Just relocating that "reset" so that it only affects the test case it's
needed for.  (I think the 0-worker test was inserted in the wrong place.)

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] Update comment in ExecPartitionCheck

[Robert Haas]

On Tue, Jul 4, 2017 at 4:55 AM, Etsuro Fujita
 wrote:
> This comment in an error handling in ExecPartitionCheck():
>
> if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
> {
> char   *val_desc;
> Relationorig_rel = rel;
>
> /* See the comment above. */
> if (resultRelInfo->ri_PartitionRoot)
>
> should be updated because we don't have any comment on that above in the
> code.  Since we have a comment on that in ExecConstraints() defined just
> below that function, I think the comment should be something like this: "See
> the comment in ExecConstraints().".  Attached is a patch for that.

Hrm.  I'm not sure I understand which comment in ExecConstraints()
this is supposed to refer to.  Maybe we need to think a bit harder
about how to make this clear.

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


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

Re: [HACKERS] why not parallel seq scan for slow functions

[Tom Lane]

Robert Haas  writes:
> The point I was trying to make is that if you retroactively change the
> cost of a path after you've already done add_path(), it's too late to
> change your mind about whether to keep the path.  At least according
> to my current understanding, that's the root of this problem in the
> first place.  On top of that, add_path() and other routines that deal
> with RelOptInfo path lists expect surviving paths to be ordered by
> descending cost; if you frob the cost, they might not be properly
> ordered any more.

Hadn't been paying attention to this thread, but I happened to notice
Robert's comment here, and I strongly agree: it is *not* cool to be
changing a path's cost (or, really, anything else about it) after
it's already been given to add_path.  add_path has already made
irreversible choices on the basis of what it was given.

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

[Robert Haas]

On Fri, Aug 25, 2017 at 1:18 PM, Tom Lane  wrote:
> v4, with a test case and some more comment-polishing.  I think this
> is committable.

Awesome.  What's the business with enable_hashagg in the regression test stuff?

-- 
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] Range Merge Join v1

[Alexander Kuzmenkov]

Hi Jeff,

Fast range joins are very nice to have, thank you for working on this.

I've been doing a somewhat related thing recently, trying to teach the 
merge join executor to perform full joins on comparison clauses [1]. I 
have some comments after reading your patch:


* At the moment, "mergejoinable clause" and "equality clause" mean the 
same thing to the planner, and those clauses are used both to create 
equivalence classes and to perform merge joins. If we start performing 
merge joins for different kinds of clauses, such as comparison or range 
intersection, it makes sense to separate these two meanings. I tried 
this in my patch and it didn't require many changes. I use 
RestrictInfo.equivopfamilies to build equivalence classes, and 
RestrictInfo.mergeopfamilies for mergejoinable clauses.


* Semantically, MJCompare() is a function that determines whether you 
should emit a join result or advance one of the pointers. This meaning 
is not explicit in the code and is not well encapsulated: the function 
returns and int and 'compareNoMatch' flag, and the calling code combines 
them in various ways to derive the final result. This meaning can be 
made explicit by making MJCompare return enum values {Join, NextInner, 
NextOuter}, and putting inside it all the logic that decides whether we 
join or advance. ExecMergeJoin looks intimidating already, and I think 
this change would help readability. Again, you can find an illustration 
in my patch.


I hope you find these points helpful.

[1] 
https://www.postgresql.org/message-id/flat/1d23ad41-a9d9-1d1e-1d79-83b002d6f776%40postgrespro.ru#1d23ad41-a9d9-1d1e-1d79-83b002d6f...@postgrespro.ru


--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



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

Re: [HACKERS] [PATCH] Push limit to sort through a subquery

[Tom Lane]

v4, with a test case and some more comment-polishing.  I think this
is committable.

regards, tom lane

diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index ce47f1d..ad9eba6 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -47,17 +47,26 @@
  * greater than any 32-bit integer here so that values < 2^32 can be used
  * by individual parallel nodes to store their own state.
  */
-#define PARALLEL_KEY_PLANNEDSTMT		UINT64CONST(0xE001)
-#define PARALLEL_KEY_PARAMSUINT64CONST(0xE002)
-#define PARALLEL_KEY_BUFFER_USAGE		UINT64CONST(0xE003)
-#define PARALLEL_KEY_TUPLE_QUEUE		UINT64CONST(0xE004)
-#define PARALLEL_KEY_INSTRUMENTATION	UINT64CONST(0xE005)
-#define PARALLEL_KEY_DSAUINT64CONST(0xE006)
-#define PARALLEL_KEY_QUERY_TEXT		UINT64CONST(0xE007)
+#define PARALLEL_KEY_EXECUTOR_FIXED		UINT64CONST(0xE001)
+#define PARALLEL_KEY_PLANNEDSTMT		UINT64CONST(0xE002)
+#define PARALLEL_KEY_PARAMSUINT64CONST(0xE003)
+#define PARALLEL_KEY_BUFFER_USAGE		UINT64CONST(0xE004)
+#define PARALLEL_KEY_TUPLE_QUEUE		UINT64CONST(0xE005)
+#define PARALLEL_KEY_INSTRUMENTATION	UINT64CONST(0xE006)
+#define PARALLEL_KEY_DSAUINT64CONST(0xE007)
+#define PARALLEL_KEY_QUERY_TEXT		UINT64CONST(0xE008)
 
 #define PARALLEL_TUPLE_QUEUE_SIZE		65536
 
 /*
+ * Fixed-size random stuff that we need to pass to parallel workers.
+ */
+typedef struct FixedParallelExecutorState
+{
+	int64		tuples_needed;	/* tuple bound, see ExecSetTupleBound */
+} FixedParallelExecutorState;
+
+/*
  * DSM structure for accumulating per-PlanState instrumentation.
  *
  * instrument_options: Same meaning here as in instrument.c.
@@ -381,12 +390,14 @@ ExecParallelReinitialize(ParallelExecutorInfo *pei)
  * execution and return results to the main backend.
  */
 ParallelExecutorInfo *
-ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers)
+ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers,
+	 int64 tuples_needed)
 {
 	ParallelExecutorInfo *pei;
 	ParallelContext *pcxt;
 	ExecParallelEstimateContext e;
 	ExecParallelInitializeDSMContext d;
+	FixedParallelExecutorState *fpes;
 	char	   *pstmt_data;
 	char	   *pstmt_space;
 	char	   *param_space;
@@ -418,6 +429,11 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers)
 	 * for the various things we need to store.
 	 */
 
+	/* Estimate space for fixed-size state. */
+	shm_toc_estimate_chunk(>estimator,
+		   sizeof(FixedParallelExecutorState));
+	shm_toc_estimate_keys(>estimator, 1);
+
 	/* Estimate space for query text. */
 	query_len = strlen(estate->es_sourceText);
 	shm_toc_estimate_chunk(>estimator, query_len);
@@ -487,6 +503,11 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers)
 	 * asked for has been allocated or initialized yet, though, so do that.
 	 */
 
+	/* Store fixed-size state. */
+	fpes = shm_toc_allocate(pcxt->toc, sizeof(FixedParallelExecutorState));
+	fpes->tuples_needed = tuples_needed;
+	shm_toc_insert(pcxt->toc, PARALLEL_KEY_EXECUTOR_FIXED, fpes);
+
 	/* Store query string */
 	query_string = shm_toc_allocate(pcxt->toc, query_len);
 	memcpy(query_string, estate->es_sourceText, query_len);
@@ -833,6 +854,7 @@ ExecParallelInitializeWorker(PlanState *planstate, shm_toc *toc)
 void
 ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
 {
+	FixedParallelExecutorState *fpes;
 	BufferUsage *buffer_usage;
 	DestReceiver *receiver;
 	QueryDesc  *queryDesc;
@@ -841,6 +863,9 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
 	void	   *area_space;
 	dsa_area   *area;
 
+	/* Get fixed-size state. */
+	fpes = shm_toc_lookup(toc, PARALLEL_KEY_EXECUTOR_FIXED, false);
+
 	/* Set up DestReceiver, SharedExecutorInstrumentation, and QueryDesc. */
 	receiver = ExecParallelGetReceiver(seg, toc);
 	instrumentation = shm_toc_lookup(toc, PARALLEL_KEY_INSTRUMENTATION, true);
@@ -868,8 +893,17 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
 	queryDesc->planstate->state->es_query_dsa = area;
 	ExecParallelInitializeWorker(queryDesc->planstate, toc);
 
-	/* Run the plan */
-	ExecutorRun(queryDesc, ForwardScanDirection, 0L, true);
+	/* Pass down any tuple bound */
+	ExecSetTupleBound(fpes->tuples_needed, queryDesc->planstate);
+
+	/*
+	 * Run the plan.  If we specified a tuple bound, be careful not to demand
+	 * more tuples than that.
+	 */
+	ExecutorRun(queryDesc,
+ForwardScanDirection,
+fpes->tuples_needed < 0 ? (int64) 0 : fpes->tuples_needed,
+true);
 
 	/* Shut down the executor */
 	ExecutorFinish(queryDesc);
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index 36d2914..c1aa506 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -757,3 

Re: [HACKERS] paths in partitions of a dummy partitioned table

[Robert Haas]

On Thu, Jul 6, 2017 at 11:35 AM, Ashutosh Bapat
 wrote:
> If a partitioned table is proven dummy, set_rel_pathlist() doesn't mark the
> partition relations dummy and thus doesn't set any (dummy) paths in the
> partition relations. The lack of paths in the partitions means that we can
> not use partition-wise join while joining this table with some other similarly
> partitioned table as the partitions do not have any paths that child-joins can
> use. This means that we can not create partition relations for such a join and
> thus can not consider the join to be partitioned. This doesn't matter much 
> when
> the dummy relation is the outer relation, since the resultant join is also
> dummy. But when the dummy relation is inner relation, the resultant join is 
> not
> dummy and can be considered to be partitioned with same partitioning scheme as
> the outer relation to be joined with other similarly partitioned table. Not
> having paths in the partitions deprives us of this future optimization.

I think it's wrong for any code to be examining the path list for a
rel marked dummy, so I would suggest approaching this from a different
direction.  Given A LEFT JOIN B where Bk is dummy, I suggest
constructing the path for (AB)k by taking a path from Ak and applying
an appropriate PathTarget.  You don't really need a join path at all;
a path for the non-dummy input is fine - and, in fact, better, since
it will be cheaper to execute.  One problem is that it may not produce
the correct output columns.  (AB) may drop some columns that were
being generated by A because they were only needed to perform the
join, and it may add additional columns taken from B.  But I think
that if you take the default PathTarget for (AB) and replace
references to columns of B with NULL constants, you should be able to
apply the resulting PathTarget to a path for Ak and get a valid path
for (AB)k.  Maybe there is some reason why that won't work exactly,
but I think it is probably more promising to attack the problem from
this angle than to do what you propose.  Sticking dummy joins into the
query plan that are really just projecting out NULLs is not appealing.

-- 
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] [RFC] What would be difficult to make data models pluggable for making PostgreSQL a multi-model database?

[Henry M]

This may be interesting... they implement cypher (unfortunately they had to
fork in order to have cypher be a first class query language with SQL).

https://github.com/bitnine-oss/agensgraph



On Mon, Aug 21, 2017 at 12:44 AM Chris Travers 
wrote:

> On Sun, Aug 20, 2017 at 4:10 AM, MauMau  wrote:
>
>> From: Chris Travers
>> > Why cannot you do all this in a language handler and treat as a user
>> defined function?
>> > ...
>> > If you have a language handler for cypher, why do you need in_region
>> or cast_region?  Why not just have a graph_search() function which
>> takes in a cypher query and returns a set of records?
>>
>> The language handler is for *stored* functions.  The user-defined
>> function (UDF) doesn't participate in the planning of the outer
>> (top-level) query.  And they both assume that they are executed in SQL
>> commands.
>>
>
> Sure but stored functions can take arguments, such as a query string which
> gets handled by the language handler.  There's absolutely no reason you
> cannot declare a function in C that takes in a Cypher query and returns a
> set of tuples.   And you can do a whole lot with preloaded shared libraries
> if you need to.
>
> The planning bit is more difficult, but see below as to where I see major
> limits here.
>
>>
>> I want the data models to meet these:
>>
>> 1) The query language can be used as a top-level session language.
>> For example, if an app specifies "region=cypher_graph" at database
>> connection, it can use the database as a graph database and submit
>> Cypher queries without embedding them in SQL.
>>
>
> That sounds like a foot gun.  I would probably think of those cases as
> being ideal for a custom background worker, similar to Mongress.
> Expecting to be able to switch query languages on the fly strikes me as
> adding totally needless complexity everywhere to be honest.  Having
> different listeners on different ports simplifies this a lot and having,
> additionally, query languages for ad-hoc mixing via language handlers might
> be able to get most of what you want already.
>
>>
>> 2) When a query contains multiple query fragments of different data
>> models, all those fragments are parsed and planned before execution.
>> The planner comes up with the best plan, crossing the data model
>> boundary.  To take the query example in my first mail, which joins a
>> relational table and the result of a graph query.  The relational
>> planner considers how to scan the table, the graph planner considers
>> how to search the graph, and the relational planner considers how to
>> join the two fragments.
>>
>
> It seems like all you really need is a planner hook for user defined
> languages (I.e. "how many rows does this function return with these
> parameters" right?).  Right now we allow hints but they are static.  I
> wonder how hard this would be using preloaded, shared libraries.
>
>
>>
>> So in_region() and cast_region() are not functions to be executed
>> during execution phase, but are syntax constructs that are converted,
>> during analysis phase, into calls to another region's parser/analyzer
>> and an inter-model cast routine.
>>
>
> So basically they work like immutable functions except that you cannot
> index the output?
>
>>
>> 1. The relational parser finds in_region('cypher_graph', 'graph
>> query') and produces a parse node InRegion(region_name, query) in the
>> parse tree.
>>
>> 2. The relational analyzer looks up the system catalog to checks if
>> the specified region exists, then calls its parser/analyzer to produce
>>
> the query tree for the graph query fragment.  The relational analyser
>
>
>> attaches the graph query tree to the InRegion node.
>>
>> 3. When the relational planner finds the graph query tree, it passes
>> the graph query tree to the graph planner to produce the graph
>> execution plan.
>>
>> 4. The relational planner produces a join plan node, based on the
>> costs/statistics of the relational table scan and graph query.  The
>> graph execution plan is attached to the join plan node.
>>
>> The parse/query/plan nodes have a label to denote a region, so that
>> appropriate region's routines can be called.
>>
>
> It would be interesting to see how much of what you want you can get with
> what we currently have and what pieces are really missing.
>
> Am I right that if you wrote a function in C to take a Cypher query plan,
> and analyse it, and execute it, the only thing really missing would be
> feedback to the PostgreSQL planner regarding number of rows expected?
>
>>
>> Regards
>> MauMau
>>
>>
>
>
> --
> Best Regards,
> Chris Travers
> Database Administrator
>
> Tel: +49 162 9037 210 <+49%20162%209037210> | Skype: einhverfr |
> www.adjust.com
> Saarbrücker Straße 37a, 10405 Berlin
>
>

Re: [HACKERS] subscription worker signalling wal writer too much

[Robert Haas]

On Mon, Jul 3, 2017 at 11:26 PM, Jeff Janes  wrote:
> My understanding is that you can't start on a new file until the old file is
> completely synced, because the book keeping can currently only handle one
> file at a time.  So if you signal the wal writer to do the sync, you would
> just end up immediately blocking on it to finish.  Getting around that would
> require substantially more changes; we would need to implement a queue of
> full log files not yet synced so that we could move on without waiting.

Yeah.  I bet that effort would be reasonably worthwhile.  I believe
that the forced end-of-segment syncs are costing us a noticeable
amount of performance.  Once or twice I took a very half-hearted run
at doing what you describe here, but gave up pretty quickly; it seems
like a somewhat tricky problem.

-- 
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] assorted code cleanup

[Robert Haas]

On Mon, Aug 21, 2017 at 1:11 AM, Michael Paquier
 wrote:
> On Fri, Aug 18, 2017 at 1:56 AM, Peter Eisentraut
>  wrote:
>> Here are a few assorted patches I made while working on the stdbool set,
>> cleaning up various pieces of dead code and weird styles.
>>
>> - Drop excessive dereferencing of function pointers
>
> -   (*next_ProcessUtility_hook) (pstmt, queryString,
> +   next_ProcessUtility_hook(pstmt, queryString,
>  context, params, queryEnv,
>  dest, completionTag);
> But this... Personally I like the current grammar which allows one to
> make the difference between a function call with something declared
> locally and something that may be going to a custom code path.

+1.

-- 
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] locale problem of bgworker: logical replication launcher and worker process

[Peter Eisentraut]

On 8/25/17 05:14, Ioseph Kim wrote:
> I resolved this problem.
> 
> This problem is that dgettext() function use codeset of database's lc_ctype.
> 
> below database's lc_ctype is C, but locale is ko_KR.UTF8.

Thank you for following up.  As you have discovered, that combination of
locale settings doesn't work well.

-- 
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] obsolete code in pg_upgrade

[Peter Eisentraut]

On 8/25/17 04:00, Christoph Berg wrote:
> Re: Peter Eisentraut 2017-08-24 
> <8aa00f15-144e-e793-750e-d1d6876d6...@2ndquadrant.com>
>> On 8/23/17 09:36, Robert Haas wrote:
>>> I think I agree.  It seems to me that the version of pg_upgrade
>>> shipped with release N only needs to support upgrades to release N,
>>> not older releases.  There's probably room for debate about whether a
>>> pg_upgrade needs to support direct upgrades FROM very old releases,
>>> but we need not decide anything about that right now.
>>>
>>> I think you could go ahead and rip out this code, but as it seems like
>>> a non-critical change, -1 for back-patching it.
>>
>> committed to PG10 and master
> 
> This #define seems to be obsolete now as well:
> src/bin/pg_upgrade/pg_upgrade.h:#define TABLE_SPACE_SUBDIRS_CAT_VER 20100

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

[Robert Haas]

On Sun, Aug 20, 2017 at 11:25 PM, Etsuro Fujita
 wrote:
>> I agree, but I wonder if we ought to make it work first using the
>> existing APIs and then add these new APIs as an optimization.
>
> I'm not sure that's a good idea because that once we support INSERT
> tuple-routing for foreign partitions, we would have a workaround: INSERT
> INTO partitioned_table SELECT * from data_table where data_table is a
> foreign table defined for an input file using file_fdw.

That's true, but I don't see how it refutes the point I was trying to raise.

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


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

Re: [HACKERS] PoC: full merge join on comparison clause

[Alexander Kuzmenkov]

Here is a new version of the patch, rebased to 749c7c41 and with some 
cosmetic changes.


--
Alexander Kuzmenkov
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 d77c2a70e4..19bc90aa32 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -722,19 +722,19 @@ get_useful_ecs_for_relation(PlannerInfo *root, RelOptInfo *rel)
 	{
 		RestrictInfo *restrictinfo = (RestrictInfo *) lfirst(lc);
 
-		/* Consider only mergejoinable clauses */
-		if (restrictinfo->mergeopfamilies == NIL)
+		/* Consider only mergejoinable equality clauses */
+		if (restrictinfo->equivopfamilies == NIL)
 			continue;
 
 		/* Make sure we've got canonical ECs. */
-		update_mergeclause_eclasses(root, restrictinfo);
+		update_equivclause_eclasses(root, restrictinfo);
 
 		/*
-		 * restrictinfo->mergeopfamilies != NIL is sufficient to guarantee
+		 * restrictinfo->equivopfamilies != NIL is sufficient to guarantee
 		 * that left_ec and right_ec will be initialized, per comments in
 		 * distribute_qual_to_rels.
 		 *
-		 * We want to identify which side of this merge-joinable clause
+		 * We want to identify which side of this merge-joinable equality clause
 		 * contains columns from the relation produced by this RelOptInfo. We
 		 * test for overlap, not containment, because there could be extra
 		 * relations on either side.  For example, suppose we've got something
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index 925b4cf553..8eb5c8fd1d 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -172,31 +172,32 @@ typedef enum
  * to the opfamily and collation, with nulls at the indicated end of the range.
  * This allows us to obtain the needed comparison function from the opfamily.
  */
-static MergeJoinClause
+static void
 MJExamineQuals(List *mergeclauses,
 			   Oid *mergefamilies,
 			   Oid *mergecollations,
 			   int *mergestrategies,
 			   bool *mergenullsfirst,
-			   PlanState *parent)
+			   MergeJoinState *parent)
 {
-	MergeJoinClause clauses;
 	int			nClauses = list_length(mergeclauses);
 	int			iClause;
 	ListCell   *cl;
 
-	clauses = (MergeJoinClause) palloc0(nClauses * sizeof(MergeJoinClauseData));
+	parent->mj_Clauses = (MergeJoinClause) palloc0(nClauses * sizeof(MergeJoinClauseData));
+	parent->mj_UseEqual = (bool *) palloc0(nClauses * sizeof(bool));
+	parent->mj_UseLesser = (bool *) palloc0(nClauses * sizeof(bool));
 
 	iClause = 0;
 	foreach(cl, mergeclauses)
 	{
 		OpExpr	   *qual = (OpExpr *) lfirst(cl);
-		MergeJoinClause clause = [iClause];
+		MergeJoinClause clause = >mj_Clauses[iClause];
 		Oid			opfamily = mergefamilies[iClause];
 		Oid			collation = mergecollations[iClause];
-		StrategyNumber opstrategy = mergestrategies[iClause];
+		StrategyNumber sort_op_strategy = mergestrategies[iClause];
 		bool		nulls_first = mergenullsfirst[iClause];
-		int			op_strategy;
+		int			join_op_strategy;
 		Oid			op_lefttype;
 		Oid			op_righttype;
 		Oid			sortfunc;
@@ -207,28 +208,50 @@ MJExamineQuals(List *mergeclauses,
 		/*
 		 * Prepare the input expressions for execution.
 		 */
-		clause->lexpr = ExecInitExpr((Expr *) linitial(qual->args), parent);
-		clause->rexpr = ExecInitExpr((Expr *) lsecond(qual->args), parent);
+		clause->lexpr = ExecInitExpr((Expr *) linitial(qual->args), (PlanState *) parent);
+		clause->rexpr = ExecInitExpr((Expr *) lsecond(qual->args), (PlanState *) parent);
 
 		/* Set up sort support data */
 		clause->ssup.ssup_cxt = CurrentMemoryContext;
 		clause->ssup.ssup_collation = collation;
-		if (opstrategy == BTLessStrategyNumber)
+		if (sort_op_strategy == BTLessStrategyNumber)
 			clause->ssup.ssup_reverse = false;
-		else if (opstrategy == BTGreaterStrategyNumber)
+		else if (sort_op_strategy == BTGreaterStrategyNumber)
 			clause->ssup.ssup_reverse = true;
 		else	/* planner screwed up */
-			elog(ERROR, "unsupported mergejoin strategy %d", opstrategy);
+			elog(ERROR, "unsupported mergejoin strategy %d", sort_op_strategy);
 		clause->ssup.ssup_nulls_first = nulls_first;
 
 		/* Extract the operator's declared left/right datatypes */
 		get_op_opfamily_properties(qual->opno, opfamily, false,
-   _strategy,
+   _op_strategy,
    _lefttype,
    _righttype);
-		if (op_strategy != BTEqualStrategyNumber)	/* should not happen */
-			elog(ERROR, "cannot merge using non-equality operator %u",
- qual->opno);
+
+		/* 
+		 * Determine whether we accept lesser and/or equal tuples 
+		 * of the inner relation.
+		 */
+		switch (join_op_strategy)
+		{
+			case BTEqualStrategyNumber:
+parent->mj_UseEqual[iClause] = true;
+break;
+			case BTLessEqualStrategyNumber:
+parent->mj_UseEqual[iClause] = true;
+/* fall through */
+			case BTLessStrategyNumber:
+parent->mj_UseLesser[iClause] = true;
+break;

Re: [HACKERS] why not parallel seq scan for slow functions

[Robert Haas]

On Mon, Aug 21, 2017 at 5:08 AM, Amit Kapila  wrote:
> Thanks for acknowledging the idea.  I have written a patch which
> implements the above idea.  At this stage, it is merely to move the
> discussion forward. Few things which I am not entirely happy about
> this patch are:
>
> (a) To skip generating gather path for top level scan node, I have
> used the number of relations which has RelOptInfo, basically
> simple_rel_array_size. Is there any problem with it or do you see any
> better way?

I'm not sure.

> (b) I have changed the costing of gather path for path target in
> generate_gather_paths which I am not sure is the best way. Another
> possibility could have been that I change the code in
> apply_projection_to_path as done in the previous patch and just call
> it from generate_gather_paths.  I have not done that because of your
> comment above thread ("is probably unsafe, because it might confuse
> code that reaches the modified-in-place path through some other
> pointer (e.g. code which expects the RelOptInfo's paths to still be
> sorted by cost).").  It is not clear to me what exactly is bothering
> you if we directly change costing in apply_projection_to_path.

The point I was trying to make is that if you retroactively change the
cost of a path after you've already done add_path(), it's too late to
change your mind about whether to keep the path.  At least according
to my current understanding, that's the root of this problem in the
first place.  On top of that, add_path() and other routines that deal
with RelOptInfo path lists expect surviving paths to be ordered by
descending cost; if you frob the cost, they might not be properly
ordered any more.

I don't really have time right now to give this patch the attention
which it deserves; I can possibly come back to it at some future
point, or maybe somebody else will have time to give it a look.

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

[Robert Haas]

On Fri, Aug 25, 2017 at 12:05 PM, Tom Lane  wrote:
> Hm.  It seemed pretty reliable for me, but we already know that the
> parallel machinery is quite timing-sensitive.  I think we'd better
> incorporate a regression test case into the committed patch so we
> can see what the buildfarm thinks.  I'll see about adding one.

That would be great.  I didn't have a great idea how to write a test
for this -- maybe one of my systematic failures as a developer.  :-(

>> I think that the comment at the bottom of ExecSetTupleBound should
>> probably be rethought a bit in light of these changes.
>
> Agreed; I'll revisit all the comments, actually.

That also sounds great.

> No, I think that's fundamentally the wrong idea.  The tuple bound
> inherently is a piece of information that has to apply across a whole
> scan.  If you pass it to ExecProcNode then you'll have to get into
> API contracts about whether it's allowed to change across calls,
> which is a mess, even aside from efficiency concerns (and for
> ExecProcNode, I *am* concerned about efficiency).

Fair.

> You could imagine adding it as a parameter to ExecReScan, instead, but
> then there are a bunch of issues with how that would interact with the
> existing optimizations for skipped/delayed/merged rescan calls (cf.
> the existing open issue with parallel rescans).  That is a can of worms
> I'd just as soon not open.
>
> I'm content to leave it as a separate call.  I still think that worrying
> about extra C function calls for this purpose is, at best, premature
> optimization.

OK, you may well be right.  Outside of Limit, I suppose we're not
likely to set any limit other than 1, and if we set a limit of 1 it'll
probably be a limit that survives rescans, because it'll be based on
the parent node not caring about anything more than the first tuple.

BTW, I think another reason why we're likely to eventually want to get
very aggressive about passing down bounds is batch execution.  That's
somewhere on Andres's list of things that could speed up the executor,
although I believe at present there are quite a few other bottlenecks
that are more urgent.  But certainly if we start doing that, then even
things like SeqScan are going to want to know about applicable limit
clauses.

> I'll do another pass over the patch and get back to you.  I've not
> looked at the instrumentation patch yet --- is there a reason to
> worry about it before we finish this one?

I think they're independent.

-- 
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] Proposal: global index

[Robert Haas]

On Fri, Aug 25, 2017 at 6:52 AM, Ildar Musin  wrote:
> I agree with you that garbage collection after partitions drop could be a
> major downside of single index scheme. On the other hand not all
> partitioning use-cases imply dropping partitions. What worries me about
> global unique index built on multiple local indexes is the need to lookup
> (almost) every index for every insert/update/FK check. In some cases we can
> reduce the number of the indexes to be checked (e.g. by storing min/max
> values in metapage), but it will not be possible if key values are spread
> across indexes evenly. And it can get quite expensive as partition count
> grows.

+1.  I think that in the end we probably need both things for
different use cases.  Some people are going to want 1000 partitions
(or, if they can get away with it, 100,000 partitions) and be able to
do lookups on a secondary key without searching O(n) indexes.  Other
people are going to want partitioned indexes so that they can drop
them quickly, vacuum them quickly, etc.  I don't see anything wrong
with eventually offering both things.

I do think that it might be premature to work on this without solving
some of the other problems in this area first.  I think a good first
step would be to solve all the problems with declaring an index on a
parent table and having it cascade down to all children - i.e. a
partitioned index - cf.
https://www.postgresql.org/message-id/c8fe4f6b-ff46-aae0-89e3-e936a35f0...@postgrespro.ru
- and then work on the problems associated with defining foreign keys
reference such an index (e.g. in the case where the index matches the
partitioning key, or using the technique Andres describes) - and only
then do what you're proposing here, once all of those preliminaries
have been sorted out.  Otherwise, I fear that this patch will get
tangled up in a lot of issues that are really separate concerns.

JD is quite right that there are a lot of things about partitioning
that need to be improved from where we are today, but I think it's
important that we're a bit methodical about how we do that so that we
don't end up with a mess.  We're not going accept quick hacks in
related areas just to get global indexes; all of the issues about how
global indexes interact with the SQL syntax, foreign key constraints,
partitioned indexes, etc. need to be well-sorted out before we accept
a patch for global indexes.  It will be easiest, I think, to sort
those things out first and add this at the end.  That doesn't mean
that development can't be done concurrently, but I think what you're
likely to find is that getting the actual index machinery to do what
you want is a job and a half by itself without burdening the same
patch with anything additional.

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

[Tom Lane]

Robert Haas  writes:
> I had the same thought.  Done in the attached revision of your
> version.  I couldn't consistently reproduce the failure you saw -- it
> failed for me only about 1 time in 10 -- but I don't think it's
> failing any more with this version.

Hm.  It seemed pretty reliable for me, but we already know that the
parallel machinery is quite timing-sensitive.  I think we'd better
incorporate a regression test case into the committed patch so we
can see what the buildfarm thinks.  I'll see about adding one.

> I think that the comment at the bottom of ExecSetTupleBound should
> probably be rethought a bit in light of these changes.

Agreed; I'll revisit all the comments, actually.

> The details aside, Konstantin Knizhnik's proposal to teach this code
> about ForeignScans is yet another independent way for this to win, and
> I think that will also be quite a large win.

+1

> It's possible that we should work out some of these things at plan
> time rather than execution time, and it's also possible that a
> separate call to ExecSetTupleBound is too much work.  I've mulled over
> the idea of whether we should get rid of this mechanism altogether in
> favor of passing the tuple bound as an additional argument to
> ExecProcNode, because it seems silly to call node-specific code once
> to set a (normally small, possibly 1) bound and then call it again to
> get a (or the) tuple.

No, I think that's fundamentally the wrong idea.  The tuple bound
inherently is a piece of information that has to apply across a whole
scan.  If you pass it to ExecProcNode then you'll have to get into
API contracts about whether it's allowed to change across calls,
which is a mess, even aside from efficiency concerns (and for
ExecProcNode, I *am* concerned about efficiency).

You could imagine adding it as a parameter to ExecReScan, instead, but
then there are a bunch of issues with how that would interact with the
existing optimizations for skipped/delayed/merged rescan calls (cf.
the existing open issue with parallel rescans).  That is a can of worms
I'd just as soon not open.

I'm content to leave it as a separate call.  I still think that worrying
about extra C function calls for this purpose is, at best, premature
optimization.

I'll do another pass over the patch and get back to you.  I've not
looked at the instrumentation patch yet --- is there a reason to
worry about it before we finish this one?

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

[Robert Haas]

On Fri, Aug 25, 2017 at 11:15 AM, Tom Lane  wrote:
> The problem I exhibited with the updated patch could probably be resolved
> if the top level logic in a parallel worker knows about tuples_needed
> and doesn't try to pull more than that from its subplan.  So what you're
> describing isn't just an additional optimization, it's necessary to make
> this work at all.

I had the same thought.  Done in the attached revision of your
version.  I couldn't consistently reproduce the failure you saw -- it
failed for me only about 1 time in 10 -- but I don't think it's
failing any more with this version.

I think that the comment at the bottom of ExecSetTupleBound should
probably be rethought a bit in light of these changes.  Teaching the
parallel worker about tuples_needed may not be JUST an additional
optimization, but it IS an additional optimization; IOW, what the
planner can put between Sort and Limit will no longer be the only
relevant consideration.

The details aside, Konstantin Knizhnik's proposal to teach this code
about ForeignScans is yet another independent way for this to win, and
I think that will also be quite a large win.  In the future, I believe
that we might want to use a mechanism like this in even more cases.
For example, a Semi Join or Anti Join only needs 1 row from the inner
side per execution.  There's probably no reason to put a Sort under
the inner side of a Semi Join or Anti Join, but it's certainly not
impossible to have a Foreign Scan there.  It's likely to be an
expensive plan, but it's a heck of a lot more expensive if we fetch
100 rows when we only need 1.

I'm not sure the existing mechanism will stretch to handle such cases.
It's possible that we should work out some of these things at plan
time rather than execution time, and it's also possible that a
separate call to ExecSetTupleBound is too much work.  I've mulled over
the idea of whether we should get rid of this mechanism altogether in
favor of passing the tuple bound as an additional argument to
ExecProcNode, because it seems silly to call node-specific code once
to set a (normally small, possibly 1) bound and then call it again to
get a (or the) tuple.  But I'm not sure passing it to ExecProcNode is
really the right idea; it's adding some overhead for something that is
a bit of a special case, and I'm not sure it will work out cleanly
otherwise, either.  I'd be interested in your thoughts if you have
any.  My intuition is that there's substantially more to be gained in
this area, but exactly how best to gain it is not very clear to me.

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


push-down-bound-to-workers-3.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] [PATCH] Push limit to sort through a subquery

[Tom Lane]

Robert Haas  writes:
> However, there's another opportunity for optimization here, which is
> the Limit -> Gather -> Anything case.  A worker which has itself
> generated enough tuples to fill the limit can certainly stop working,
> but right now it doesn't know that.  The patch I posted before didn't
> think about that case, but it seems to require only trivial
> modifications.

Ah, good point.

> I was going to post an updated patch trying to
> address that, but I see that you've already posted something, so I'll
> go have a look at that instead.

The problem I exhibited with the updated patch could probably be resolved
if the top level logic in a parallel worker knows about tuples_needed
and doesn't try to pull more than that from its subplan.  So what you're
describing isn't just an additional optimization, it's necessary to make
this work at all.

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

[Robert Haas]

On Fri, Aug 25, 2017 at 9:24 AM, Tom Lane  wrote:
> Basically, this argument is that we should contort the code in order
> to avoid tail recursion, rather than assuming the compiler will turn
> that recursion into iteration, even in cases where there is really
> zero evidence that we should be worrying about performance at all
> rather than correctness and readability.  I do not agree (and just
> finished pushing a patch to change it).

I'm not prepared to argue further about it.  This strikes me as one of
those things that is really a matter of judgement; I don't really care
much about how the code ends up but I think your portrayal of what was
there is unduly negative.

>> On another note, here's a second patch applying on top of my earlier
>> patch to push down Limit through Gather and Gather Merge.
>
> Can you demonstrate any case where the planner would put Gather between
> Sort and Limit?  The GatherMerge case is probably interesting, but I'm
> having doubts about Gather.

That's an interesting point.  I was thinking that we needed both of
these patches in order to make the proposed regression test pass,
because in the relevant case we'll have Gather -> Limit -> Sort, but
actually we only need the second one, because as you point out here
what matters is what's between the Limit and the Sort.  I was thinking
that we'd need to be able to see through the Gather at the top of the
plan, but we don't.

However, there's another opportunity for optimization here, which is
the Limit -> Gather -> Anything case.  A worker which has itself
generated enough tuples to fill the limit can certainly stop working,
but right now it doesn't know that.  The patch I posted before didn't
think about that case, but it seems to require only trivial
modifications.   I was going to post an updated patch trying to
address that, but I see that you've already posted something, so I'll
go have a look at that instead.

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

[Tom Lane]

Amit Kapila  writes:
> On Fri, Aug 25, 2017 at 7:35 PM, Tom Lane  wrote:
>> it would be stupid to put a filter on that node rather than its
>> children, but I see this in both nodeGather.c and nodeGatherMerge.c:
>>
>> gatherstate->ps.qual =
>> ExecInitQual(node->plan.qual, (PlanState *) gatherstate);
>> 
>> It doesn't look like the qual is actually used anywhere in either node
>> type.  Am I right in thinking this is dead code?

> I also think so.  I think this was required in some initial versions
> of gather node patch where we were thinking of having a single node
> (instead of what we have now that Gather node and beneath there will
> be partial scan node) to perform parallel scans.

Thanks for confirming.  I'll replace that with something like
Assert(!node->plan.qual).  I have some other code-review-ish
fixes to do in nodeGatherMerge, too.

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

[Tom Lane]

Robert Haas  writes:
> I'm inclined to commit both of these after a little more testing and
> self-review, but let me know if anyone else wants to review first.

Attached is an updated version of the first patch, which I rebased
over 3f4c7917b, improved a bunch of the comments for, and fixed a
couple of obvious typos in.  However, when I went to test it,
it blew up real good:

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=# explain analyze select * from tenk1 order by fivethous limit 4;
ERROR:  retrieved too many tuples in a bounded sort
CONTEXT:  parallel worker

The cause is obvious: GatherMerge doesn't know about the contract that
it's not supposed to pull more than tuples_needed rows from an input after
promising not to do so.  I am not convinced that it's worth adding the
logic that would be needed to make that happen, so my inclination is to
abandon this patch.  But here it is if you want to push further.

regards, tom lane

diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index ce47f1d..1287025 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -47,17 +47,26 @@
  * greater than any 32-bit integer here so that values < 2^32 can be used
  * by individual parallel nodes to store their own state.
  */
-#define PARALLEL_KEY_PLANNEDSTMT		UINT64CONST(0xE001)
-#define PARALLEL_KEY_PARAMSUINT64CONST(0xE002)
-#define PARALLEL_KEY_BUFFER_USAGE		UINT64CONST(0xE003)
-#define PARALLEL_KEY_TUPLE_QUEUE		UINT64CONST(0xE004)
-#define PARALLEL_KEY_INSTRUMENTATION	UINT64CONST(0xE005)
-#define PARALLEL_KEY_DSAUINT64CONST(0xE006)
-#define PARALLEL_KEY_QUERY_TEXT		UINT64CONST(0xE007)
+#define PARALLEL_KEY_EXECUTOR_FIXED		UINT64CONST(0xE001)
+#define PARALLEL_KEY_PLANNEDSTMT		UINT64CONST(0xE002)
+#define PARALLEL_KEY_PARAMSUINT64CONST(0xE003)
+#define PARALLEL_KEY_BUFFER_USAGE		UINT64CONST(0xE004)
+#define PARALLEL_KEY_TUPLE_QUEUE		UINT64CONST(0xE005)
+#define PARALLEL_KEY_INSTRUMENTATION	UINT64CONST(0xE006)
+#define PARALLEL_KEY_DSAUINT64CONST(0xE007)
+#define PARALLEL_KEY_QUERY_TEXT		UINT64CONST(0xE008)
 
 #define PARALLEL_TUPLE_QUEUE_SIZE		65536
 
 /*
+ * Fixed-size random stuff that we need to pass to parallel workers.
+ */
+typedef struct FixedParallelExecutorState
+{
+	int64		tuples_needed;	/* tuple bound, see ExecSetTupleBound */
+} FixedParallelExecutorState;
+
+/*
  * DSM structure for accumulating per-PlanState instrumentation.
  *
  * instrument_options: Same meaning here as in instrument.c.
@@ -381,12 +390,14 @@ ExecParallelReinitialize(ParallelExecutorInfo *pei)
  * execution and return results to the main backend.
  */
 ParallelExecutorInfo *
-ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers)
+ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers,
+	 int64 tuples_needed)
 {
 	ParallelExecutorInfo *pei;
 	ParallelContext *pcxt;
 	ExecParallelEstimateContext e;
 	ExecParallelInitializeDSMContext d;
+	FixedParallelExecutorState *fpes;
 	char	   *pstmt_data;
 	char	   *pstmt_space;
 	char	   *param_space;
@@ -418,6 +429,11 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers)
 	 * for the various things we need to store.
 	 */
 
+	/* Estimate space for fixed-size state. */
+	shm_toc_estimate_chunk(>estimator,
+		   sizeof(FixedParallelExecutorState));
+	shm_toc_estimate_keys(>estimator, 1);
+
 	/* Estimate space for query text. */
 	query_len = strlen(estate->es_sourceText);
 	shm_toc_estimate_chunk(>estimator, query_len);
@@ -487,6 +503,11 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers)
 	 * asked for has been allocated or initialized yet, though, so do that.
 	 */
 
+	/* Store fixed-size state. */
+	fpes = shm_toc_allocate(pcxt->toc, sizeof(FixedParallelExecutorState));
+	fpes->tuples_needed = tuples_needed;
+	shm_toc_insert(pcxt->toc, PARALLEL_KEY_EXECUTOR_FIXED, fpes);
+
 	/* Store query string */
 	query_string = shm_toc_allocate(pcxt->toc, query_len);
 	memcpy(query_string, estate->es_sourceText, query_len);
@@ -833,6 +854,7 @@ ExecParallelInitializeWorker(PlanState *planstate, shm_toc *toc)
 void
 ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
 {
+	FixedParallelExecutorState *fpes;
 	BufferUsage *buffer_usage;
 	DestReceiver *receiver;
 	QueryDesc  *queryDesc;
@@ -841,6 +863,9 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
 	void	   *area_space;
 	dsa_area   *area;
 
+	/* Get fixed-size state. */
+	fpes = shm_toc_lookup(toc, PARALLEL_KEY_EXECUTOR_FIXED, false);
+
 	/* Set up DestReceiver, SharedExecutorInstrumentation, and 

Re: [HACKERS] [PATCH] Push limit to sort through a subquery

[Amit Kapila]

On Fri, Aug 25, 2017 at 7:35 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I'm inclined to commit both of these after a little more testing and
>> self-review, but let me know if anyone else wants to review first.
>
> Looking through the first one, I wondered whether we needed to
> check for a qual expression on Gather or GatherMerge.  It seems like
> it would be stupid to put a filter on that node rather than its
> children, but I see this in both nodeGather.c and nodeGatherMerge.c:
>
> /*
>  * initialize child expressions
>  */
> gatherstate->ps.qual =
> ExecInitQual(node->plan.qual, (PlanState *) gatherstate);
>
> It doesn't look like the qual is actually used anywhere in either node
> type.  Am I right in thinking this is dead code?
>

I also think so.  I think this was required in some initial versions
of gather node patch where we were thinking of having a single node
(instead of what we have now that Gather node and beneath there will
be partial scan node) to perform parallel scans.

-- 
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] Not listed as committer

[Stephen Frost]

* Magnus Hagander (mag...@hagander.net) wrote:
> On Fri, Aug 25, 2017 at 3:47 PM, Michael Meskes 
> wrote:
> > I just committed a patch that is listed in the CF and had to put
> > somebody (I chose you Bruce :)) in as a committer because I was not
> > listed. Do I have to register somewhere?
> >
> 
> Ha, that's interesting.
> 
> Should be fixed now, please try again.

Almost certainly because he hadn't logged into the commitfest app at the
time that the initial set of committers were selected, so he didn't have
an account on the CF system yet.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] Push limit to sort through a subquery

[Tom Lane]

Robert Haas  writes:
> I'm inclined to commit both of these after a little more testing and
> self-review, but let me know if anyone else wants to review first.

Looking through the first one, I wondered whether we needed to
check for a qual expression on Gather or GatherMerge.  It seems like
it would be stupid to put a filter on that node rather than its
children, but I see this in both nodeGather.c and nodeGatherMerge.c:

/*
 * initialize child expressions
 */
gatherstate->ps.qual =
ExecInitQual(node->plan.qual, (PlanState *) gatherstate);

It doesn't look like the qual is actually used anywhere in either node
type.  Am I right in thinking this is dead code?

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] ECPG: WHENEVER statement with DO CONTINUE action

[Christian Ullrich]

* Michael Meskes wrote:


The v3 patch looks good to me. I've changed this patch status to Ready
for Committer.


Thank you all, committed.


The buildfarm says that sorting is frequently done case-sensitively:

*** 1,2 
- josh 1.00 10.00
  Ram 00.00 21.00
--- 1,2 
  Ram 00.00 21.00
+ josh 1.00 10.00

--
Christian


--
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] Not listed as committer

[Magnus Hagander]

On Fri, Aug 25, 2017 at 3:47 PM, Michael Meskes 
wrote:

> All,
>
> I just committed a patch that is listed in the CF and had to put
> somebody (I chose you Bruce :)) in as a committer because I was not
> listed. Do I have to register somewhere?
>

Ha, that's interesting.

Should be fixed now, please try again.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [HACKERS] Proposal: global index

[Chris Travers]

On Fri, Aug 25, 2017 at 12:15 PM, Petr Jelinek  wrote:

> On 25/08/17 10:28, Chris Travers wrote:
> >
> >
> > On Thu, Aug 24, 2017 at 9:44 PM, Andres Freund  > > wrote:
> >
> > Hi,
> >
> > On 2017-08-18 12:12:58 +0300, Ildar Musin wrote:
> > > While we've been developing pg_pathman extension one of the most
> frequent
> > > questions we got from our users was about global index support. We
> cannot
> > > provide it within an extension. And I couldn't find any recent
> discussion
> > > about someone implementing it. So I'm thinking about giving it a
> shot and
> > > start working on a patch for postgres.
> >
> > FWIW, I personally think for constraints the better approach is to
> make
> > the constraint checking code cope with having to check multiple
> > indexes. Initially by just checking all indexes, over the longer term
> > perhaps pruning the set of to-be-checked indexes based on the values
> in
> > the partition key if applicable.   The problem with creating huge
> global
> > indexes is that you give away some the major advantages of
> partitioning:
> > - dropping partitions now is slow / leaves a lof of garbage again
> > - there's no way you can do this with individual partitions being
> remote
> >   or such
> > - there's a good chunk of locality loss in global indexes
> >
> > The logic we have for exclusion constraints checking can essentially
> be
> > extended to do uniqueness checking over multiple partitions.
> Depending
> > on the desired deadlock behaviour one might end up doing speculative
> > insertions in addition.  The foreign key constraint checking is
> fairly
> > simple, essentially one "just" need to remove the ONLY from the
> > generated check query.
> >
>
> +1 (or +as much as I am allowed to get away with really ;) )
>
> >
> > To be clear, this would still require a high-level concept of a global
> > index and the only question is whether it gets stored as multiple
> > partitions against partitioned tables vs stored in one giant index,
> right?
> >
> No, just global constraints. For example, if you consider unique index
> to be implementation detail of a unique constraint, there is nothing
> stopping us to use multiple such indexes (one per partition) as
> implementation detail to single global unique constraint. No need for
> global index at all.
>

Ok so in this case a global constraint needs to track partitioned indexes,
right?

How does this differ, in practice from a "global but partitioned index?"
 This seems like splitting hairs but I am trying to see if there is
disagreement here that goes beyond language.

For example, could I have a global, partial unique constraint the way I can
do things with a single table currently (something like create global
unique index foo_id_idxuf on foo(Id) where id > 12345)?  Is this something
the discussion here would foreclose?

It seems to me that you can get both of these (and more) by adding the
concept of a global index which means:
1.  Index is on parent table
2.  Index is inherited to child tables and managed on parent.
3.  Writes to children that hit inherited unique index ALSO must check
(with exclusion constraints etc) ALL other tables in the inheritance tree
of the index.

That would also have a few important side benefits:
1.  Easier management of indexes where all partitions (or other children
since there are other uses for table inheritance than partitioning) must be
indexed
2.  Ability to have partial unique indexes enforced consistently across an
inheritance tree.

An alternative might be to generalise partial unique indexes into partial
unique constraints. (alter table foo add unique (bar) where id > 12345)


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



-- 
Best Regards,
Chris Travers
Database Administrator

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

Re: [HACKERS] ECPG: WHENEVER statement with DO CONTINUE action

[Vinayak Pokale]

On Aug 25, 2017 10:45 PM, "Michael Meskes"  wrote:
>
> > The v3 patch looks good to me. I've changed this patch status to Ready
> > for Committer.
>
> Thank you all, committed.

Thank you very much.

Regards,
Vinayak Pokale

[HACKERS] Not listed as committer

[Michael Meskes]

All,

I just committed a patch that is listed in the CF and had to put
somebody (I chose you Bruce :)) in as a committer because I was not
listed. Do I have to register somewhere?

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] ECPG: WHENEVER statement with DO CONTINUE action

[Michael Meskes]

> The v3 patch looks good to me. I've changed this patch status to Ready
> for Committer.

Thank you all, committed.

Michael

-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL


-- 
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] visual studio 2017 build support

[Christian Ullrich]

* On 2017-06-21 02:06, Haribabu Kommi wrote:


Thanks for the review. Here I attached an updated patch with README update.


Hello,

the most recent update to VS 2017, version 15.3, now identifies as 
"14.11" rather than "14.10" in the output of nmake /?. Simply adding 
this value to the two places that check for 14.10 in your patch appears 
to work for me.


In a newly created project, PlatformToolset is still "v141". 
ToolsVersion is "15.0" whereas your patch uses "14.1".


ISTM that the ToolsVersion has been like this in all versions of VS 
2017; in my collection of .vcxproj files the auto-generated PostgreSQL 
projects are the only ones using "14.1".


The build is successful with either value.

I think there was some discussion on this topic, but I cannot find it in 
the archives. If there was, I certainly don't want to reopen any 
discussions.


All the best,

--
Christian


--
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] Tuple-routing for certain partitioned tables not working as expected

[Robert Haas]

On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita
 wrote:
> Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just
> ripping the CheckValidResultRel checks out entirely is not a good idea, but
> that seems OK to me at least as a fix just for v10.

I'm still not on-board with having this be the one case where we don't
do CheckValidResultRel.  If we want to still call it but pass down
some additional information that can selectively skip certain checks,
I could probably live with that.

At some point we've got to stop developing v10 and just let it be what it is.

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

[Tom Lane]

Robert Haas  writes:
> On Thu, Aug 24, 2017 at 2:24 PM, Tom Lane  wrote:
>> I'd have been okay with changing the entire function if it could still be
>> doing all cases the same way.  But the exception for merge-append (and
>> probably soon other cases) means you still end up with a readability
>> problem.

> I don't really agree with that.  I think it's reasonable to handle the
> cases where we are just looking through nodes differently than the
> others.  Just because we can't conveniently avoid recursing in every
> case doesn't mean, to me, that we should recurse even when it's easily
> avoidable.

Basically, this argument is that we should contort the code in order
to avoid tail recursion, rather than assuming the compiler will turn
that recursion into iteration, even in cases where there is really
zero evidence that we should be worrying about performance at all
rather than correctness and readability.  I do not agree (and just
finished pushing a patch to change it).

> On another note, here's a second patch applying on top of my earlier
> patch to push down Limit through Gather and Gather Merge.

Can you demonstrate any case where the planner would put Gather between
Sort and Limit?  The GatherMerge case is probably interesting, but I'm
having doubts about Gather.

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

[Robert Haas]

On Thu, Aug 24, 2017 at 2:24 PM, Tom Lane  wrote:
> I'd have been okay with changing the entire function if it could still be
> doing all cases the same way.  But the exception for merge-append (and
> probably soon other cases) means you still end up with a readability
> problem.

I don't really agree with that.  I think it's reasonable to handle the
cases where we are just looking through nodes differently than the
others.  Just because we can't conveniently avoid recursing in every
case doesn't mean, to me, that we should recurse even when it's easily
avoidable.  I think what we might think about doing is have a node
that loops over Subquery Scan and Result and drills down, and then
handle the rest of the cases either as at present or, perhaps, with a
switch.

On another note, here's a second patch applying on top of my earlier
patch to push down Limit through Gather and Gather Merge.  This one
propagates information about sorts performed in parallel workers back
to the leader and adjusts the test case to use a non-temp table again.
This second patch has the nice property of being a fairly good test
that the first patch is actually doing something.

I'm inclined to commit both of these after a little more testing and
self-review, but let me know if anyone else wants to review first.

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


propagate-sort-instrumentation.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] MAIN, Uncompressed?

[Tom Lane]

Simon Riggs  writes:
> On 25 August 2017 at 13:21, Tom Lane  wrote:
>> If you know compression isn't useful, but you don't want to fail on
>> wide values, then "external" should serve the purpose.

> Well, almost. External toasts at 2048-ish bytes whereas Main toasts at
> 8160 bytes.
> The rows are typically near 4kB long, so if marked External they would
> always be toasted.
> It's desirable to have the full row in the heap block, rather than
> have to access heap-toastindex-toastblocks in all cases.
> The data is also incompressible, so Main just wastes time on insert.
> Hence, we have a missing option.

Maybe, but the use case seems mighty narrow.

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] MAIN, Uncompressed?

[Simon Riggs]

On 25 August 2017 at 13:21, Tom Lane  wrote:
> Simon Riggs  writes:
>> Main is roughly what is wanted, yet it always tries to compress. If
>> you already know that won't be useful it should be possible to turn
>> compression off.
>
> If you know compression isn't useful, but you don't want to fail on
> wide values, then "external" should serve the purpose.

Well, almost. External toasts at 2048-ish bytes whereas Main toasts at
8160 bytes.

The rows are typically near 4kB long, so if marked External they would
always be toasted.

It's desirable to have the full row in the heap block, rather than
have to access heap-toastindex-toastblocks in all cases.

The data is also incompressible, so Main just wastes time on insert.

Hence, we have a missing option.

-- 
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] MAIN, Uncompressed?

[Tom Lane]

Simon Riggs  writes:
> Main is roughly what is wanted, yet it always tries to compress. If
> you already know that won't be useful it should be possible to turn
> compression off.

If you know compression isn't useful, but you don't want to fail on
wide values, then "external" should serve the purpose.

regards, tom lane


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

Re: [HACKERS] MAIN, Uncompressed?

[Simon Riggs]

On 25 August 2017 at 12:24, Tom Lane  wrote:
> Simon Riggs  writes:
>> It looks like we need a new Column Storage option for MAIN, Uncompressed.
>> We have these Column Storage options
>> Plain - inline only, uncompressed
>> Main - inline until external as last resort, compressible
>> External - external, uncompressed
>> Extended - external, compressible
>
>> So there is no option for Main, but not compressible...
>
> Doesn't Plain serve the purpose?

No, because that just gives an error if you try to insert a large column value.

> In point of fact, though, "never inline and never compress" is not really
> a useful option, as it can be more easily read as "fail on wide values".

Agreed, but that is not what I am proposing.

Main is roughly what is wanted, yet it always tries to compress. If
you already know that won't be useful it should be possible to turn
compression off.

-- 
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] MAIN, Uncompressed?

[Tom Lane]

Simon Riggs  writes:
> It looks like we need a new Column Storage option for MAIN, Uncompressed.
> We have these Column Storage options
> Plain - inline only, uncompressed
> Main - inline until external as last resort, compressible
> External - external, uncompressed
> Extended - external, compressible

> So there is no option for Main, but not compressible...

Doesn't Plain serve the purpose?

In point of fact, though, "never inline and never compress" is not really
a useful option, as it can be more easily read as "fail on wide values".

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] Explicit relation name in VACUUM VERBOSE log

[Masahiko Sawada]

On Thu, Aug 24, 2017 at 11:35 PM, Tom Lane  wrote:
> Alvaro Herrera  writes:
>> Michael Paquier wrote:
>>> Hm. I am not sure what you have in mind here.
>
>> I'm thinking that this data is useful to analyze as a stream of related
>> events, rather than as individual data points.  Grepping logs in order to
>> extract the numbers is lame and slow.
>
> Yes.  And there is a bigger issue here, which is that the output of
> VACUUM VERBOSE is meant to be sent to the client for *human* readability.
> (As you noted, INFO-level messages don't normally go to the server log
> in the first place, and that's not by accident.)  Repeating the full table
> name in every line will be really annoying for that primary use-case.
> I am not sure what we want to do to address Masahiko-san's use-case, but
> ideally his workflow wouldn't involve log-scraping at all.

The use-case I had is that I run vacuumdb *without -j option* and save
all verbose logs into a text file, and then checking it later. I said
vacuumdb with -j option before but it was wrong. It cannot happen two
vacuum verbose logs are emitted mixed together even if -j option is
specified.

I sometimes search a particular table/index from the logs but also in
that case it was hard to distinguish logs. This is still not primary
case though.

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] Proposal: global index

[Ildar Musin]

Hi,

On 24.08.2017 22:44, Andres Freund wrote:

Hi,

On 2017-08-18 12:12:58 +0300, Ildar Musin wrote:

While we've been developing pg_pathman extension one of the most frequent
questions we got from our users was about global index support. We cannot
provide it within an extension. And I couldn't find any recent discussion
about someone implementing it. So I'm thinking about giving it a shot and
start working on a patch for postgres.


FWIW, I personally think for constraints the better approach is to make
the constraint checking code cope with having to check multiple
indexes. Initially by just checking all indexes, over the longer term
perhaps pruning the set of to-be-checked indexes based on the values in
the partition key if applicable.   The problem with creating huge global
indexes is that you give away some the major advantages of partitioning:
- dropping partitions now is slow / leaves a lof of garbage again
- there's no way you can do this with individual partitions being remote
  or such
- there's a good chunk of locality loss in global indexes



I agree with you that garbage collection after partitions drop could be 
a major downside of single index scheme. On the other hand not all 
partitioning use-cases imply dropping partitions. What worries me about 
global unique index built on multiple local indexes is the need to 
lookup (almost) every index for every insert/update/FK check. In some 
cases we can reduce the number of the indexes to be checked (e.g. by 
storing min/max values in metapage), but it will not be possible if key 
values are spread across indexes evenly. And it can get quite expensive 
as partition count grows.


The good thing about multiple indexes is that they are more compact and 
manageable.


--
Ildar Musin
i.mu...@postgrespro.ru


--
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: global index

[Petr Jelinek]

On 25/08/17 10:28, Chris Travers wrote:
> 
> 
> On Thu, Aug 24, 2017 at 9:44 PM, Andres Freund  > wrote:
> 
> Hi,
> 
> On 2017-08-18 12:12:58 +0300, Ildar Musin wrote:
> > While we've been developing pg_pathman extension one of the most 
> frequent
> > questions we got from our users was about global index support. We 
> cannot
> > provide it within an extension. And I couldn't find any recent 
> discussion
> > about someone implementing it. So I'm thinking about giving it a shot 
> and
> > start working on a patch for postgres.
> 
> FWIW, I personally think for constraints the better approach is to make
> the constraint checking code cope with having to check multiple
> indexes. Initially by just checking all indexes, over the longer term
> perhaps pruning the set of to-be-checked indexes based on the values in
> the partition key if applicable.   The problem with creating huge global
> indexes is that you give away some the major advantages of partitioning:
> - dropping partitions now is slow / leaves a lof of garbage again
> - there's no way you can do this with individual partitions being remote
>   or such
> - there's a good chunk of locality loss in global indexes
> 
> The logic we have for exclusion constraints checking can essentially be
> extended to do uniqueness checking over multiple partitions. Depending
> on the desired deadlock behaviour one might end up doing speculative
> insertions in addition.  The foreign key constraint checking is fairly
> simple, essentially one "just" need to remove the ONLY from the
> generated check query.
> 

+1 (or +as much as I am allowed to get away with really ;) )

> 
> To be clear, this would still require a high-level concept of a global
> index and the only question is whether it gets stored as multiple
> partitions against partitioned tables vs stored in one giant index, right?
> 
No, just global constraints. For example, if you consider unique index
to be implementation detail of a unique constraint, there is nothing
stopping us to use multiple such indexes (one per partition) as
implementation detail to single global unique constraint. No need for
global index at all.

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


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

Re: [HACKERS] More replication race conditions

[Petr Jelinek]

On 24/08/17 19:54, Tom Lane wrote:
> sungazer just failed with
> 
> pg_recvlogical exited with code '256', stdout '' and stderr 'pg_recvlogical: 
> could not send replication command "START_REPLICATION SLOT "test_slot" 
> LOGICAL 0/0 ("include-xids" '0', "skip-empty-xacts" '1')": ERROR:  
> replication slot "test_slot" is active for PID 8913148
> pg_recvlogical: disconnected
> ' at 
> /home/nm/farm/gcc64/HEAD/pgsql.build/src/test/recovery/../../../src/test/perl/PostgresNode.pm
>  line 1657.
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer=2017-08-24%2015%3A16%3A10
> 
> Looks like we're still not there on preventing replication startup
> race conditions.

Hmm, that looks like "by design" behavior. Slot acquiring will throw
error if the slot is already used by somebody else (slots use their own
locking mechanism that does not wait). In this case it seems the
walsender which was using slot for previous previous step didn't finish
releasing the slot by the time we start new command. We can work around
this by changing the test to wait perhaps.

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


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

[HACKERS] MAIN, Uncompressed?

[Simon Riggs]

It looks like we need a new Column Storage option for MAIN, Uncompressed.

We have these Column Storage options
Plain - inline only, uncompressed
Main - inline until external as last resort, compressible
External - external, uncompressed
Extended - external, compressible

So there is no option for Main, but not compressible...

With reference to code... there seems to be no way to skip step 3

/* --
 * Compress and/or save external until data fits into target length
 *
 *  1: Inline compress attributes with attstorage 'x', and store very
 * large attributes with attstorage 'x' or 'e' external immediately
 *  2: Store attributes with attstorage 'x' or 'e' external
 *  3: Inline compress attributes with attstorage 'm'
 *  4: Store attributes with attstorage 'm' external
 * --
 */

Not sure what to call this new option? MAINU?

Objections?

-- 
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] Thoughts on unit testing?

[Chris Travers]

On Thu, Aug 24, 2017 at 9:07 AM, Torsten Zuehlsdorff <
mailingli...@toco-domains.de> wrote:

>
>
> On 13.08.2017 21:19, Peter Geoghegan wrote:
>
>> On Thu, Aug 10, 2017 at 2:53 PM, Thomas Munro
>>  wrote:
>>
>>> The current regression tests, isolation tests and TAP tests are very
>>> good (though I admit my experience with TAP is limited), but IMHO we
>>> are lacking support for C-level unit testing.  Complicated, fiddly
>>> things with many states, interactions, edge cases etc can be hard to
>>> get full test coverage on from the outside.  Consider
>>> src/backend/utils/mmgr/freepage.c as a case in point.
>>>
>>
I don't want to see full test coverage of the code btw.  I think that
undermines the benefit from testing.

I would like to see better test coverage though for reasons below.

>
>> It is my understanding that much of the benefit of unit testing comes
>> from maintainability.
>>
>
> I never had this understanding. I write tests to test expected behavior
> and not the coded one. If possible i separate the persons writing
> unit-tests from the ones writing the function itself. Having someone really
> read the documentation or translate the expectation into a test-case, makes
> sure, the function itself works well.
>

Right.  I would go so far as to say I test to the documentation, not to the
code.  Usually when I test my own code, I write docs, then code, then I
re-read the docs, and write test cases from the docs.

In my not-so humble opinion, the point of tests is to make sure you don't
break other people's stuff, people who are reading the docs and using them
when they write their code.


>
> Also it really safes time in the long run. Subtle changes / bugs can be
> caught which unit-tests. Also a simple bug-report can be translated into a
> unit-test make sure that the bugfix really works and that no regression
> will happen later. I literally saved ones a week of work with a single
> unit-test.
>

Also comparing to the docs means we have a way to reason about where
misbehaviour is occurring that means better maintainability and less code
entropy in the course of fixing bugs.

>
> There are many other advantages, but to earn them the code need to be
> written to be testable. And this is often not the case. Most literature
> advises to Mocking, mixins or other techniques, which most times just
> translate into "this code is not written testable" or "the technique /
> language / concept / etc is not very good in being testable".
>

Agreed here.  Usually you end up with better, more stable, more carefully
designed components as a result.  But as I say, documentation, design, and
testing are actually harder to get right than coding

So with the above being said, the fact is that a lot of advanced stuff can
be done by writing C libraries that get preloaded into PostgreSQL. These C
libraries risk being broken by cases where behaviour does not match
documentation.  So if I want to translate an oid into a db name or vice
versa, I might call the internal functions to do this.  Having a stable C
API would be of great help in ensuring longer-term maintainability of such
libraries.  So I think it would be good to add some unit tests here.

Of course it also means we get to decide what functionality is sufficiently
stable to test and guarantee, and that saves both time in maintenance, and
it improves the safety of moving forward.

But I still think the question of what to test ought to be geared around
"what are we willing to try to guarantee as behaviour for some years, not
just to ourselves but to third parties."

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



-- 
Best Regards,
Chris Travers
Database Administrator

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

Re: [HACKERS] CONNECTION LIMIT and Parallel Query don't play well together

[David Rowley]

On 24 August 2017 at 11:15, Peter Eisentraut
 wrote:
> Here is a slightly updated patch for consideration in the upcoming
> commit fest.

Hi Peter,

I just had a quick glance over this and wondered about 2 things.

1. Why a GUC and not a new per user option so it can be configured
differently for different users? Something like ALTER USER ... WORKER
LIMIT ; perhaps. I mentioned about this up-thread a bit.

2.

+ if (count > max_worker_processes_per_user)
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
+ errmsg("too many worker processes for role \"%s\"",
+ GetUserNameFromId(GetUserId(), false;
+ LWLockRelease(BackgroundWorkerLock);
+ return false;

Unless I've misunderstood something, it seems that this is going to
give random errors to users which might only occur when they run
queries against larger tables. Part of why it made sense not to count
workers towards the CONNECTION LIMIT was the fact that we didn't want
to throw these random errors when workers could not be obtained when
we take precautions in other places to just silently have fewer
workers. There's lots of discussions earlier in this thread about this
and I don't think anyone was in favour of queries randomly working
sometimes.

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


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

Re: [HACKERS] locale problem of bgworker: logical replication launcher and worker process

[Ioseph Kim]

Thanks for reply.

I resolved this problem.

This problem is that dgettext() function use codeset of database's lc_ctype.

below database's lc_ctype is C, but locale is ko_KR.UTF8.

I made a new database with lc_ctype is ko_KR.UTF8.

this problem is resolved.



work logs are here.


(10) postgres@postgres=# \l
   데이터베이스 목록
   이름|  소유주  | 인코딩 | Collate |Ctype|  액세스 권한
---+--++-+-+---
 krdb  | postgres | UTF8   | C   | ko_KR.UTF-8 |
 postgres  | postgres | UTF8   | C   | C   |
 template0 | postgres | UTF8   | C   | C   | 
=c/postgres  +
   |  || | | 
postgres=CTc/postgres
 template1 | postgres | UTF8   | C   | C   | 
=c/postgres  +
   |  || | | 
postgres=CTc/postgres

(4개 행)

(10) postgres@postgres=# \c
접속정보: 데이터베이스="postgres", 사용자="postgres".
(10) postgres@postgres=# create subscription sub1 connection 
'host=127.0.0.1 port=5432 client_encoding=C' publication pub1;
2017-08-25 18:13:34.556 KST [5401] 오류:  발행 서버에 연결 할 수 없음: ??? 
??? ? ??: ??? ???

"127.0.0.1"  ??? ?? ???,
5432 ??? TCP/IP ???  ??.
2017-08-25 18:13:34.556 KST [5401] 명령 구문:  create subscription sub1 
connection 'host=127.0.0.1 port=5432 client_encoding=C' publication pub1;

오류:  발행 서버에 연결 할 수 없음: ??? ??? ? ??: ??? ???
"127.0.0.1"  ??? ?? ???,
5432 ??? TCP/IP ???  ??.
(10) postgres@postgres=# \c krdb
접속정보: 데이터베이스="krdb", 사용자="postgres".
(10) postgres@krdb=# create subscription sub1 connection 'host=127.0.0.1 
port=5432 client_encoding=C' publication pub1;
2017-08-25 18:13:45.687 KST [5402] 오류:  발행 서버에 연결 할 수 없음: 
서버에 연결할 수 없음: 연결이 거부됨

"127.0.0.1" 호스트에 서버가 가동 중인지,
5432 포트로 TCP/IP 연결이 가능한지 살펴보십시오.
2017-08-25 18:13:45.687 KST [5402] 명령 구문:  create subscription sub1 
connection 'host=127.0.0.1 port=5432 client_encoding=C' publication pub1;

오류:  발행 서버에 연결 할 수 없음: 서버에 연결할 수 없음: 연결이 거부됨
"127.0.0.1" 호스트에 서버가 가동 중인지,
5432 포트로 TCP/IP 연결이 가능한지 살펴보십시오.


2017년 08월 23일 22:40에 Peter Eisentraut 이(가) 쓴 글:

On 8/22/17 01:19, Ioseph Kim wrote:

2017-08-22 14:06:21.697 KST [306] 로그:  logical replication apply
worker for subscription "replica_a" has started
2017-08-22 14:06:21.698 KST [306] 오류:  발행 서버에 연결 할 수 없음:
??? ??? ? ??: ??? ???
 "localhost" (::1)  ??? ?? ???,
 5432 ??? TCP/IP ???  ??.
 ??? ??? ? ??: ??? ???
 "localhost" (127.0.0.1)  ??? ?? ???,
 5432 ??? TCP/IP ???  ??.
-

main postmaster messages are printed  in korean well,
but bgworker process message is not.

This problem seems to have occurred because the server locale
environment and the client's that are different.

I have tried it locally with a ko_KR locale, and it seems to work
correctly for me.  Still, I can imagine there are all kinds of ways this
could go wrong in particular configurations.  Could you construct a
reproducible test setup, including specific initdb and locale settings,
operating system, etc.?





--
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: global index

[Chris Travers]

On Thu, Aug 24, 2017 at 9:44 PM, Andres Freund  wrote:

> Hi,
>
> On 2017-08-18 12:12:58 +0300, Ildar Musin wrote:
> > While we've been developing pg_pathman extension one of the most frequent
> > questions we got from our users was about global index support. We cannot
> > provide it within an extension. And I couldn't find any recent discussion
> > about someone implementing it. So I'm thinking about giving it a shot and
> > start working on a patch for postgres.
>
> FWIW, I personally think for constraints the better approach is to make
> the constraint checking code cope with having to check multiple
> indexes. Initially by just checking all indexes, over the longer term
> perhaps pruning the set of to-be-checked indexes based on the values in
> the partition key if applicable.   The problem with creating huge global
> indexes is that you give away some the major advantages of partitioning:
> - dropping partitions now is slow / leaves a lof of garbage again
> - there's no way you can do this with individual partitions being remote
>   or such
> - there's a good chunk of locality loss in global indexes
>
> The logic we have for exclusion constraints checking can essentially be
> extended to do uniqueness checking over multiple partitions. Depending
> on the desired deadlock behaviour one might end up doing speculative
> insertions in addition.  The foreign key constraint checking is fairly
> simple, essentially one "just" need to remove the ONLY from the
> generated check query.
>


To be clear, this would still require a high-level concept of a global
index and the only question is whether it gets stored as multiple
partitions against partitioned tables vs stored in one giant index, right?

Also for foreign key constraints, does it make sense, for
backwards-compatibility reasons to introduce a new syntax for checking all
child tables?



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



-- 
Best Regards,
Chris Travers
Database Administrator

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

Re: [HACKERS] ECPG: WHENEVER statement with DO CONTINUE action

[vinayak]

On 2017/08/25 17:13, Masahiko Sawada wrote:

On Fri, Aug 25, 2017 at 4:27 PM, vinayak
 wrote:


On 2017/08/25 16:18, Masahiko Sawada wrote:

On Fri, Aug 25, 2017 at 2:57 PM, vinayak
 wrote:

Hi Sawada-san,


On 2017/08/25 11:07, Masahiko Sawada wrote:

On Fri, Aug 18, 2017 at 5:20 PM, vinayak
 wrote:

On 2017/06/20 17:35, vinayak wrote:

Hi Sawada-san,

On 2017/06/20 17:22, Masahiko Sawada wrote:

On Tue, Jun 20, 2017 at 1:51 PM, vinayak
 wrote:


On 2017/06/12 13:09, vinayak wrote:

Hi,

On 2017/06/10 12:23, Vinayak Pokale wrote:

Thank you for your reply

On Jun 9, 2017 5:39 PM, "Michael Meskes" 
wrote:

Could you please add a "DO CONTINUE" case to one of the test cases?
Or
add a new one? We would need a test case IMO.


Yes I will add test case and send updated patch.

I have added new test case for DO CONTINUE.
Please check the attached patch.

I have added this in Sept. CF
https://commitfest.postgresql.org/14/1173/


--
In whenever_do_continue.pgc file, the following line seems not to be
processed successfully by ecpg but should we fix that?

+
+   exec sql whenever sqlerror continue;
+

Also, you wrote the test case using "WHENEVER sqlerror DO CONTINUE"
action but that seems not to emit sqlerror, so "DO CONTINUE" is not
executed. I think the test case for DO CONTINUE should be a C code
that executes the "continue" clause.

Thank you for testing the patch.
I agreed with your comments. I will update the patch.

Please check the attached updated patch.


Thank you for updating.

The regression test failed after applied latest patch by git am.

***
/tmp/pg/src/interfaces/ecpg/test/expected/preproc-whenever_do_continue.c
  2017-08-24 20:01:10.023201132 -0700
---
/tmp/pg/src/interfaces/ecpg/test/results/preproc-whenever_do_continue.c
   2017-08-24 20:22:54.308200853 -0700
***
*** 140,147 
   printf("%s %7.2f %9.2f\n", emp.ename, emp.sal,
emp.comm);
   }

!   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
program to
!proceed if any further errors do occur. */
   /* exec sql whenever sqlerror  continue ; */
 #line 53 "whenever_do_continue.pgc"

--- 140,147 
   printf("%s %7.2f %9.2f\n", emp.ename, emp.sal,
emp.comm);
   }

!   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
program to
!   proceed if any further errors do occur. */
   /* exec sql whenever sqlerror  continue ; */
 #line 53 "whenever_do_continue.pgc"

==

+   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
program to
+   proceed if any further errors do occur. */

I think this comment should obey the coding style guide.

Thank you for testing.

I have updated the patch.
PFA.


Thank you for updating the patch. It seems not to incorporate my
second review comment. Attached an updated patch including a fix of a
comment style in whenever_do_continue.pgc file. Please find an
attached file.

Sorry, I missed it.
Thank you for fixing the comment style.


The v3 patch looks good to me. I've changed this patch status to Ready
for Committer.

Thank you for updating the status in the CF.
We can wait for committers feedback.

Regards,
Vinayak Pokale
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] ECPG: WHENEVER statement with DO CONTINUE action

[Masahiko Sawada]

On Fri, Aug 25, 2017 at 4:27 PM, vinayak
 wrote:
>
>
> On 2017/08/25 16:18, Masahiko Sawada wrote:
>>
>> On Fri, Aug 25, 2017 at 2:57 PM, vinayak
>>  wrote:
>>>
>>> Hi Sawada-san,
>>>
>>>
>>> On 2017/08/25 11:07, Masahiko Sawada wrote:

 On Fri, Aug 18, 2017 at 5:20 PM, vinayak
  wrote:
>
> On 2017/06/20 17:35, vinayak wrote:
>>
>> Hi Sawada-san,
>>
>> On 2017/06/20 17:22, Masahiko Sawada wrote:
>>>
>>> On Tue, Jun 20, 2017 at 1:51 PM, vinayak
>>>  wrote:


 On 2017/06/12 13:09, vinayak wrote:

 Hi,

 On 2017/06/10 12:23, Vinayak Pokale wrote:

 Thank you for your reply

 On Jun 9, 2017 5:39 PM, "Michael Meskes" 
 wrote:
>
> Could you please add a "DO CONTINUE" case to one of the test cases?
> Or
> add a new one? We would need a test case IMO.
>
 Yes I will add test case and send updated patch.

 I have added new test case for DO CONTINUE.
 Please check the attached patch.

 I have added this in Sept. CF
 https://commitfest.postgresql.org/14/1173/

>>> --
>>> In whenever_do_continue.pgc file, the following line seems not to be
>>> processed successfully by ecpg but should we fix that?
>>>
>>> +
>>> +   exec sql whenever sqlerror continue;
>>> +
>>>
>>> Also, you wrote the test case using "WHENEVER sqlerror DO CONTINUE"
>>> action but that seems not to emit sqlerror, so "DO CONTINUE" is not
>>> executed. I think the test case for DO CONTINUE should be a C code
>>> that executes the "continue" clause.
>>
>> Thank you for testing the patch.
>> I agreed with your comments. I will update the patch.
>
> Please check the attached updated patch.
>
 Thank you for updating.

 The regression test failed after applied latest patch by git am.

 ***
 /tmp/pg/src/interfaces/ecpg/test/expected/preproc-whenever_do_continue.c
  2017-08-24 20:01:10.023201132 -0700
 ---
 /tmp/pg/src/interfaces/ecpg/test/results/preproc-whenever_do_continue.c
   2017-08-24 20:22:54.308200853 -0700
 ***
 *** 140,147 
   printf("%s %7.2f %9.2f\n", emp.ename, emp.sal,
 emp.comm);
   }

 !   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
 program to
 !proceed if any further errors do occur. */
   /* exec sql whenever sqlerror  continue ; */
 #line 53 "whenever_do_continue.pgc"

 --- 140,147 
   printf("%s %7.2f %9.2f\n", emp.ename, emp.sal,
 emp.comm);
   }

 !   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
 program to
 !   proceed if any further errors do occur. */
   /* exec sql whenever sqlerror  continue ; */
 #line 53 "whenever_do_continue.pgc"

 ==

 +   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
 program to
 +   proceed if any further errors do occur. */

 I think this comment should obey the coding style guide.
>>>
>>> Thank you for testing.
>>>
>>> I have updated the patch.
>>> PFA.
>>>
>> Thank you for updating the patch. It seems not to incorporate my
>> second review comment. Attached an updated patch including a fix of a
>> comment style in whenever_do_continue.pgc file. Please find an
>> attached file.
>
> Sorry, I missed it.
> Thank you for fixing the comment style.
>

The v3 patch looks good to me. I've changed this patch status to Ready
for Committer.

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] Re: [COMMITTERS] pgsql: Fix bug that can cause walsender not to terminating at shutdown.

[Michael Paquier]

On Fri, Aug 25, 2017 at 3:36 PM, Alvaro Herrera
 wrote:
> Andres Freund wrote:
>> Fix bug that can cause walsender not to terminating at shutdown.
>>
>> When backpatching c6c333436 I (Andres Freund) mis-resolved a conflict
>> in the 9.4 branch. Unfortunately that leads to walsenders waiting
>> forever when shutting down with connected standbys, unless immediate
>> mode is used, or the standbys are forced to disconnect by other means.
>
> Hmm, I think we should issue a new 9.4 release with this bug fix.

+1.
-- 
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] obsolete code in pg_upgrade

[Christoph Berg]

Re: Peter Eisentraut 2017-08-24 
<8aa00f15-144e-e793-750e-d1d6876d6...@2ndquadrant.com>
> On 8/23/17 09:36, Robert Haas wrote:
> > I think I agree.  It seems to me that the version of pg_upgrade
> > shipped with release N only needs to support upgrades to release N,
> > not older releases.  There's probably room for debate about whether a
> > pg_upgrade needs to support direct upgrades FROM very old releases,
> > but we need not decide anything about that right now.
> > 
> > I think you could go ahead and rip out this code, but as it seems like
> > a non-critical change, -1 for back-patching it.
> 
> committed to PG10 and master

This #define seems to be obsolete now as well:
src/bin/pg_upgrade/pg_upgrade.h:#define TABLE_SPACE_SUBDIRS_CAT_VER 20100

Christoph


-- 
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] ECPG: WHENEVER statement with DO CONTINUE action

[vinayak]

On 2017/08/25 16:18, Masahiko Sawada wrote:

On Fri, Aug 25, 2017 at 2:57 PM, vinayak
 wrote:

Hi Sawada-san,


On 2017/08/25 11:07, Masahiko Sawada wrote:

On Fri, Aug 18, 2017 at 5:20 PM, vinayak
 wrote:

On 2017/06/20 17:35, vinayak wrote:

Hi Sawada-san,

On 2017/06/20 17:22, Masahiko Sawada wrote:

On Tue, Jun 20, 2017 at 1:51 PM, vinayak
 wrote:


On 2017/06/12 13:09, vinayak wrote:

Hi,

On 2017/06/10 12:23, Vinayak Pokale wrote:

Thank you for your reply

On Jun 9, 2017 5:39 PM, "Michael Meskes" 
wrote:

Could you please add a "DO CONTINUE" case to one of the test cases?
Or
add a new one? We would need a test case IMO.


Yes I will add test case and send updated patch.

I have added new test case for DO CONTINUE.
Please check the attached patch.

I have added this in Sept. CF
https://commitfest.postgresql.org/14/1173/


--
In whenever_do_continue.pgc file, the following line seems not to be
processed successfully by ecpg but should we fix that?

+
+   exec sql whenever sqlerror continue;
+

Also, you wrote the test case using "WHENEVER sqlerror DO CONTINUE"
action but that seems not to emit sqlerror, so "DO CONTINUE" is not
executed. I think the test case for DO CONTINUE should be a C code
that executes the "continue" clause.

Thank you for testing the patch.
I agreed with your comments. I will update the patch.

Please check the attached updated patch.


Thank you for updating.

The regression test failed after applied latest patch by git am.

***
/tmp/pg/src/interfaces/ecpg/test/expected/preproc-whenever_do_continue.c
 2017-08-24 20:01:10.023201132 -0700
---
/tmp/pg/src/interfaces/ecpg/test/results/preproc-whenever_do_continue.c
  2017-08-24 20:22:54.308200853 -0700
***
*** 140,147 
  printf("%s %7.2f %9.2f\n", emp.ename, emp.sal, emp.comm);
  }

!   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
program to
!proceed if any further errors do occur. */
  /* exec sql whenever sqlerror  continue ; */
#line 53 "whenever_do_continue.pgc"

--- 140,147 
  printf("%s %7.2f %9.2f\n", emp.ename, emp.sal, emp.comm);
  }

!   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
program to
!   proceed if any further errors do occur. */
  /* exec sql whenever sqlerror  continue ; */
#line 53 "whenever_do_continue.pgc"

==

+   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
program to
+   proceed if any further errors do occur. */

I think this comment should obey the coding style guide.

Thank you for testing.

I have updated the patch.
PFA.


Thank you for updating the patch. It seems not to incorporate my
second review comment. Attached an updated patch including a fix of a
comment style in whenever_do_continue.pgc file. Please find an
attached file.

Sorry, I missed it.
Thank you for fixing the comment style.

Regards,
Vinayak Pokale
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] ECPG: WHENEVER statement with DO CONTINUE action

[Masahiko Sawada]

On Fri, Aug 25, 2017 at 2:57 PM, vinayak
 wrote:
> Hi Sawada-san,
>
>
> On 2017/08/25 11:07, Masahiko Sawada wrote:
>>
>> On Fri, Aug 18, 2017 at 5:20 PM, vinayak
>>  wrote:
>>>
>>> On 2017/06/20 17:35, vinayak wrote:

 Hi Sawada-san,

 On 2017/06/20 17:22, Masahiko Sawada wrote:
>
> On Tue, Jun 20, 2017 at 1:51 PM, vinayak
>  wrote:
>>
>>
>> On 2017/06/12 13:09, vinayak wrote:
>>
>> Hi,
>>
>> On 2017/06/10 12:23, Vinayak Pokale wrote:
>>
>> Thank you for your reply
>>
>> On Jun 9, 2017 5:39 PM, "Michael Meskes" 
>> wrote:
>>>
>>> Could you please add a "DO CONTINUE" case to one of the test cases?
>>> Or
>>> add a new one? We would need a test case IMO.
>>>
>> Yes I will add test case and send updated patch.
>>
>> I have added new test case for DO CONTINUE.
>> Please check the attached patch.
>>
>> I have added this in Sept. CF
>> https://commitfest.postgresql.org/14/1173/
>>
> --
> In whenever_do_continue.pgc file, the following line seems not to be
> processed successfully by ecpg but should we fix that?
>
> +
> +   exec sql whenever sqlerror continue;
> +
>
> Also, you wrote the test case using "WHENEVER sqlerror DO CONTINUE"
> action but that seems not to emit sqlerror, so "DO CONTINUE" is not
> executed. I think the test case for DO CONTINUE should be a C code
> that executes the "continue" clause.

 Thank you for testing the patch.
 I agreed with your comments. I will update the patch.
>>>
>>> Please check the attached updated patch.
>>>
>> Thank you for updating.
>>
>> The regression test failed after applied latest patch by git am.
>>
>> ***
>> /tmp/pg/src/interfaces/ecpg/test/expected/preproc-whenever_do_continue.c
>> 2017-08-24 20:01:10.023201132 -0700
>> ---
>> /tmp/pg/src/interfaces/ecpg/test/results/preproc-whenever_do_continue.c
>>  2017-08-24 20:22:54.308200853 -0700
>> ***
>> *** 140,147 
>>  printf("%s %7.2f %9.2f\n", emp.ename, emp.sal, emp.comm);
>>  }
>>
>> !   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
>> program to
>> !proceed if any further errors do occur. */
>>  /* exec sql whenever sqlerror  continue ; */
>>#line 53 "whenever_do_continue.pgc"
>>
>> --- 140,147 
>>  printf("%s %7.2f %9.2f\n", emp.ename, emp.sal, emp.comm);
>>  }
>>
>> !   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
>> program to
>> !   proceed if any further errors do occur. */
>>  /* exec sql whenever sqlerror  continue ; */
>>#line 53 "whenever_do_continue.pgc"
>>
>> ==
>>
>> +   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the
>> program to
>> +   proceed if any further errors do occur. */
>>
>> I think this comment should obey the coding style guide.
>
> Thank you for testing.
>
> I have updated the patch.
> PFA.
>

Thank you for updating the patch. It seems not to incorporate my
second review comment. Attached an updated patch including a fix of a
comment style in whenever_do_continue.pgc file. Please find an
attached file.

Regards,

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


0001-WHENEVER-statement-DO-CONTINUE-support_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

[HACKERS] WIP: Separate log file for extension

[Antonin Houska]

Attached is a draft patch to allow extension to write log messages to a
separate file. It introduces a concept of a "log stream". The extension's
shared library gets its stream assigned by calling this function from
_PG_init()

my_stream_id = get_log_stream("my_extension", _log_stream);

Then it's supposed to change some of its attributes

adjust_log_stream_attr(>filename, "my_extension.log");

and to use the stream id in ereport() calls

ereport(LOG, (errmsg("Hello world"), errstream(my_stream_id)));

The EXEC_BACKEND mechanism makes initialization of the log streams by
postmaster child processes non-trivial. I decided to extend
save_backend_variables() and restore_backend_variables() accordingly. Maybe
someone has better idea.

pgaudit seems to be the most obvious use case for this enhancement, but it
might be useful for many other extensions.

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

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
new file mode 100644
index 95180b2..e9b5684
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*** typedef struct
*** 464,469 
--- 464,470 
  
  static pid_t backend_forkexec(Port *port);
  static pid_t internal_forkexec(int argc, char *argv[], Port *port);
+ static Size get_backend_params_size(void);
  
  /* Type for a socket that can be inherited to a client process */
  #ifdef WIN32
*** typedef int InheritableSocket;
*** 482,488 
--- 483,495 
   */
  typedef struct
  {
+ 	/*
+ 	 * read_backend_variables() relies on size to be the first field, followed
+ 	 * by port.
+ 	 */
+ 	Size		size;
  	Port		port;
+ 
  	InheritableSocket portsocket;
  	char		DataDir[MAXPGPATH];
  	pgsocket	ListenSocket[MAXLISTEN];
*** typedef struct
*** 528,533 
--- 535,542 
  	char		my_exec_path[MAXPGPATH];
  	char		pkglib_path[MAXPGPATH];
  	char		ExtraOptions[MAXPGPATH];
+ 	int			nlogstreams;
+ 	char		log_streams[FLEXIBLE_ARRAY_MEMBER];
  } BackendParameters;
  
  static void read_backend_variables(char *id, Port *port);
*** PostmasterMain(int argc, char *argv[])
*** 578,583 
--- 587,593 
  	bool		listen_addr_saved = false;
  	int			i;
  	char	   *output_config_variable = NULL;
+ 	LogStream  *log = _streams[0];
  
  	MyProcPid = PostmasterPid = getpid();
  
*** PostmasterMain(int argc, char *argv[])
*** 1273,1279 
  	 * saying so, to provide a breadcrumb trail for users who may not remember
  	 * that their logging is configured to go somewhere else.
  	 */
! 	if (!(Log_destination & LOG_DESTINATION_STDERR))
  		ereport(LOG,
  (errmsg("ending log output to stderr"),
   errhint("Future log output will go to log destination \"%s\".",
--- 1283,1289 
  	 * saying so, to provide a breadcrumb trail for users who may not remember
  	 * that their logging is configured to go somewhere else.
  	 */
! 	if (!(log->destination & LOG_DESTINATION_STDERR))
  		ereport(LOG,
  (errmsg("ending log output to stderr"),
   errhint("Future log output will go to log destination \"%s\".",
*** internal_forkexec(int argc, char *argv[]
*** 4421,4431 
  	static unsigned long tmpBackendFileNum = 0;
  	pid_t		pid;
  	char		tmpfilename[MAXPGPATH];
! 	BackendParameters param;
  	FILE	   *fp;
  
! 	if (!save_backend_variables(, port))
  		return -1;/* log made by save_backend_variables */
  
  	/* Calculate name for temp file */
  	snprintf(tmpfilename, MAXPGPATH, "%s/%s.backend_var.%d.%lu",
--- 4431,4448 
  	static unsigned long tmpBackendFileNum = 0;
  	pid_t		pid;
  	char		tmpfilename[MAXPGPATH];
! 	Size		param_size;
! 	BackendParameters *param;
  	FILE	   *fp;
  
! 	param_size = get_backend_params_size();
! 	param = (BackendParameters *) palloc(param_size);
! 	if (!save_backend_variables(param, port))
! 	{
! 		pfree(param);
  		return -1;/* log made by save_backend_variables */
+ 	}
+ 	Assert(param->size == param_size);
  
  	/* Calculate name for temp file */
  	snprintf(tmpfilename, MAXPGPATH, "%s/%s.backend_var.%d.%lu",
*** internal_forkexec(int argc, char *argv[]
*** 4449,4466 
  	(errcode_for_file_access(),
  	 errmsg("could not create file \"%s\": %m",
  			tmpfilename)));
  			return -1;
  		}
  	}
  
! 	if (fwrite(, sizeof(param), 1, fp) != 1)
  	{
  		ereport(LOG,
  (errcode_for_file_access(),
   errmsg("could not write to file \"%s\": %m", tmpfilename)));
  		FreeFile(fp);
  		return -1;
  	}
  
  	/* Release file */
  	if (FreeFile(fp))
--- 4466,4486 
  	(errcode_for_file_access(),
  	 errmsg("could not create file \"%s\": %m",
  			tmpfilename)));
+ 			pfree(param);
  			return -1;
  		}
  	}
  
! 	if (fwrite(param, param_size, 1, fp) != 1)
  	{
  		ereport(LOG,
  (errcode_for_file_access(),
   errmsg("could 

Re: [HACKERS] Draft release notes up for review

[Alvaro Herrera]

Tom Lane wrote:
> Jonathan Katz  writes:
> > I see this one
> > > Fix potential data corruption when freezing a tuple whose XMAX is a 
> > multixact with exactly one still-interesting member
> > But I’m unsure how prevalent it is and if it should be highlighted.
> 
> I'm not sure about that either.  I do not think anyone did the legwork
> to determine the exact consequences of that bug, or the probability of
> someone hitting it in the field.  But I think the latter must be really
> low, because we haven't heard any field reports that seem to match up.

My assessment is that that bug is extremely hard to hit.  The main
conditions are, according to FreezeMultiXactId, that
1) the tuple must have a multixact xmax; and
2) the update xid must be newer than the cutoff freeze xid;
3) the multixact itself must be older than the cutoff freeze multi.

so the multixact counter needs to go faster than the xid counter (in
terms of who gets past the freeze age first), and a vacuum freeze must
be attempted on that tuple before the update xid becomes freezable.

The consequence is that the infomask, instead of ending up as

 frz->t_infomask = tuple->t_infomask;
 frz->t_infomask &= ~HEAP_XMAX_BITS;
 |= HEAP_XMAX_COMMITTED;
 tuple->t_infomask = frz->t_infomask;

is instead

 frz->t_infomask = tuple->t_infomask;
 frz->t_infomask &= ~HEAP_XMAX_BITS;
 &= HEAP_XMAX_COMMITTED;
 tuple->t_infomask = frz->t_infomask;

so any bit other than XMAX_COMMITTED is turned off -- which could be
pretty bad for HEAP_HASNULL, etc.

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


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

Re: [HACKERS] proposal: psql command \graw

[Fabien COELHO]

Argh, sorry, I put it in the September commitfest, and it seems that it
cannot be changed afterwards.

Maybe you can close it and redeclare it in the commitfest you want?


It can be moved


Indeed it can. Feel free to move it, then.

--
Fabien.


--
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: [COMMITTERS] pgsql: Fix bug that can cause walsender not to terminating at shutdown.

[Alvaro Herrera]

Andres Freund wrote:
> Fix bug that can cause walsender not to terminating at shutdown.
> 
> When backpatching c6c333436 I (Andres Freund) mis-resolved a conflict
> in the 9.4 branch. Unfortunately that leads to walsenders waiting
> forever when shutting down with connected standbys, unless immediate
> mode is used, or the standbys are forced to disconnect by other means.

Hmm, I think we should issue a new 9.4 release with this bug fix.

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


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

Re: [HACKERS] ECPG: WHENEVER statement with DO CONTINUE action

[vinayak]

Hi Sawada-san,

On 2017/08/25 11:07, Masahiko Sawada wrote:

On Fri, Aug 18, 2017 at 5:20 PM, vinayak
 wrote:

On 2017/06/20 17:35, vinayak wrote:

Hi Sawada-san,

On 2017/06/20 17:22, Masahiko Sawada wrote:

On Tue, Jun 20, 2017 at 1:51 PM, vinayak
 wrote:


On 2017/06/12 13:09, vinayak wrote:

Hi,

On 2017/06/10 12:23, Vinayak Pokale wrote:

Thank you for your reply

On Jun 9, 2017 5:39 PM, "Michael Meskes"  wrote:

Could you please add a "DO CONTINUE" case to one of the test cases? Or
add a new one? We would need a test case IMO.


Yes I will add test case and send updated patch.

I have added new test case for DO CONTINUE.
Please check the attached patch.

I have added this in Sept. CF
https://commitfest.postgresql.org/14/1173/


--
In whenever_do_continue.pgc file, the following line seems not to be
processed successfully by ecpg but should we fix that?

+
+   exec sql whenever sqlerror continue;
+

Also, you wrote the test case using "WHENEVER sqlerror DO CONTINUE"
action but that seems not to emit sqlerror, so "DO CONTINUE" is not
executed. I think the test case for DO CONTINUE should be a C code
that executes the "continue" clause.

Thank you for testing the patch.
I agreed with your comments. I will update the patch.

Please check the attached updated patch.


Thank you for updating.

The regression test failed after applied latest patch by git am.

*** /tmp/pg/src/interfaces/ecpg/test/expected/preproc-whenever_do_continue.c
2017-08-24 20:01:10.023201132 -0700
--- /tmp/pg/src/interfaces/ecpg/test/results/preproc-whenever_do_continue.c
 2017-08-24 20:22:54.308200853 -0700
***
*** 140,147 
 printf("%s %7.2f %9.2f\n", emp.ename, emp.sal, emp.comm);
 }

!   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the program to
!proceed if any further errors do occur. */
 /* exec sql whenever sqlerror  continue ; */
   #line 53 "whenever_do_continue.pgc"

--- 140,147 
 printf("%s %7.2f %9.2f\n", emp.ename, emp.sal, emp.comm);
 }

!   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the program to
!   proceed if any further errors do occur. */
 /* exec sql whenever sqlerror  continue ; */
   #line 53 "whenever_do_continue.pgc"

==

+   /* This 'CONTINUE' shuts off the 'DO CONTINUE' and allow the program to
+   proceed if any further errors do occur. */

I think this comment should obey the coding style guide.

Thank you for testing.

I have updated the patch.
PFA.

Regards,
Vinayak Pokale
NTT Open Source Software Center
>From cd71bf7229a8566cadfde3d0e89b1b445baf1fee Mon Sep 17 00:00:00 2001
From: Vinayak Pokale 
Date: Thu, 22 Jun 2017 11:08:38 +0900
Subject: [PATCH] WHENEVER statement DO CONTINUE support

---
 doc/src/sgml/ecpg.sgml |   12 ++
 src/interfaces/ecpg/preproc/ecpg.trailer   |6 +
 src/interfaces/ecpg/preproc/output.c   |3 +
 src/interfaces/ecpg/test/ecpg_schedule |1 +
 .../test/expected/preproc-whenever_do_continue.c   |  159 
 .../expected/preproc-whenever_do_continue.stderr   |  112 ++
 .../expected/preproc-whenever_do_continue.stdout   |2 +
 src/interfaces/ecpg/test/preproc/Makefile  |1 +
 .../ecpg/test/preproc/whenever_do_continue.pgc |   61 
 9 files changed, 357 insertions(+), 0 deletions(-)
 create mode 100644 src/interfaces/ecpg/test/expected/preproc-whenever_do_continue.c
 create mode 100644 src/interfaces/ecpg/test/expected/preproc-whenever_do_continue.stderr
 create mode 100644 src/interfaces/ecpg/test/expected/preproc-whenever_do_continue.stdout
 create mode 100644 src/interfaces/ecpg/test/preproc/whenever_do_continue.pgc

diff --git a/doc/src/sgml/ecpg.sgml b/doc/src/sgml/ecpg.sgml
index f13a0e9..3cb4001 100644
--- a/doc/src/sgml/ecpg.sgml
+++ b/doc/src/sgml/ecpg.sgml
@@ -4763,6 +4763,17 @@ EXEC SQL WHENEVER condition action
 
  
+  DO CONTINUE
+  
+   
+Execute the C statement continue.  This should
+only be used in loops statements.  if executed, will cause the flow 
+of control to return to the top of the loop.
+   
+  
+ 
+
+ 
   CALL name (args)
   DO name (args)
   
@@ -7799,6 +7810,7 @@ WHENEVER { NOT FOUND | SQLERROR | SQLWARNING } ac
 
 EXEC SQL WHENEVER NOT FOUND CONTINUE;
 EXEC SQL WHENEVER NOT FOUND DO BREAK;
+EXEC SQL WHENEVER NOT FOUND DO CONTINUE;
 EXEC SQL WHENEVER SQLWARNING SQLPRINT;
 EXEC SQL WHENEVER SQLWARNING DO warn();
 EXEC SQL WHENEVER SQLERROR sqlprint;
diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer b/src/interfaces/ecpg/preproc/ecpg.trailer
index 1c10879..b42bca4 100644
--- a/src/interfaces/ecpg/preproc/ecpg.trailer
+++