Re: [HACKERS] Dangling Client Backend Process

[Rajeev rastogi]

On 10 October 2015 20:45, Amit Kapila Wrote:

>> I observed one strange behavior today that if postmaster process gets 
>> crashed/killed, then it kill all background processes but not the client 
>> backend process.

> This is a known behaviour and there was some discussion on this
> topic [1] previously as well.  I think that thread didn't reach to conclusion,
> but there were couple of other reasons discussed in that thread as well to
> have the behaviour as you are proposing here.

Oops..I did not know about this. I shall check the older thread to get other 
opinions.

>> One way to handle this issue will be to check whether postmaster is alive 
>> after every command read but it will add extra cost for each query execution.

> I don't think that is a good idea as if there is no command execution
> it will still stay as it is and doing such operations on each command
> doesn't sound to be good idea even though overhead might not be
> big.  There are some other ideas discussed in that thread [2] to achieve
> this behaviour, but I think we need to find a portable way to achieve it.

Yes, you are right that process will not be closed till a new command comes but 
I think it does not harm functionality in anyway except that the process and 
its acquired resources
does not get freed. Also use-case of application will be very less where their 
client process stays idle for very long time.
But at the same time I agree this is not the best solution, we should look for 
more appropriate/better one.
Now as it is confirmed to be valid issue, I will spend some time on this to 
find if there is something more appropriate solution.

Thanks and Regards,
Kumar Rajeev Rastogi

Re: [HACKERS] More work on SortSupport for text - strcoll() and strxfrm() caching

[Peter Geoghegan]

On Sat, Oct 10, 2015 at 12:32 PM, Peter Geoghegan  wrote:
> I noticed that there is still one comment that I really should have
> removed as part of this work.

I also noticed that I failed to reset the last_returned strcoll()
cache variable as part of an abbreviation call, despite the fact that
tapesort may freely interleave conversions with comparisons, while
reusing buf1 and buf2 both as scratch space for strxfrm() blobs, as
well as for storing strings to be compared with strcoll(). I suggest
that the attached patch also be applied to fix this issue.

-- 
Peter Geoghegan
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index d545c34..9725a5f 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -2173,6 +2173,12 @@ bttext_abbrev_convert(Datum original, SortSupport ssup)
 
 done:
 	/*
+	 * Set last_returned to sentinel value.  Comparisons that attempt to reuse
+	 * last_returned may be interleaved with calls here.
+	 */
+	tss->last_returned = INT_MIN;
+
+	/*
 	 * Byteswap on little-endian machines.
 	 *
 	 * This is needed so that bttextcmp_abbrev() (an unsigned integer 3-way

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

Re: [HACKERS] point_ops for GiST

[Alexander Korotkov]

Hi, Stefan!

On Sun, Oct 11, 2015 at 10:00 PM, Stefan Keller  wrote:

> Pls. don't misunderstand my questions: They are directed to get an
> even more useful spatial data handling of PostgreSQL. I'm working with
> PostGIS since years and are interested in any work regarding spatial
> types...
>
> Can anyone report use cases or applications of these built-in geometric
> types?
>
> Would'nt it be even more useful to concentrate to PostGIS
> geometry/geography types and extend BRIN to these types?
>

Note, that PostGIS is a different project which is maintained by separate
team. PostGIS have its own priorities, development plan etc.
PostgreSQL have to be self-consistent. In particular, it should have
reference implementation of operator classes which extensions can use as
the pattern. This is why it's important to maintain built-in geometric
types.

In short: once we implement it for built-in geometric types, you can ask
PostGIS team to do the same for their geometry/geography.

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

Re: [HACKERS] point_ops for GiST

[Alexander Korotkov]

On Mon, Oct 12, 2015 at 12:47 PM, Emre Hasegeli  wrote:

> > This was already fixed for GiST.
> > See following discussion
> >
> http://www.postgresql.org/message-id/capphfdvgticgniaj88vchzhboxjobuhjlm6c09q_op_u9eo...@mail.gmail.com
> > and commit
> >
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3c29b196b0ce46662cb9bb7a1f91079fbacbcabb
> > "Consistent" method of GiST influences only search and can't lead to
> corrupt
> > indexes. However, "same" method can lead to corrupt indexes.
> > However, this is not the reason to not backpatch the changes and preserve
> > buggy behaviour; this is the reason to recommend reindexing to users.
> And it
> > was already backpatched.
>
> Fixing it on the opclass is not an option for BRIN.  We designed BRIN
> opclasses extensible using extra SQL level support functions and
> operators.  It is possible to support point datatype using box vs
> point operators.  Doing so would lead to wrong results, because point
> operators use FP macros, but box_contain_pt() doesn't.
>

You still can workaround this problem in opclass. For instance, you can
assign different strategy number for this case. And call another support
function instead of overlap operator in brin_inclusion_consistent. For
sure, this would be a kluge.


> GiST opclass could be more clean and extensible, if we wouldn't have
> those macros.
>

In my opinion it would be cool remove FP macros. I see absolutely no sense
in them. But since it break compatibility it would be quite hard though.

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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Magnus Hagander]

On Mon, Oct 12, 2015 at 12:25 PM, Andres Freund  wrote:

> On 2015-10-12 11:25:35 +0530, Amit Kapila wrote:
> >   /*
> > +  * Close the shared memory handle as the syslogger doesn't need to
> > +  * attach to it.  For EXEC_BACKEND case, the shared memory handle
> > +  * is inherited by all postmaster child processes irrespective of
> > +  * whether they need it or not.
> > +  */
> > +#ifdef EXEC_BACKEND
> > + if (!CloseHandle(UsedShmemSegID))
> > + elog(LOG, "could not close handle to shared memory: error
> code %lu", GetLastError());
> > +#endif
> > +
>
> It feels wrong to do this in syslogger.c - I mean it's not the only
> process that's not attached to shared memory. Sure, the others get
> killed, but nonetheless...
>

+1. It feels like we're setting our selves up for repeating this mistake at
some later time :)

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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Andres Freund]

On 2015-10-12 11:25:35 +0530, Amit Kapila wrote:
>   /*
> +  * Close the shared memory handle as the syslogger doesn't need to
> +  * attach to it.  For EXEC_BACKEND case, the shared memory handle
> +  * is inherited by all postmaster child processes irrespective of
> +  * whether they need it or not.
> +  */
> +#ifdef EXEC_BACKEND
> + if (!CloseHandle(UsedShmemSegID))
> + elog(LOG, "could not close handle to shared memory: error code 
> %lu", GetLastError());
> +#endif
> +

It feels wrong to do this in syslogger.c - I mean it's not the only
process that's not attached to shared memory. Sure, the others get
killed, but nonetheless...

Greetings,

Andres Freund


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

Re: [HACKERS] point_ops for GiST

[Emre Hasegeli]

>> Pls. don't misunderstand my questions: They are directed to get an
>> even more useful spatial data handling of PostgreSQL. I'm working with
>> PostGIS since years and are interested in any work regarding spatial
>> types...
>>
>> Can anyone report use cases or applications of these built-in geometric
>> types?
>>
>> Would'nt it be even more useful to concentrate to PostGIS
>> geometry/geography types and extend BRIN to these types?
>
>
> Note, that PostGIS is a different project which is maintained by separate
> team. PostGIS have its own priorities, development plan etc.
> PostgreSQL have to be self-consistent. In particular, it should have
> reference implementation of operator classes which extensions can use as the
> pattern. This is why it's important to maintain built-in geometric types.
>
> In short: once we implement it for built-in geometric types, you can ask
> PostGIS team to do the same for their geometry/geography.

The problem is that geometric types don't even serve well to this
purpose in their current state.  We have to make the operators
consistent to better demonstrate index support of cross type
operators.


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

Re: [HACKERS] point_ops for GiST

[Alexander Korotkov]

Hi, Alvaro!

On Tue, May 12, 2015 at 9:13 PM, Alvaro Herrera 
wrote:

> Robert Haas wrote:
> > 2009/12/30 Teodor Sigaev :
> > > Sync with current CVS
> >
> > I have reviewed this patch and it looks good to me.  The only
> > substantive question I have is why gist_point_consistent() uses a
> > different coding pattern for the box case than it does for the polygon
> > and circle cases?  It's not obvious to me on the face of it why these
> > aren't consistent.
>
> Emre Hasegeli just pointed out to me that this patch introduced
> box_contain_pt() and in doing so used straight C comparison (<= etc)
> instead of FPlt() and friends.  I would think that that's a bug and
> needs to be changed -- but certainly not backpatched, because gist
> indexes would/might become corrupt.
>

This was already fixed for GiST.
See following discussion
http://www.postgresql.org/message-id/capphfdvgticgniaj88vchzhboxjobuhjlm6c09q_op_u9eo...@mail.gmail.com
and
commit
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3c29b196b0ce46662cb9bb7a1f91079fbacbcabb
"Consistent" method of GiST influences only search and can't lead to
corrupt indexes. However, "same" method can lead to corrupt indexes.
However, this is not the reason to not backpatch the changes and preserve
buggy behaviour; this is the reason to recommend reindexing to users. And
it was already backpatched.

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

Re: [HACKERS] point_ops for GiST

[Emre Hasegeli]

> This was already fixed for GiST.
> See following discussion
> http://www.postgresql.org/message-id/capphfdvgticgniaj88vchzhboxjobuhjlm6c09q_op_u9eo...@mail.gmail.com
> and commit
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3c29b196b0ce46662cb9bb7a1f91079fbacbcabb
> "Consistent" method of GiST influences only search and can't lead to corrupt
> indexes. However, "same" method can lead to corrupt indexes.
> However, this is not the reason to not backpatch the changes and preserve
> buggy behaviour; this is the reason to recommend reindexing to users. And it
> was already backpatched.

Fixing it on the opclass is not an option for BRIN.  We designed BRIN
opclasses extensible using extra SQL level support functions and
operators.  It is possible to support point datatype using box vs
point operators.  Doing so would lead to wrong results, because point
operators use FP macros, but box_contain_pt() doesn't.

GiST opclass could be more clean and extensible, if we wouldn't have
those macros.


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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Amit Kapila]

On Mon, Oct 12, 2015 at 3:45 PM, Michael Paquier 
wrote:
>
> On Mon, Oct 12, 2015 at 2:55 PM, Amit Kapila 
wrote:
> > On Sun, Oct 11, 2015 at 9:12 PM, Tom Lane  wrote:
> > I could easily reproduce the issue if logging collector is on and even
if
> > we try to increase the loop count or sleep time in
PGSharedMemoryCreate(),
> > it doesn't change the situation as the syslogger has a valid handle to
> > shared memory.  One way to fix is to just close the shared memory handle
> > in sys logger as we are not going to need it and attached patch which
does
> > this fixes the issue for me.  Another invasive fix in case we want to
> > retain shared memory handle for some purpose (future requirement) could
> > be to send some signal to syslogger in restart path so that it can
release
> > the shared memory handle.
>
> +#ifdef EXEC_BACKEND
> +if (!CloseHandle(UsedShmemSegID))
> +elog(LOG, "could not close handle to shared memory: error
> code %lu", GetLastError());
> +#endif
> I am pretty sure that you would want a WIN32 block here, not
> EXEC_BACKEND as the latter can be used on non-Windows platforms as
> well to emulate Windows behavior.
>

Agreed, I can change the patch to use WIN32, but it seems not all
people want to follow this approach.  So lets first try to see what
is the best way to fix.


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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Michael Paquier]

On Mon, Oct 12, 2015 at 2:55 PM, Amit Kapila  wrote:
> On Sun, Oct 11, 2015 at 9:12 PM, Tom Lane  wrote:
> I could easily reproduce the issue if logging collector is on and even if
> we try to increase the loop count or sleep time in PGSharedMemoryCreate(),
> it doesn't change the situation as the syslogger has a valid handle to
> shared memory.  One way to fix is to just close the shared memory handle
> in sys logger as we are not going to need it and attached patch which does
> this fixes the issue for me.  Another invasive fix in case we want to
> retain shared memory handle for some purpose (future requirement) could
> be to send some signal to syslogger in restart path so that it can release
> the shared memory handle.

+#ifdef EXEC_BACKEND
+if (!CloseHandle(UsedShmemSegID))
+elog(LOG, "could not close handle to shared memory: error
code %lu", GetLastError());
+#endif
I am pretty sure that you would want a WIN32 block here, not
EXEC_BACKEND as the latter can be used on non-Windows platforms as
well to emulate Windows behavior.
-- 
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] Parallel Seq Scan

[Haribabu Kommi]

On Mon, Oct 5, 2015 at 11:20 PM, Amit Kapila  wrote:
> For now, I have fixed this by not preserving the startblock incase of rescan
> for parallel scan. Note that, I have created a separate patch
> (parallel_seqscan_heaprescan_v1.patch) for support of rescan (for parallel
> scan).

while testing parallel seqscan, My colleague Jing Wang has found a problem in
parallel_seqscan_heapscan_v2.patch.

In function initscan, the allow_sync flag is set to false as the
number of pages in the
table are less than NBuffers/4.

if (!RelationUsesLocalBuffers(scan->rs_rd) &&
  scan->rs_nblocks > NBuffers / 4)

As allow_sync flag is false, the function
heap_parallelscan_initialize_startblock is not
called in initscan function to initialize the
parallel_scan->phs_cblock parameter. Because
of this reason while getting the next page in
heap_parallelscan_nextpage, it returns
InvalidBlockNumber, thus it ends the scan without returning the results.


Regards,
Hari Babu
Fujitsu Australia


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

Re: [HACKERS] Improve the concurency of vacuum full table and select statement on the same relation

[Jim Nasby]

On 10/11/15 6:55 AM, Jinyu wrote:

Are there other solutions to improve the concurency of vacuum
full/cluster and select statement on the same relation?


ISTM that if we were going to put effort into this it makes more sense 
to pull pg_repack into core. BTW, it's approach to this is to summarily 
kill anything that attempts DDL on a table being repacked.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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] Parallel Seq Scan

[Robert Haas]

On Sun, Oct 11, 2015 at 7:56 PM, Noah Misch  wrote:
> I see no mention in this thread of varatt_indirect, but I anticipated
> datumSerialize() reacting to it the same way datumCopy() reacts.  If
> datumSerialize() can get away without doing so, why is that?

Good point.  I don't think it can.  Attached is a patch to fix that.
This patch also includes some somewhat-related changes to
plpgsql_param_fetch() upon which I would appreciate any input you can
provide.

plpgsql_param_fetch() assumes that it can detect whether it's being
called from copyParamList() by checking whether params !=
estate->paramLI.  I don't know why this works, but I do know that this
test fails to detect the case where it's being called from
SerializeParamList(), which causes failures in exec_eval_datum() as
predicted.  Calls from SerializeParamList() need the same treatment as
calls from copyParamList() because it, too, will try to evaluate every
parameter in the list.  Here, I've taken the approach of making that
check unconditional, which seems to work, but I'm not sure if some
other approach would be better, such as adding an additional Boolean
(or enum context?) argument to ParamFetchHook.  I *think* that
skipping this check is merely a performance optimization rather than
anything that affects correctness, and bms_is_member() is pretty
cheap, so perhaps the way I've done it is OK.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c
index 3d9e354..0d61950 100644
--- a/src/backend/utils/adt/datum.c
+++ b/src/backend/utils/adt/datum.c
@@ -264,6 +264,11 @@ datumEstimateSpace(Datum value, bool isnull, bool typByVal, int typLen)
 		/* no need to use add_size, can't overflow */
 		if (typByVal)
 			sz += sizeof(Datum);
+		else if (VARATT_IS_EXTERNAL_EXPANDED(value))
+		{
+			ExpandedObjectHeader *eoh = DatumGetEOHP(value);
+			sz += EOH_get_flat_size(eoh);
+		}
 		else
 			sz += datumGetSize(value, typByVal, typLen);
 	}
@@ -292,6 +297,7 @@ void
 datumSerialize(Datum value, bool isnull, bool typByVal, int typLen,
 			   char **start_address)
 {
+	ExpandedObjectHeader *eoh = NULL;
 	int		header;
 
 	/* Write header word. */
@@ -299,6 +305,11 @@ datumSerialize(Datum value, bool isnull, bool typByVal, int typLen,
 		header = -2;
 	else if (typByVal)
 		header = -1;
+	else if (VARATT_IS_EXTERNAL_EXPANDED(value))
+	{
+		eoh = DatumGetEOHP(value);
+		header = EOH_get_flat_size(eoh);
+	}
 	else
 		header = datumGetSize(value, typByVal, typLen);
 	memcpy(*start_address, , sizeof(int));
@@ -312,6 +323,11 @@ datumSerialize(Datum value, bool isnull, bool typByVal, int typLen,
 			memcpy(*start_address, , sizeof(Datum));
 			*start_address += sizeof(Datum);
 		}
+		else if (eoh)
+		{
+			EOH_flatten_into(eoh, (void *) *start_address, header);
+			*start_address += header;
+		}
 		else
 		{
 			memcpy(*start_address, DatumGetPointer(value), header);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index c73f20b..346e8f8 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -5696,21 +5696,17 @@ plpgsql_param_fetch(ParamListInfo params, int paramid)
 	/* now we can access the target datum */
 	datum = estate->datums[dno];
 
-	/* need to behave slightly differently for shared and unshared arrays */
-	if (params != estate->paramLI)
-	{
-		/*
-		 * We're being called, presumably from copyParamList(), for cursor
-		 * parameters.  Since copyParamList() will try to materialize every
-		 * single parameter slot, it's important to do nothing when asked for
-		 * a datum that's not supposed to be used by this SQL expression.
-		 * Otherwise we risk failures in exec_eval_datum(), not to mention
-		 * possibly copying a lot more data than the cursor actually uses.
-		 */
-		if (!bms_is_member(dno, expr->paramnos))
-			return;
-	}
-	else
+	/*
+	 * Since copyParamList() and SerializeParamList() will try to materialize
+	 * every single parameter slot, it's important to do nothing when asked for
+	 * a datum that's not supposed to be used by this SQL expression.
+	 * Otherwise we risk failures in exec_eval_datum(), not to mention
+	 * possibly copying a lot more data than the cursor actually uses.
+	 */
+	if (!bms_is_member(dno, expr->paramnos))
+		return;
+
+	if (params == estate->paramLI)
 	{
 		/*
 		 * Normal evaluation cases.  We don't need to sanity-check dno, but we

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

[HACKERS] pam auth - add rhost item

[kolo hhmow]

Wheter it would be a problem to set additional item (rhost) before
pam_authentication function in backend/libpq/auth.c?
It is very useful because you can restrict access to given ip address like
in mysql.
And this actually utilized in pam-pgsql, wich cannot be used because rhost
item is empty.

Thanks.
Grzegorz.

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Mon, Oct 12, 2015 at 11:51 AM, Haribabu Kommi 
wrote:
>
> On Mon, Oct 5, 2015 at 11:20 PM, Amit Kapila 
wrote:
> > For now, I have fixed this by not preserving the startblock incase of
rescan
> > for parallel scan. Note that, I have created a separate patch
> > (parallel_seqscan_heaprescan_v1.patch) for support of rescan (for
parallel
> > scan).
>
> while testing parallel seqscan, My colleague Jing Wang has found a
problem in
> parallel_seqscan_heapscan_v2.patch.
>

Thanks for spotting the issue.

> In function initscan, the allow_sync flag is set to false as the
> number of pages in the
> table are less than NBuffers/4.
>
> if (!RelationUsesLocalBuffers(scan->rs_rd) &&
>   scan->rs_nblocks > NBuffers / 4)
>
> As allow_sync flag is false, the function
> heap_parallelscan_initialize_startblock is not
> called in initscan function to initialize the
> parallel_scan->phs_cblock parameter. Because
> of this reason while getting the next page in
> heap_parallelscan_nextpage, it returns
> InvalidBlockNumber, thus it ends the scan without returning the results.
>

Right, it should initialize parallel scan properly even for non-synchronized
scans.  Fixed the issue in attached patch.  Rebased heap rescan is
attached as well.


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


parallel_seqscan_heapscan_v3.patch
Description: Binary data


parallel_seqscan_heaprescan_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] Postgres service stops when I kill client backend on Windows

[Michael Paquier]

On Mon, Oct 12, 2015 at 7:26 PM, Magnus Hagander  wrote:
>
>
> On Mon, Oct 12, 2015 at 12:25 PM, Andres Freund  wrote:
>>
>> On 2015-10-12 11:25:35 +0530, Amit Kapila wrote:
>> >   /*
>> > +  * Close the shared memory handle as the syslogger doesn't need to
>> > +  * attach to it.  For EXEC_BACKEND case, the shared memory handle
>> > +  * is inherited by all postmaster child processes irrespective of
>> > +  * whether they need it or not.
>> > +  */
>> > +#ifdef EXEC_BACKEND
>> > + if (!CloseHandle(UsedShmemSegID))
>> > + elog(LOG, "could not close handle to shared memory: error
>> > code %lu", GetLastError());
>> > +#endif
>> > +
>>
>> It feels wrong to do this in syslogger.c - I mean it's not the only
>> process that's not attached to shared memory. Sure, the others get
>> killed, but nonetheless...
>
>
> +1. It feels like we're setting our selves up for repeating this mistake at
> some later time :)

Actually, doesn't this apply as well to the archiver and the pgstat
collector? So perhaps we may want to do that in SubPostmasterMain with
PGSharedMemoryDetach. See for example the attached as an idea (patch
completely untested).
-- 
Michael
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 24e8404..2076d96 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4637,6 +4637,16 @@ SubPostmasterMain(int argc, char *argv[])
 		strncmp(argv[1], "--forkbgworker=", 15) == 0)
 		PGSharedMemoryReAttach();
 
+	/*
+	 * Close any existing shared memory segment as those processes do not
+	 * need to have an access to it. This state is inherited from the
+	 * postmaster whether they need it or not.
+	 */
+	if (strcmp(argv[1], "--forkarch") == 0 ||
+		strcmp(argv[1], "--forkcol") == 0 ||
+		strcmp(argv[1], "--forklog") == 0)
+		PGSharedMemoryDetach();
+
 	/* autovacuum needs this set before calling InitProcess */
 	if (strcmp(argv[1], "--forkavlauncher") == 0)
 		AutovacuumLauncherIAm();

-- 
Sent 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 v1] GSSAPI encryption support

[Michael Paquier]

On Sat, Oct 10, 2015 at 3:10 AM, Robbie Harwood wrote:
> Michael Paquier writes:
>>> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
>>> [Andres' comments]
>>
>> Here are some comments on top of what Andres has mentioned.
>>
>> --- a/configure.in
>> +++ b/configure.in
>> @@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI 
>> support],
>>krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab"
>>  ])
>>  AC_MSG_RESULT([$with_gssapi])
>> +AC_SUBST(with_gssapi)
>>
>> I think that using a new configure variable like that with a dedicated
>> file fe-secure-gss.c and be-secure-gss.c has little sense done this
>> way, and that it would be more adapted to get everything grouped in
>> fe-auth.c for the frontend and auth.c for the backend, or move all the
>> GSSAPI-related stuff in its own file. I can understand the move
>> though: this is to imitate OpenSSL in a way somewhat similar to what
>> has been done for it with a rather generic set if routines, but with
>> GSSAPI that's a bit different, we do not have such a set of routines,
>> hence based on this argument moving it to its own file has little
>> sense. Now, a move that would make sense though is to move all the
>> GSSAPI stuff in its own file, for example pg_GSS_recvauth and
>> pg_GSS_error for the backend, and you should do the same for the
>> frontend with all the pg_GSS_* routines. This should be as well a
>> refactoring patch on top of the actual feature.
>
> My understanding is that frontend and backend code need to be separate
> (for linking), so it's automatically in two places. I really don't want
> to put encryption-related code in files called "auth.c" and "fe-auth.c"
> since those files are presumably for authentication, not encryption.
>
> I'm not sure what you mean about "rather generic set if routines";
> GSSAPI is a RFC-standardized interface.  I think I also don't understand
> the last half of your above paragraph.

src/interfaces/libpq/fe-auth.c contains the following set of routines
related to GSS (frontend code in libpq):
- pg_GSS_error_int
- pg_GSS_error
- pg_GSS_continue
- pg_GSS_startup
src/backend/libpq/auth.c contains the following routines related to
GSS (backend code):
- pg_GSS_recvauth
- pg_GSS_error
My point would be simply to move all those routines in two new files
dedicated to GSS, then add your new routines for encryption in it.
Still, the only reason why the OpenSSL routines have been moved out of
be-secure.c to be-secure-openssl.c is to allow other libraries to be
plugged into that, the primary target being SChannel on Windows. And
that's not the case of GSS, so I think that the separation done as in
your patch is not adapted.

>> diff --git a/src/interfaces/libpq/fe-secure-gss.c
>> b/src/interfaces/libpq/fe-secure-gss.c
>> new file mode 100644
>> index 000..afea9c3
>> --- /dev/null
>> +++ b/src/interfaces/libpq/fe-secure-gss.c
>> @@ -0,0 +1,92 @@
>> +#include 
>> You should add a proper header to those new files.
>
> Sorry, what?

All the files in the source tree need to have a header like that:
/*-
 *
 * file_name.c
 *  Description
 *
 * Portions Copyright (c) 2015, PostgreSQL Global Development Group
 *
 * IDENTIFICATION
 *   path/to/file/file_name.c
 *
 *-
 */
-- 
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] Postgres service stops when I kill client backend on Windows

[Andres Freund]

On 2015-10-12 21:38:12 +0900, Michael Paquier wrote:
> >> It feels wrong to do this in syslogger.c - I mean it's not the only
> >> process that's not attached to shared memory. Sure, the others get
> >> killed, but nonetheless...
> >
> >
> > +1. It feels like we're setting our selves up for repeating this mistake at
> > some later time :)
> 
> Actually, doesn't this apply as well to the archiver and the pgstat
> collector?

As mentioned above? The difference is that the archiver et al get killed
by postmaster during a PANIC restart thus don't present the problem
discussed here.

> So perhaps we may want to do that in SubPostmasterMain with
> PGSharedMemoryDetach. See for example the attached as an idea (patch
> completely untested).

> + /*
> +  * Close any existing shared memory segment as those processes do not
> +  * need to have an access to it. This state is inherited from the
> +  * postmaster whether they need it or not.
> +  */
> + if (strcmp(argv[1], "--forkarch") == 0 ||
> + strcmp(argv[1], "--forkcol") == 0 ||
> + strcmp(argv[1], "--forklog") == 0)
> + PGSharedMemoryDetach();
> +

Well, in those cases we won't have attached to shared memory, so I'm not
convinced that this is the right solution. In fact, won't this lead to
hitting the elog in
void
PGSharedMemoryDetach(void)
{
if (UsedShmemSegAddr != NULL)
{
if (!UnmapViewOfFile(UsedShmemSegAddr))
elog(LOG, "could not unmap view of shared memory: error 
code %lu", GetLastError());

UsedShmemSegAddr = NULL;
}
}
UsedShmemSegAddr will have been setup by read_backend_variables(), but
the process won't have anything mapped at this point?

Greetings,

Andres Freund


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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Dmitry Vasilyev]

Hello, Amit!
On Пн, 2015-10-12 at 11:25 +0530, Amit Kapila wrote:
> On Sun, Oct 11, 2015 at 9:12 PM, Tom Lane  wrote:
> >
> > Magnus Hagander  writes:
> > > On Sun, Oct 11, 2015 at 5:22 PM, Tom Lane 
> wrote:
> > >> I'm a bit suspicious that we may have leaked a handle to the
> shared
> > >> memory block someplace, for example.  That would explain why
> this
> > >> symptom is visible now when it was not in 2009.  Or maybe it's
> dependent
> > >> on some feature that we didn't test back then --- for instance,
> if
> > >> the logging collector is in use, could it have inherited a
> handle and
> > >> not closed it?
> >
> > > Even if we leaked it, it should go away when the other processes
> died.
> >
> > I'm fairly certain that we do not kill/restart the logging
> collector
> > during a database restart (because it's impossible to reproduce the
> > original stderr destination if we do).  
> 
> True and it seems this is the reason for issue we are discussing
> here.
> The reason why this happens is that during creation of shared memory
> (PGSharedMemoryCreate()), we duplicate the handle such that it
> become inheritable by all child processes.  Then during fork
> (syslogger_forkexec()->postmaster_forkexec()->internal_forkexec) we
> always inherit the handles which causes syslogger to get a copy of
> shared memory handle which it neither uses and nor closes it.
> 
> I could easily reproduce the issue if logging collector is on and
> even if
> we try to increase the loop count or sleep time
> in PGSharedMemoryCreate(),
> it doesn't change the situation as the syslogger has a valid handle
> to
> shared memory.  One way to fix is to just close the shared memory
> handle
> in sys logger as we are not going to need it and attached patch which
> does
> this fixes the issue for me.  Another invasive fix in case we want to
> retain shared memory handle for some purpose (future requirement)
> could
> be to send some signal to syslogger in restart path so that it can
> release
> the shared memory handle.
> 
> 
> 
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
Specified patch with "ifdef WIN32" is working for me. Maybe it’s
necessary to check open handlers from replication for example?
--
Dmitry Vasilyev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Andres Freund]

On 2015-10-12 10:04:55 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2015-10-12 21:38:12 +0900, Michael Paquier wrote:
> >> Actually, doesn't this apply as well to the archiver and the pgstat
> >> collector?
> 
> > As mentioned above? The difference is that the archiver et al get killed
> > by postmaster during a PANIC restart thus don't present the problem
> > discussed here.
> 
> I thought your objection to the original patch was exactly that we should
> not treat syslogger as a special case for this purpose.

Yes. The above was just about this not being actively broken - I'd
mentioned the other processes before and to me it sounded like Michael
thought there might be an active problem.

> > Well, in those cases we won't have attached to shared memory, so I'm not
> > convinced that this is the right solution.
> 
> No, you're missing the point.

Don't think so.

> In Windows builds, child processes inherit
> a "handle" reference to the shared memory mapping, whether or not they
> make any use of the handle to re-attach to that shared memory.  The point
> here is that we need to close that handle if we're not going to use it.

Right. But that doesn't mean it's right to call PGSharedMemoryDetach()
without other changes as done in Michael's proposed patch? That'll do an
UnmapViewOfFile() which'll fail because nothing i mapped, but still not
close UsedShmemSegID?

Greetings,

Andres Freund


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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Tom Lane]

Andres Freund  writes:
> Right. But that doesn't mean it's right to call PGSharedMemoryDetach()
> without other changes as done in Michael's proposed patch? That'll do an
> UnmapViewOfFile() which'll fail because nothing i mapped, but still not
> close UsedShmemSegID?

Ah, right, I'd not noticed that he proposed changing
CloseHandle(UsedShmemSegID) to PGSharedMemoryDetach().  The latter is
clearly the wrong thing.

I'm not sure whether we should just put the CloseHandle call in
postmaster.c, or invent a function in win32_shmem.c to provide a
layer of abstraction.

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] Postgres service stops when I kill client backend on Windows

[Amit Kapila]

On Mon, Oct 12, 2015 at 8:10 PM, Tom Lane  wrote:
>
> I wrote:
> > Andres Freund  writes:
> >> Right. But that doesn't mean it's right to call PGSharedMemoryDetach()
> >> without other changes as done in Michael's proposed patch? That'll do
an
> >> UnmapViewOfFile() which'll fail because nothing i mapped, but still not
> >> close UsedShmemSegID?
>
> > Ah, right, I'd not noticed that he proposed changing
> > CloseHandle(UsedShmemSegID) to PGSharedMemoryDetach().  The latter is
> > clearly the wrong thing.
>
> Actually, now that I look at it, it's even more obvious that this is the
> wrong thing because *all the subprocess types in question already call
> PGSharedMemoryDetach*.  That's necessary on Unix, but I should think that
> on Windows all it will do is provoke the log message:
>
> elog(LOG, "could not unmap view of shared memory: error code
%lu", GetLastError());
>
> Could someone confirm whether syslogger, archiver, stats collector
> processes reliably produce that log message at startup on Windows?
>

I have tried this approach of calling PGSharedMemoryDetach() for
syslogger before calling closehandle() patch and I saw that message
and understood that it is not going to work.

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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Tom Lane]

I wrote:
> Actually, now that I look at it, it's even more obvious that this is the
> wrong thing because *all the subprocess types in question already call
> PGSharedMemoryDetach*.

Ah, scratch that: in most of them, the call is in #ifndef EXEC_BACKEND
stanzas.  The exception is bgworker start for a non-attached-to-shmem
worker, and in that case there's no log message because in fact
SubPostmasterMain did reattach.

This is kind of a mess :-(.  But it does look like what we want is
for SubPostmasterMain to do more than nothing when it chooses not to
reattach.  Probably that should include resetting UsedShmemSegAddr to
NULL, as well as closing the handle.

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] Postgres service stops when I kill client backend on Windows

[Tom Lane]

Oleg Bartunov  writes:
> Assuming the problem will be fixed, should we release Beta2 soon ?

This bug has existed since we had native Windows support.  It's entirely
immaterial for beta purposes, and I have a hard time thinking it's
critical enough to justify a short release cycle for the back branches
either.

regards, tom lane


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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Oleg Bartunov]

On Mon, Oct 12, 2015 at 4:42 PM, Dmitry Vasilyev 
wrote:

> Hello, Amit!
>
> On Пн, 2015-10-12 at 11:25 +0530, Amit Kapila wrote:
>
> On Sun, Oct 11, 2015 at 9:12 PM, Tom Lane  wrote:
> >
> > Magnus Hagander  writes:
> > > On Sun, Oct 11, 2015 at 5:22 PM, Tom Lane  wrote:
> > >> I'm a bit suspicious that we may have leaked a handle to the shared
> > >> memory block someplace, for example.  That would explain why this
> > >> symptom is visible now when it was not in 2009.  Or maybe it's
> dependent
> > >> on some feature that we didn't test back then --- for instance, if
> > >> the logging collector is in use, could it have inherited a handle and
> > >> not closed it?
> >
> > > Even if we leaked it, it should go away when the other processes died.
> >
> > I'm fairly certain that we do not kill/restart the logging collector
> > during a database restart (because it's impossible to reproduce the
> > original stderr destination if we do).
>
> True and it seems this is the reason for issue we are discussing here.
> The reason why this happens is that during creation of shared memory
> (PGSharedMemoryCreate()), we duplicate the handle such that it
> become inheritable by all child processes.  Then during fork
> (syslogger_forkexec()->postmaster_forkexec()->internal_forkexec) we
> always inherit the handles which causes syslogger to get a copy of
> shared memory handle which it neither uses and nor closes it.
>
> I could easily reproduce the issue if logging collector is on and even if
> we try to increase the loop count or sleep time in PGSharedMemoryCreate(),
> it doesn't change the situation as the syslogger has a valid handle to
> shared memory.  One way to fix is to just close the shared memory handle
> in sys logger as we are not going to need it and attached patch which does
> this fixes the issue for me.  Another invasive fix in case we want to
> retain shared memory handle for some purpose (future requirement) could
> be to send some signal to syslogger in restart path so that it can release
> the shared memory handle.
>
>
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
>
> Specified patch with "ifdef WIN32" is working for me. Maybe it’s necessary
> to check open handlers from replication for example?
>
>
Assuming the problem will be fixed, should we release Beta2 soon ?



>
> --
> Dmitry Vasilyev
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Tom Lane]

Andres Freund  writes:
> On 2015-10-12 21:38:12 +0900, Michael Paquier wrote:
>> Actually, doesn't this apply as well to the archiver and the pgstat
>> collector?

> As mentioned above? The difference is that the archiver et al get killed
> by postmaster during a PANIC restart thus don't present the problem
> discussed here.

I thought your objection to the original patch was exactly that we should
not treat syslogger as a special case for this purpose.

> Well, in those cases we won't have attached to shared memory, so I'm not
> convinced that this is the right solution.

No, you're missing the point.  In Windows builds, child processes inherit
a "handle" reference to the shared memory mapping, whether or not they
make any use of the handle to re-attach to that shared memory.  The point
here is that we need to close that handle if we're not going to use it.

I think the right thing is something close to Michael's proposed patch,
though not duplicating and reversing the previous if-test like that.
In other words, something like this in SubPostmasterMain:

/*
 * If appropriate, physically re-attach to shared memory segment. We 
want
 * to do this before going any further to ensure that we can attach at 
the
 * same address the postmaster used.
+* If we're not re-attaching, close the inherited handle to avoid leaks.
 */
if (strcmp(argv[1], "--forkbackend") == 0 ||
strcmp(argv[1], "--forkavlauncher") == 0 ||
strcmp(argv[1], "--forkavworker") == 0 ||
strcmp(argv[1], "--forkboot") == 0 ||
strncmp(argv[1], "--forkbgworker=", 15) == 0)
PGSharedMemoryReAttach();
+#ifdef WIN32
+   else
+   close the handle;
+#endif


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] Postgres service stops when I kill client backend on Windows

[Tom Lane]

I wrote:
> Andres Freund  writes:
>> Right. But that doesn't mean it's right to call PGSharedMemoryDetach()
>> without other changes as done in Michael's proposed patch? That'll do an
>> UnmapViewOfFile() which'll fail because nothing i mapped, but still not
>> close UsedShmemSegID?

> Ah, right, I'd not noticed that he proposed changing
> CloseHandle(UsedShmemSegID) to PGSharedMemoryDetach().  The latter is
> clearly the wrong thing.

Actually, now that I look at it, it's even more obvious that this is the
wrong thing because *all the subprocess types in question already call
PGSharedMemoryDetach*.  That's necessary on Unix, but I should think that
on Windows all it will do is provoke the log message:

elog(LOG, "could not unmap view of shared memory: error code %lu", 
GetLastError());

Could someone confirm whether syslogger, archiver, stats collector
processes reliably produce that log message at startup on Windows?

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] point_ops for GiST

[Stefan Keller]

Hi Alexander

Thanks for your succinct reply.
Actually I considered contributing myself for the first time to
PostgreSQL and/or PostGIS.
So, concluding from your explanations there's no big use case behind
build-in geometric types except serving as reference implementation?
I'm still torn over this splitting resources to implement types like
geometry twice.

:Stefan

2015-10-12 11:24 GMT+02:00 Alexander Korotkov :
> Hi, Stefan!
>
> On Sun, Oct 11, 2015 at 10:00 PM, Stefan Keller  wrote:
>>
>> Pls. don't misunderstand my questions: They are directed to get an
>> even more useful spatial data handling of PostgreSQL. I'm working with
>> PostGIS since years and are interested in any work regarding spatial
>> types...
>>
>> Can anyone report use cases or applications of these built-in geometric
>> types?
>>
>> Would'nt it be even more useful to concentrate to PostGIS
>> geometry/geography types and extend BRIN to these types?
>
>
> Note, that PostGIS is a different project which is maintained by separate
> team. PostGIS have its own priorities, development plan etc.
> PostgreSQL have to be self-consistent. In particular, it should have
> reference implementation of operator classes which extensions can use as the
> pattern. This is why it's important to maintain built-in geometric types.
>
> In short: once we implement it for built-in geometric types, you can ask
> PostGIS team to do the same for their geometry/geography.
>
> --
> Alexander Korotkov
> 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] Postgres service stops when I kill client backend on Windows

[Tom Lane]

I wrote:
> This is kind of a mess :-(.  But it does look like what we want is
> for SubPostmasterMain to do more than nothing when it chooses not to
> reattach.  Probably that should include resetting UsedShmemSegAddr to
> NULL, as well as closing the handle.

After poking around a bit more, I propose the attached patch.  I've
checked that this is happy with an EXEC_BACKEND Unix build, but I'm not
able to test it on Windows ... would somebody do that?

BTW, it appears from this that Cygwin builds have been broken right along
in a different way: according to the code in sysv_shmem's
PGSharedMemoryReAttach, Cygwin does cause a re-attach to occur, which we
were not undoing for putatively-not-connected-to-shmem child processes.
That's a robustness problem because it breaks the postmaster's expectation
that it's safe to not reinitialize shmem after a crash of one of those
processes.  I believe this patch fixes that problem as well, though if
anyone can test it on Cygwin that wouldn't be a bad thing either.

regards, tom lane

diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 8be5bbe..c7a3a91 100644
*** a/src/backend/port/sysv_shmem.c
--- b/src/backend/port/sysv_shmem.c
*** PGSharedMemoryReAttach(void)
*** 619,624 
--- 619,652 
  
  	UsedShmemSegAddr = hdr;		/* probably redundant */
  }
+ 
+ /*
+  * PGSharedMemoryNoReAttach
+  *
+  * Clean up if we choose *not* to re-attach to an already existing shared
+  * memory segment.  This is not used in the non EXEC_BACKEND case, either.
+  *
+  * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
+  * routine.  The caller must have already restored them to the postmaster's
+  * values.
+  */
+ void
+ PGSharedMemoryNoReAttach(void)
+ {
+ 	Assert(UsedShmemSegAddr != NULL);
+ 	Assert(IsUnderPostmaster);
+ 
+ #ifdef __CYGWIN__
+ 	/* cygipc (currently) appears to not detach on exec. */
+ 	PGSharedMemoryDetach();
+ #endif
+ 
+ 	/* For cleanliness, reset UsedShmemSegAddr to show we're not attached. */
+ 	UsedShmemSegAddr = NULL;
+ 	/* And the same for UsedShmemSegID. */
+ 	UsedShmemSegID = 0;
+ }
+ 
  #endif   /* EXEC_BACKEND */
  
  /*
*** PGSharedMemoryReAttach(void)
*** 629,634 
--- 657,665 
   * (it will have an on_shmem_exit callback registered to do that).  Rather,
   * this is for subprocesses that have inherited an attachment and want to
   * get rid of it.
+  *
+  * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
+  * routine.
   */
  void
  PGSharedMemoryDetach(void)
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index db67627..8152522 100644
*** a/src/backend/port/win32_shmem.c
--- b/src/backend/port/win32_shmem.c
***
*** 17,23 
  #include "storage/ipc.h"
  #include "storage/pg_shmem.h"
  
! HANDLE		UsedShmemSegID = 0;
  void	   *UsedShmemSegAddr = NULL;
  static Size UsedShmemSegSize = 0;
  
--- 17,23 
  #include "storage/ipc.h"
  #include "storage/pg_shmem.h"
  
! HANDLE		UsedShmemSegID = INVALID_HANDLE_VALUE;
  void	   *UsedShmemSegAddr = NULL;
  static Size UsedShmemSegSize = 0;
  
*** PGSharedMemoryCreate(Size size, bool mak
*** 218,226 
  		elog(LOG, "could not close handle to shared memory: error code %lu", GetLastError());
  
  
- 	/* Register on-exit routine to delete the new segment */
- 	on_shmem_exit(pgwin32_SharedMemoryDelete, PointerGetDatum(hmap2));
- 
  	/*
  	 * Get a pointer to the new shared memory segment. Map the whole segment
  	 * at once, and let the system decide on the initial address.
--- 218,223 
*** PGSharedMemoryCreate(Size size, bool mak
*** 254,259 
--- 251,259 
  	UsedShmemSegSize = size;
  	UsedShmemSegID = hmap2;
  
+ 	/* Register on-exit routine to delete the new segment */
+ 	on_shmem_exit(pgwin32_SharedMemoryDelete, PointerGetDatum(hmap2));
+ 
  	*shim = hdr;
  	return hdr;
  }
*** PGSharedMemoryReAttach(void)
*** 299,321 
  }
  
  /*
   * PGSharedMemoryDetach
   *
   * Detach from the shared memory segment, if still attached.  This is not
!  * intended for use by the process that originally created the segment. Rather,
   * this is for subprocesses that have inherited an attachment and want to
   * get rid of it.
   */
  void
  PGSharedMemoryDetach(void)
  {
  	if (UsedShmemSegAddr != NULL)
  	{
  		if (!UnmapViewOfFile(UsedShmemSegAddr))
! 			elog(LOG, "could not unmap view of shared memory: error code %lu", GetLastError());
  
  		UsedShmemSegAddr = NULL;
  	}
  }
  
  
--- 299,368 
  }
  
  /*
+  * PGSharedMemoryNoReAttach
+  *
+  * Clean up if we choose *not* to re-attach to an already existing shared
+  * memory segment.
+  *
+  * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this
+  * routine.  The caller must have already restored them to the postmaster's
+  * values.
+  */
+ void
+ PGSharedMemoryNoReAttach(void)
+ {
+ 	Assert(UsedShmemSegAddr != NULL);
+ 	

Re: [HACKERS] More work on SortSupport for text - strcoll() and strxfrm() caching

[Peter Geoghegan]

On Mon, Oct 12, 2015 at 12:47 AM, Peter Geoghegan  wrote:
> I also noticed that I failed to reset the last_returned strcoll()
> cache variable as part of an abbreviation call, despite the fact that
> tapesort may freely interleave conversions with comparisons, while
> reusing buf1 and buf2 both as scratch space for strxfrm() blobs, as
> well as for storing strings to be compared with strcoll(). I suggest
> that the attached patch also be applied to fix this issue.

I think that I jumped the gun with this fix, because theoretically you
can still get the same problem in the opposite direction -- an
original string treated as a strxfrm() blob when the cache is
consulted.

I'll consider a more comprehensive fix.

-- 
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] Re: [BUGS] BUG #13611: test_postmaster_connection failed (Windows, listen_addresses = '0.0.0.0' or '::')

[Noah Misch]

On Mon, Oct 12, 2015 at 08:07:37PM -0400, Tom Lane wrote:
> Robert Haas  writes:
> > On Fri, Oct 9, 2015 at 10:16 PM, Noah Misch  wrote:
> >> The listening side is in good shape today.  This thread is about the 
> >> address
> >> that pg_ctl uses in PQping("host=...").  Listening on 0.0.0.0 is portable.
> >> PQping("host='0.0.0.0'") relies on non-portable semantics in the underlying
> >> connect() syscall.  PQping("host='127.0.0.1'") is a portable substitute.
> 
> > Ah.  So in this case 0.0.0.0 is interpreted to mean "any IP that's a
> > way to reach the local host", and using 127.0.0.1 makes sense because
> > we know that will always be one of them?  I could buy that line of
> > reasoning.
> 
> I do *not* buy that we can safely replace "localhost" by "127.0.0.1".

Nobody proposed that.  The word "localhost" did not appear in this thread.

> Consider a system that's only set up with IPv6 local addressing.
> 
> AFAICS the complaint in this bug is about a system with broken DNS (ie,
> unable to resolve "localhost" properly, which is something mandated by
> relevant RFCs, I believe).

The original post used only "0.0.0.0" and "::", not "localhost" or anything
else entailing name resolution.  As I wrote above, Kondo proposed for pg_ctl
to use PQping("host='127.0.0.1'") in place of PQping("host='0.0.0.0'").
That's all.  pg_ctl would continue to use PQping("host='localhost'") where
it's doing so today.  A patch that changes the 0.0.0.0 case in this way should
also change PQping("host='::'") to PQping("host='::1'"); I suspect that was
implicit in the original proposal.

nm


-- 
Sent 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 work on SortSupport for text - strcoll() and strxfrm() caching

[Peter Geoghegan]

On Mon, Oct 12, 2015 at 4:15 PM, Robert Haas  wrote:
> In this case, I think
> the best thing for me to do right now is wait to commit anything
> further until you have had a chance to go over this and come up with a
> fix or set of fixes that you think are completely, 100% ready to go;
> or else until you get to the point of wanting feedback before
> proceeding further.  In general, I would appreciate if this sort of
> post-commit self-review could be done pre-commit.

This came to me while I was making dinner last night - I was not
actually poring over the code on my computer. I don't know why I
thought of it then and not at any earlier point, but it seems
reasonable to suppose it had something to do with perspective, or
being able to see a bigger picture that is difficult to keep in mind
during initial development and self review. I was mostly thinking
about external sorting at the time, and not this patch.

I cannot do that on demand. It isn't a matter of effort. When I come
back from vacation at the end of the month, I may be able to do
somewhat better for a few months.

-- 
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] Some questions about the array.

[Robert Haas]

On Fri, Oct 9, 2015 at 8:02 AM, YUriy Zhuravlev
 wrote:
> We were some of the issues associated with the behavior of arrays.
> 1. We would like to implement arrays negative indices (from the end) like in
> Python or Ruby: arr[-2]  or arr[1: -1]
> but as an array can be indexed in the negative area so it probably can not be
> done.

That seems like a complete non-starter because it would break backward
compatibility.  Our array implementation allows negative indexes:

rhaas=# select ('[-1:4]={3,1,4,1,5,9}'::int[])[-1];
 int4
--
3
(1 row)

-- 
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] Re: [BUGS] BUG #13611: test_postmaster_connection failed (Windows, listen_addresses = '0.0.0.0' or '::')

[Noah Misch]

On Mon, Oct 12, 2015 at 07:37:42PM -0400, Robert Haas wrote:
> On Fri, Oct 9, 2015 at 10:16 PM, Noah Misch  wrote:
> > On Fri, Oct 09, 2015 at 03:14:26PM -0400, Robert Haas wrote:
> >> On Thu, Oct 8, 2015 at 11:26 PM, Noah Misch  wrote:
> >> >> In particular, magically
> >> >> substituting 127.0.0.1 for 0.0.0.0 seems utterly without principle.
> >> >
> >> > Binding a listening socket to "0.0.0.0" listens on every local IPv4 
> >> > address,
> >> > and 127.0.0.1 is one of those addresses.  That's the principle.  It's
> >> > inelegant, but I expect it to work everywhere.
> >>
> >> But... what about the machine's other addresses?
> >>
> >> If Windows doesn't treat 0.0.0.0 to mean listen on every interface,
> >> that's a shame.  But making it only listen on 127.0.0.1 and not any of
> >> the others does not seem better.  Then, instead of 0.0.0.0 failing on
> >> Windows, it would instead work but with different behavior.  That
> >> doesn't seem good either.
> >
> > The listening side is in good shape today.  This thread is about the address
> > that pg_ctl uses in PQping("host=...").  Listening on 0.0.0.0 is portable.
> > PQping("host='0.0.0.0'") relies on non-portable semantics in the underlying
> > connect() syscall.  PQping("host='127.0.0.1'") is a portable substitute.
> 
> Ah.  So in this case 0.0.0.0 is interpreted to mean "any IP that's a
> way to reach the local host", and using 127.0.0.1 makes sense because
> we know that will always be one of them?  I could buy that line of
> reasoning.

Exactly.


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

Re: [HACKERS] Postgres service stops when I kill client backend on Windows

[Dmitry Vasilyev]

Hello Tom!
On Пн, 2015-10-12 at 16:35 -0400, Tom Lane wrote:
> I wrote:
> > This is kind of a mess :-(.  But it does look like what we want is
> > for SubPostmasterMain to do more than nothing when it chooses not
> > to
> > reattach.  Probably that should include resetting UsedShmemSegAddr
> > to
> > NULL, as well as closing the handle.
> 
> After poking around a bit more, I propose the attached patch.  I've
> checked that this is happy with an EXEC_BACKEND Unix build, but I'm
> not
> able to test it on Windows ... would somebody do that?
> 
> BTW, it appears from this that Cygwin builds have been broken right
> along
> in a different way: according to the code in sysv_shmem's
> PGSharedMemoryReAttach, Cygwin does cause a re-attach to occur, which
> we
> were not undoing for putatively-not-connected-to-shmem child
> processes.
> That's a robustness problem because it breaks the postmaster's
> expectation
> that it's safe to not reinitialize shmem after a crash of one of
> those
> processes.  I believe this patch fixes that problem as well, though
> if
> anyone can test it on Cygwin that wouldn't be a bad thing either.
> 
>   regards, tom lane
> 

This patch is working for me,
binaries: https://goo.gl/32j7QE (MSVC 2010, build script here: 
https://github.com/postgrespro/pgwininstall).


--
Dmitry Vasilyev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



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

Re: [HACKERS] Proposal: pg_confcheck - syntactic & semantic validation of postgresql configuration files

[Amir Rohan]

On 10/13/2015 02:02 AM, Robert Haas wrote:
> On Fri, Oct 9, 2015 at 4:38 PM, Amir Rohan  wrote:
>> It does catch bad syntax, but in most cases all you get is
>> "The setting could not be applied".  that's not great for enums
>> or a float instead of an int. I guess a future version will fix that
>> (or not).
> 
> I expect we would consider patches to improve the error messages if
> you (or someone else) wanted to propose such.  But you don't have to
> want to do that.
> 

>> You need a running server to run a check. You need to monkey
>> with said server's configuration in place to run a check. You must be on
>> 9.5+. The checking mechanism isn't extensible. Certainly not as easily
>> as dropping a new rule file somewhere. It doesn't check (AFAICT) for bad
>> combinations of values, for example it will tell you that you can't
>> change `wal_archive` without restart (without showing source location
>> btw, bug?), but not that you better set `wal_level` *before* you
>> restart.  It doesn't do any semantic checks. It won't warn you
>> about things that are not actually an error, just a bad idea.
> 
> So, I'm not saying that a config checker has no value.  In fact, I
> already said the opposite.  You seem to be jumping all over me here
> when all I was trying to do is explain what I think Tom was getting
> at.   I *do* think that pg_file_settings is a helpful feature that is
> certainly related to what you are trying to do, but I don't think that
> it means that a config checker is useless.  Fair?
> 

That wasn't my intention. Perhaps I'm overreacting to a long-standing
"Tom Lane's bucket of cold water" tradition. I'm new here.
I understand your point and I was only reiterating what in particular
makes the conf checker distinctly useful IMO, and what it could
provide that pg_settings doesn't.

I've looked at parts of the pg_settings implementation and indeed some
of that code (and legwork) could be reused so the mundane parts
of writing this will be less hassle. I might have missed that if Tom and
you hadn't pointed that out.

So, Fair, and thanks.

Amir



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

Re: [HACKERS] Parallel Aggregate

[Robert Haas]

On Sun, Oct 11, 2015 at 10:07 PM, Haribabu Kommi
 wrote:
> Parallel aggregate is the feature doing the aggregation job parallel
> with the help of Gather and
> partial seq scan nodes. The following is the basic overview of the
> parallel aggregate changes.
>
> Decision phase:
>
> Based on the following conditions, the parallel aggregate plan is generated.
>
> - check whether the below plan node is Gather + partial seq scan only.
>
> This is because to check whether the plan nodes that are present are
> aware of parallelism or not?

This is really not the right way of doing this.  We should do
something more general.  Most likely, parallel aggregate should wait
for Tom's work refactoring the upper planner to use paths.  But either
way, it's not a good idea to limit ourselves to parallel aggregation
only in the case where there is exactly one base table.

One of the things I want to do pretty early on, perhaps in time for
9.6, is create a general notion of partial paths.  A Partial Seq Scan
node creates a partial path.  A Gather node turns a partial path into
a complete path.  A join between a partial path and a complete path
creates a new partial path.  This concept lets us consider,
essentially, pushing joins below Gather nodes.  That's quite powerful
and could make Partial Seq Scan applicable to a much broader variety
of use cases.  If there are worthwhile partial paths for the final
joinrel, and aggregation of that joinrel is needed, we can consider
parallel aggregation using that partial path as an alternative to
sticking a Gather node on there and then aggregating.

> - Set the single_copy mode as true, in case if the below node of
> Gather is a parallel aggregate.

That sounds wrong.  Single-copy mode is for when we need to be certain
of running exactly one copy of the plan.  If you're trying to have
several workers aggregate in parallel, that's exactly what you don't
want.

Also, I think the path for parallel aggregation should probably be
something like FinalizeAgg -> Gather -> PartialAgg -> some partial
path here.  I'm not clear whether that is what you are thinking or
not.

-- 
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] [COMMITTERS] pgsql: Cause TestLib.pm to define $windows_os in all branches.

[Michael Paquier]

On Tue, Oct 13, 2015 at 8:35 AM, Tom Lane  wrote:
> Cause TestLib.pm to define $windows_os in all branches.
>
> Back-port of a part of commit 690ed2b76ab91eb79ea04ee2bfbdc8a2693f2a37 that
> I'd depended on without realizing that it was only added recently.  Since
> it seems entirely likely that other such tests will need to be back-patched
> in future, providing the flag seems like a better answer than just putting
> a test in-line.

Is it really worth it back-patching the portions with $windows_os to
back-branches? This indeed makes back-patch a bit easier but the tests
cannot run on Windows because 13d856e1 has not been added in PG <=
9.5.
-- 
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] More work on SortSupport for text - strcoll() and strxfrm() caching

[Robert Haas]

On Mon, Oct 12, 2015 at 3:31 PM, Peter Geoghegan  wrote:
> On Mon, Oct 12, 2015 at 12:47 AM, Peter Geoghegan  wrote:
>> I also noticed that I failed to reset the last_returned strcoll()
>> cache variable as part of an abbreviation call, despite the fact that
>> tapesort may freely interleave conversions with comparisons, while
>> reusing buf1 and buf2 both as scratch space for strxfrm() blobs, as
>> well as for storing strings to be compared with strcoll(). I suggest
>> that the attached patch also be applied to fix this issue.
>
> I think that I jumped the gun with this fix, because theoretically you
> can still get the same problem in the opposite direction -- an
> original string treated as a strxfrm() blob when the cache is
> consulted.
>
> I'll consider a more comprehensive fix.

This is not the first time I've committed one of your patches and
promptly received a whole series of emails with fixes for what went
into that commit, despite the fact that I have not and did not change
the relevant parts of the patch while committing.  Since that makes
more work for me, I am not a huge fan, and the practical effect is to
subtract from the amount of time that could otherwise have been spent
reviewing your next patch (or someone else's).  In this case, I think
the best thing for me to do right now is wait to commit anything
further until you have had a chance to go over this and come up with a
fix or set of fixes that you think are completely, 100% ready to go;
or else until you get to the point of wanting feedback before
proceeding further.  In general, I would appreciate if this sort of
post-commit self-review could be done pre-commit.

I apologize for the fact that this email probably sounds grouchy.
Please try to read it in the most positive light possible (and don't
shoot me).

-- 
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] Re: [BUGS] BUG #13611: test_postmaster_connection failed (Windows, listen_addresses = '0.0.0.0' or '::')

[Robert Haas]

On Fri, Oct 9, 2015 at 10:16 PM, Noah Misch  wrote:
> On Fri, Oct 09, 2015 at 03:14:26PM -0400, Robert Haas wrote:
>> On Thu, Oct 8, 2015 at 11:26 PM, Noah Misch  wrote:
>> >> In particular, magically
>> >> substituting 127.0.0.1 for 0.0.0.0 seems utterly without principle.
>> >
>> > Binding a listening socket to "0.0.0.0" listens on every local IPv4 
>> > address,
>> > and 127.0.0.1 is one of those addresses.  That's the principle.  It's
>> > inelegant, but I expect it to work everywhere.
>>
>> But... what about the machine's other addresses?
>>
>> If Windows doesn't treat 0.0.0.0 to mean listen on every interface,
>> that's a shame.  But making it only listen on 127.0.0.1 and not any of
>> the others does not seem better.  Then, instead of 0.0.0.0 failing on
>> Windows, it would instead work but with different behavior.  That
>> doesn't seem good either.
>
> The listening side is in good shape today.  This thread is about the address
> that pg_ctl uses in PQping("host=...").  Listening on 0.0.0.0 is portable.
> PQping("host='0.0.0.0'") relies on non-portable semantics in the underlying
> connect() syscall.  PQping("host='127.0.0.1'") is a portable substitute.

Ah.  So in this case 0.0.0.0 is interpreted to mean "any IP that's a
way to reach the local host", and using 127.0.0.1 makes sense because
we know that will always be one of them?  I could buy that line of
reasoning.

-- 
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: pg_confcheck - syntactic & semantic validation of postgresql configuration files

[Robert Haas]

On Fri, Oct 9, 2015 at 4:38 PM, Amir Rohan  wrote:
> It does catch bad syntax, but in most cases all you get is
> "The setting could not be applied".  that's not great for enums
> or a float instead of an int. I guess a future version will fix that
> (or not).

I expect we would consider patches to improve the error messages if
you (or someone else) wanted to propose such.  But you don't have to
want to do that.

> You need a running server to run a check. You need to monkey
> with said server's configuration in place to run a check. You must be on
> 9.5+. The checking mechanism isn't extensible. Certainly not as easily
> as dropping a new rule file somewhere. It doesn't check (AFAICT) for bad
> combinations of values, for example it will tell you that you can't
> change `wal_archive` without restart (without showing source location
> btw, bug?), but not that you better set `wal_level` *before* you
> restart.  It doesn't do any semantic checks. It won't warn you
> about things that are not actually an error, just a bad idea.

So, I'm not saying that a config checker has no value.  In fact, I
already said the opposite.  You seem to be jumping all over me here
when all I was trying to do is explain what I think Tom was getting
at.   I *do* think that pg_file_settings is a helpful feature that is
certainly related to what you are trying to do, but I don't think that
it means that a config checker is useless.  Fair?

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


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

Re: [HACKERS] bugs and bug tracking

[Robert Haas]

On Tue, Oct 6, 2015 at 2:33 PM, Nathan Wagner  wrote:
> Two, I think any attempt to tell the developers and committers that they
> need to change their workflow to adapt to some system is bound to fail,
> so, I have asked, just what changed would you all be willing to actually
> *do*?  Tom Lane is pretty good at noting a bug number in his commit
> messages, for example.  Would he be willing to modify that slightly to
> make it easier to machine parse?  Would you be willing to add a bug
> number to your commit messages?  I'm not asking for guarantees.
> Actually I'm not really asking for anything, I'm just trying to figure
> out what the parameters of a solution might be.  If the answer to that
> is "no, I'm not willing to change anything at all", that's fine, it just
> colors what might be done and how much automation I or someone else
> might be able to write.

I'd personally be willing to put machine-parseable metadata into my
commit messages provided that:

1. I'm not the only one doing it - i.e. at least 3 or 4
moderately-frequent committers are all doing it consistently and all
using the same format.  If Tom buys into it, that's a big plus.

2. Adding the necessary metadata to a commit can be reasonably
expected to take no more than 2 minutes in typical cases (preferably
less).

3. Adding the metadata doesn't cause lines > 70 characters.  I am not
a fan of the "Discussion: Message-ID-Here" format which some
committers have begun using, sometimes with just the message ID and
sometimes with the full URL, because anything which causes horizontal
scrolling makes me sad.

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


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

Re: [HACKERS] pg_ctl/pg_rewind tests vs. slow AIX buildfarm members

[Tom Lane]

Michael Paquier  writes:
>> On Wed, Oct 7, 2015 at 11:52 PM, Tom Lane  wrote:
>>> I think there is still room to salvage something without fully rewriting
>>> the postmaster invocation logic to avoid using CMD, because it's still
>>> true that checking whether the CMD process is still there should be as
>>> good as checking the postmaster proper.  We just can't use kill() for it.

> I had a look at that, and attached is an updated patch showing the
> concept of using an HANDLE that pg_ctl waits for. I agree that saving
> an HANDLE the way this patch does using a static variable is a bit
> ugly especially knowing that service registration uses
> test_postmaster_connection as well with directly a postmaster. The
> good thing is that tapcheck runs smoothly, with one single failure
> though: the second call to pg_ctl start -w may succeed instead of
> failing if kicked within an interval of 3 seconds after the first one,
> based on the tests on my Windows VM. My guess is that this is caused
> by the fact that we monitor the shell process and not the postmaster
> itself. Thoughts?

Yeah, it can still succeed because pg_ctl can't tell that the
postmaster.pid created by the earlier invocation isn't the one it wants.
It adopts the values out of that file, tests the connection, finds it
works, and declares victory, not realizing that the postmaster *it*
started will soon fail (or maybe already has).

Waiting more than 2 seconds is enough to make sure that
test_postmaster_connection sees the pre-existing pidfile as stale and
doesn't believe that it represents a successful postmaster start.

So there's still something to be desired on Windows; but it's still
better than before in that we can reliably detect child process exit
instead of having to use the five-second heuristic.  And of course on
Unix this is way better than before.

So I've pushed this with some cosmetic adjustments, as well as the not
so cosmetic adjustment of making the service-start path also use handle
testing.  If there are remaining problems, the buildfarm should tell us.

I'm not sure if this will completely fix our problems with "pg_ctl start"
related buildfarm failures on very slow critters.  It does get rid of the
hard wired 5-second timeout, but the 60-second timeout could still be an
issue.  I think Noah was considering a patch to allow that number to be
raised.  I'd be in favor of letting pg_ctl accept a default timeout length
from an environment variable, and then the slower critters could be fixed
by adjusting their buildfarm configurations.

regards, tom lane


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

Re: [HACKERS] Re: [BUGS] BUG #13611: test_postmaster_connection failed (Windows, listen_addresses = '0.0.0.0' or '::')

[Tom Lane]

Robert Haas  writes:
> On Fri, Oct 9, 2015 at 10:16 PM, Noah Misch  wrote:
>> The listening side is in good shape today.  This thread is about the address
>> that pg_ctl uses in PQping("host=...").  Listening on 0.0.0.0 is portable.
>> PQping("host='0.0.0.0'") relies on non-portable semantics in the underlying
>> connect() syscall.  PQping("host='127.0.0.1'") is a portable substitute.

> Ah.  So in this case 0.0.0.0 is interpreted to mean "any IP that's a
> way to reach the local host", and using 127.0.0.1 makes sense because
> we know that will always be one of them?  I could buy that line of
> reasoning.

I do *not* buy that we can safely replace "localhost" by "127.0.0.1".
Consider a system that's only set up with IPv6 local addressing.

AFAICS the complaint in this bug is about a system with broken DNS (ie,
unable to resolve "localhost" properly, which is something mandated by
relevant RFCs, I believe).  We should not break legally, if perhaps
strangely, configured systems in order to work around a misconfigured one.
Failure to resolve localhost to a working loopback address will break a
lot of services besides ours, so the OP is going to need to fix the
configuration problem eventually anyway.

BTW, it looks like pgstat.c will work in such a case, but it's only
accidentally so; as Noah notes, listening to 0.0.0.0 works, and then
it looks like we get the actual IP address for backends to connect to
from the listen socket itself.  That wasn't a situation that the code
was meant to handle, for sure, and I wouldn't want to bet that it would
work like that on every platform.

Now, if you're proposing trying "localhost" and falling back to 127.0.0.1
if it fails to resolve, that would be OK with me --- but it would be a
significant amount of additional complexity for what remains a case I do
not think we need to support.

regards, tom lane


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

Re: [HACKERS] bugs and bug tracking

[Michael Paquier]

On Tue, Oct 13, 2015 at 8:36 AM, Robert Haas wrote:
> 1. I'm not the only one doing it - i.e. at least 3 or 4
> moderately-frequent committers are all doing it consistently and all
> using the same format.  If Tom buys into it, that's a big plus.
>
> 2. Adding the necessary metadata to a commit can be reasonably
> expected to take no more than 2 minutes in typical cases (preferably
> less).
>
> 3. Adding the metadata doesn't cause lines > 70 characters.  I am not
> a fan of the "Discussion: Message-ID-Here" format which some
> committers have begun using, sometimes with just the message ID and
> sometimes with the full URL, because anything which causes horizontal
> scrolling makes me sad.

Adding the discussion related to the commit directly in the log
history is really a bit plus IMO even if this breaks the rule of
you-shall-not-write-more-than-72-characters-per-line (gmail ones are
close to this limit), this allows to gain a lot of time when looking
for a particular discussion without having to go through the archives
by looking for example for the author report or a bug number, those
fields being rather helpful in general:
Discussion: Message-ID
Reviewed by: Jane Doe
Author: John Doe
[Bug Number: #XXX]
Regards,
-- 
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] [COMMITTERS] pgsql: Cause TestLib.pm to define $windows_os in all branches.

[Tom Lane]

Michael Paquier  writes:
> On Tue, Oct 13, 2015 at 8:35 AM, Tom Lane  wrote:
>> Cause TestLib.pm to define $windows_os in all branches.
>> 
>> Back-port of a part of commit 690ed2b76ab91eb79ea04ee2bfbdc8a2693f2a37 that
>> I'd depended on without realizing that it was only added recently.  Since
>> it seems entirely likely that other such tests will need to be back-patched
>> in future, providing the flag seems like a better answer than just putting
>> a test in-line.

> Is it really worth it back-patching the portions with $windows_os to
> back-branches? This indeed makes back-patch a bit easier but the tests
> cannot run on Windows because 13d856e1 has not been added in PG <=
> 9.5.

Hmm, well, why wasn't that back-patched?  We expect these tests to run
on Windows don't we?

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] Parallel Aggregate

[Haribabu Kommi]

On Tue, Oct 13, 2015 at 12:14 PM, Robert Haas  wrote:
> On Sun, Oct 11, 2015 at 10:07 PM, Haribabu Kommi
>  wrote:
>> Parallel aggregate is the feature doing the aggregation job parallel
>> with the help of Gather and
>> partial seq scan nodes. The following is the basic overview of the
>> parallel aggregate changes.
>>
>> Decision phase:
>>
>> Based on the following conditions, the parallel aggregate plan is generated.
>>
>> - check whether the below plan node is Gather + partial seq scan only.
>>
>> This is because to check whether the plan nodes that are present are
>> aware of parallelism or not?
>
> This is really not the right way of doing this.  We should do
> something more general.  Most likely, parallel aggregate should wait
> for Tom's work refactoring the upper planner to use paths.  But either
> way, it's not a good idea to limit ourselves to parallel aggregation
> only in the case where there is exactly one base table.

Ok. Thanks for the details.

> One of the things I want to do pretty early on, perhaps in time for
> 9.6, is create a general notion of partial paths.  A Partial Seq Scan
> node creates a partial path.  A Gather node turns a partial path into
> a complete path.  A join between a partial path and a complete path
> creates a new partial path.  This concept lets us consider,
> essentially, pushing joins below Gather nodes.  That's quite powerful
> and could make Partial Seq Scan applicable to a much broader variety
> of use cases.  If there are worthwhile partial paths for the final
> joinrel, and aggregation of that joinrel is needed, we can consider
> parallel aggregation using that partial path as an alternative to
> sticking a Gather node on there and then aggregating.
>
>> - Set the single_copy mode as true, in case if the below node of
>> Gather is a parallel aggregate.
>
> That sounds wrong.  Single-copy mode is for when we need to be certain
> of running exactly one copy of the plan.  If you're trying to have
> several workers aggregate in parallel, that's exactly what you don't
> want.

I mean of setting the flag is to avoid backend executing the child plan.

> Also, I think the path for parallel aggregation should probably be
> something like FinalizeAgg -> Gather -> PartialAgg -> some partial
> path here.  I'm not clear whether that is what you are thinking or
> not.

No. I am thinking of the following way.
Gather->partialagg->some partial path

I want the Gather node to merge the results coming from all workers, otherwise
it may be difficult to merge at parent of gather node. Because in case
the partial
group aggregate is under the Gather node, if any of two workers are returning
same group key data, we need to compare them and combine it to make it a
single group. If we are at Gather node, it is possible that we can
wait till we get
slots from all workers. Once all workers returns the slots we can compare
and merge the necessary slots and return the result. Am I missing something?

Regards,
Hari Babu
Fujitsu Australia


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

[HACKERS] [PROPOSAL] DIAGNOSTICS = SKIPPED_ROW_COUNT

[dinesh kumar]

Hi Team,

Would like to propose a new DIAGNOSTICS attribute, which returns the no.of
rows got skipped during the FOR UPDATE SKIP LOCKED;

Using this attribute, we can have more control on parallel operations like,

IF SKIPPED_ROW_COUNT =0 THEN
<>
ELSE
<>
END IF;

Kindly let me know your inputs/suggestions on this.

Regards,
Dinesh
manojadinesh.blogspot.com

Re: [HACKERS] [PROPOSAL] DIAGNOSTICS = SKIPPED_ROW_COUNT

[dinesh kumar]

On Mon, Oct 12, 2015 at 9:38 PM, Tom Lane  wrote:

> dinesh kumar  writes:
> > Would like to propose a new DIAGNOSTICS attribute, which returns the
> no.of
> > rows got skipped during the FOR UPDATE SKIP LOCKED;
>
> I'm concerned that there may not be any implementation-independent
> definition of this.  That is, the query plan might or might not reject
> rows before the locking step is reached, which would result in
> random-looking changes in the output of the proposed counter.
>
> Constraining the query plan might fix that, but only at unacceptable
> performance costs, especially since those constraints would have to apply
> to every plan ever generated (since the query planner can't know whether
> you will inquire about this counter value later).
>
>
Thanks Tom. Understood.


> > Using this attribute, we can have more control on parallel operations
> like,
>
> > IF SKIPPED_ROW_COUNT =0 THEN
> > <>
> > ELSE
> > <>
> > END IF;
>
> Um ... so what?  This is not a use-case.
>
>
In my view, "How one can be sure that, he obtained all the tuples with SKIP
LOCKED". If the end user has this counter value, he may proceed with a
different approach with partially locked tuples.

regards, tom lane
>



-- 

Regards,
Dinesh
manojadinesh.blogspot.com

Re: [HACKERS] bugs and bug tracking

[Tom Lane]

Michael Paquier  writes:
> On Tue, Oct 13, 2015 at 8:36 AM, Robert Haas wrote:
>> 3. Adding the metadata doesn't cause lines > 70 characters.  I am not
>> a fan of the "Discussion: Message-ID-Here" format which some
>> committers have begun using, sometimes with just the message ID and
>> sometimes with the full URL, because anything which causes horizontal
>> scrolling makes me sad.

> Adding the discussion related to the commit directly in the log
> history is really a bit plus IMO even if this breaks the rule of
> you-shall-not-write-more-than-72-characters-per-line (gmail ones are
> close to this limit), this allows to gain a lot of time when looking
> for a particular discussion without having to go through the archives
> by looking for example for the author report or a bug number, those
> fields being rather helpful in general:

Sure, but if we're adding a bug tracker that links to email, one of the
functions of the tracker can be to provide a less-verbose linking URL.

(Although I'm not sure how well that squares with the opinion I expressed
earlier that the tracker needs to link to the commit not vice versa.
It will not always be the case that a tracker entry gets made before the
commit is; besides which commit log entries cannot be fixed after the
fact.  For the same reason, I'm not that excited about embedding email
URLs in commit log entries.)

I'm with Robert on the idea that commit log entries need to be
limited-width.  I personally format them to 75 characters, so that
git_changelog's output is less than 80 characters.

regards, tom lane


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

Re: [HACKERS] [COMMITTERS] pgsql: Cause TestLib.pm to define $windows_os in all branches.

[Michael Paquier]

On Tue, Oct 13, 2015 at 11:31 AM, Tom Lane wrote:
> Hmm, well, why wasn't that back-patched?  We expect these tests to run
> on Windows don't we?

The message related to this particular commit is here:
http://www.postgresql.org/message-id/55b90161.5090...@iki.fi
I recall that we discussed about back-patching more such things to
improve the buildfarm coverage but I guess it fell from other's radar.
Would you consider pushing any sync-up patch for 9.5 and 9.4 I could
send? At quick glance, I think that's basically a combination of
adb4950, 13d856e1, 690ed2b and ff85fc8. Andrew, Noah, Heikki, and
others feel free to object of course if you think that's an utterly
bad idea.
-- 
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] [PROPOSAL] DIAGNOSTICS = SKIPPED_ROW_COUNT

[Tom Lane]

dinesh kumar  writes:
> Would like to propose a new DIAGNOSTICS attribute, which returns the no.of
> rows got skipped during the FOR UPDATE SKIP LOCKED;

I'm concerned that there may not be any implementation-independent
definition of this.  That is, the query plan might or might not reject
rows before the locking step is reached, which would result in
random-looking changes in the output of the proposed counter.

Constraining the query plan might fix that, but only at unacceptable
performance costs, especially since those constraints would have to apply
to every plan ever generated (since the query planner can't know whether
you will inquire about this counter value later).

> Using this attribute, we can have more control on parallel operations like,

> IF SKIPPED_ROW_COUNT =0 THEN
> <>
> ELSE
> <>
> END IF;

Um ... so what?  This is not a use-case.

regards, tom lane


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