Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Victor Wagner]

On Wed, 9 Nov 2016 15:23:11 +0900
Michael Paquier  wrote:


> 
> (This is about patch 0007, not 0001)
> Thanks, you are right. That's not good as-is. So this basically means
> that the characters here should be from 32 to 127 included.

Really, most important is to exclude comma from the list of allowed
characters. And this prevents us from using a range.

I'd do something like:

char prinables="0123456789ABCDE...xyz!@#*&+";
unsigned int r;

for (i=0;i generate_nonce needs just to be made smarter in the way it selects the
> character bytes.



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

Re: [HACKERS] WAL logging problem in 9.4.3?

[Michael Paquier]

On Wed, Nov 9, 2016 at 9:27 AM, Michael Paquier 
wrote:
> On Wed, Nov 9, 2016 at 5:39 AM, Robert Haas  wrote:
>> On Thu, Feb 4, 2016 at 7:24 AM, Heikki Linnakangas 
wrote:
>>> I dropped the ball on this one back in July, so here's an attempt to
revive
>>> this thread.
>>>
>>> I spent some time fixing the remaining issues with the prototype patch I
>>> posted earlier, and rebased that on top of current git master. See
attached.
>>>
>>> Some review of that would be nice. If there are no major issues with
it, I'm
>>> going to create backpatchable versions of this for 9.4 and below.
>>
>> Are you going to do commit something here?  This thread and patch are
>> now 14 months old, which is a long time to make people wait for a bug
>> fix.  The status in the CF is "Ready for Committer" although I am not
>> sure if that's accurate.
>
> "Needs Review" is definitely a better definition of its current state.
> The last time I had a look at this patch I thought that it was in
> pretty good shape (not Horiguchi-san's version, but the one in
>
https://www.postgresql.org/message-id/CAB7nPqR+3JjS=JB3R=axxkxcyeb-q77u-erw7_ukajctwnt...@mail.gmail.com
).
> With some of the recent changes, surely it needs a second look, things
> related to heap handling tend to rot quickly.
>
> I'll look into it once again by the end of this week if Heikki does
> not show up, the rest will be on him I am afraid...

I have been able to hit a crash with recovery test 008:
(lldb) bt
* thread #1: tid = 0x, 0x7fff96d48f06
libsystem_kernel.dylib`__pthread_kill + 10, stop reason = signal SIGSTOP
  * frame #0: 0x7fff96d48f06 libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x7fff9102e4ec libsystem_pthread.dylib`pthread_kill + 90
frame #2: 0x7fff8e5cc6df libsystem_c.dylib`abort + 129
frame #3: 0x000106ef10f0
postgres`ExceptionalCondition(conditionName="!(( !( ((void) ((bool) (!
(!((buffer) <= NBuffers && (buffer) >= -NLocBuffer)) ||
(ExceptionalCondition(\"!((buffer) <= NBuffers && (buffer) >=
-NLocBuffer)\", (\"FailedAssertion\"), \"bufmgr.c\", 2593), 0, (buffer)
!= 0 ) ? ((bool) 0) : ((buffer) < 0) ? (LocalRefCount[-(buffer) - 1] > 0) :
(GetPrivateRefCount(buffer) > 0) ))", errorType="FailedAssertion",
fileName="bufmgr.c", lineNumber=2593) + 128 at assert.c:54
frame #4: 0x000106cf4a2c postgres`BufferGetBlockNumber(buffer=0) +
204 at bufmgr.c:2593
frame #5: 0x00010694e6ad
postgres`HeapNeedsWAL(rel=0x7f9454804118, buf=0) + 61 at heapam.c:9234
frame #6: 0x00010696d8bd
postgres`visibilitymap_set(rel=0x7f9454804118, heapBlk=1, heapBuf=0,
recptr=50841176, vmBuf=118, cutoff_xid=866, flags='\x01') + 989 at
visibilitymap.c:310
frame #7: 0x00010695d020
postgres`heap_xlog_visible(record=0x7f94520035d0) + 896 at heapam.c:8148
frame #8: 0x00010695c582
postgres`heap2_redo(record=0x7f94520035d0) + 242 at heapam.c:9107
frame #9: 0x0001069d132d postgres`StartupXLOG + 9181 at xlog.c:6950
frame #10: 0x000106c9d783 postgres`StartupProcessMain + 339 at
startup.c:216
frame #11: 0x0001069ee6ec postgres`AuxiliaryProcessMain(argc=2,
argv=0x7fff59316d80) + 1676 at bootstrap.c:420
frame #12: 0x000106c98002
postgres`StartChildProcess(type=StartupProcess) + 322 at postmaster.c:5221
frame #13: 0x000106c96031 postgres`PostmasterMain(argc=3,
argv=0x7f9451c04210) + 6033 at postmaster.c:1301
frame #14: 0x000106bc30cf postgres`main(argc=3,
argv=0x7f9451c04210) + 751 at main.c:228
(lldb) up 1
frame #4: 0x000106cf4a2c postgres`BufferGetBlockNumber(buffer=0) + 204
at bufmgr.c:2593
   2590{
   2591BufferDesc *bufHdr;
   2592
-> 2593Assert(BufferIsPinned(buffer));
   2594
   2595if (BufferIsLocal(buffer))
   2596bufHdr = GetLocalBufferDescriptor(-buffer - 1);
-- 
Michael

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Wed, Nov 9, 2016 at 3:13 PM, Victor Wagner  wrote:
> On Tue, 18 Oct 2016 16:35:27 +0900
> Michael Paquier  wrote:
>
>  Hi
>> Attached is a rebased patch set for SCRAM, with the following things:
>> - 0001, moving all the SHA2 functions to src/common/ and introducing a
>> PG-like interface. No actual changes here.
>
> It seems, that client nonce generation in this patch is not
> RFC-compliant.
>
> RFC 5802 states that SCRAM nonce should be
>
> a sequence of random printable ASCII
>   characters excluding ','
>
> while this patch uses sequence of random bytes from pg_strong_random
> function with zero byte appended.

(This is about patch 0007, not 0001)
Thanks, you are right. That's not good as-is. So this basically means
that the characters here should be from 32 to 127 included.
generate_nonce needs just to be made smarter in the way it selects the
character bytes.
-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Victor Wagner]

On Tue, 18 Oct 2016 16:35:27 +0900
Michael Paquier  wrote:

 Hi
> Attached is a rebased patch set for SCRAM, with the following things:
> - 0001, moving all the SHA2 functions to src/common/ and introducing a
> PG-like interface. No actual changes here.

It seems, that client nonce generation in this patch is not
RFC-compliant.

RFC 5802 states that SCRAM nonce should be

a sequence of random printable ASCII
  characters excluding ','

while this patch uses sequence of random bytes from pg_strong_random
function with zero byte appended.

It could cause following problems

1. If zero byte happens inside random sequence, nonce would be shorter
than expected, or even empty.

2. If one of bytes happens to be ASCII Code of comma, than server
to the client-first message, which includes copy of client nonce,
appended by server nonce,
as one of unquoted comman-separated field, would be parsed incorrectly.


Regards, Victor
-- 





-- 
Sent 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] Reload SSL certificates on SIGHUP

[Michael Paquier]

On Wed, Nov 9, 2016 at 3:48 AM, Andreas Karlsson  wrote:
> On 11/08/2016 01:22 PM, Michael Banck wrote:
>>
>> Thanks! I couldn't find furhter faults in my testing. I guess the
>> question what to do about this on Windows is possibly still open, but as
>> I am not familiar with the Windows port at all I've marked it Ready for
>> Committer for now.
>
> Thanks again for the review!

It seems to me that this patch is missing something... To begin with,
src/test/ssl/ServerSetup.pm should be patched so as the new SSL
configuration is reloaded after pg_ctl reload, and not after an
instance restart. That's straight-forward:
--- a/src/test/ssl/ServerSetup.pm
+++ b/src/test/ssl/ServerSetup.pm
@@ -96,7 +96,7 @@ sub configure_test_server_for_ssl
close HBA;
 }

-# Change the configuration to use given server cert file, and restart
+# Change the configuration to use given server cert file, and reload
 # the server so that the configuration takes effect.
 sub switch_server_cert
 {
@@ -115,6 +115,6 @@ sub switch_server_cert
print SSLCONF "ssl_crl_file='root+client.crl'\n";
close SSLCONF;

-   # Stop and restart server to reload the new config.
-   $node->restart;
+   # Reload the new configuration set.
+   $node->reload;
 }

Once I did that, half of the tests are failing. And I would have
expected all of them to work properly.
-- 
Michael
diff --git a/src/test/ssl/ServerSetup.pm b/src/test/ssl/ServerSetup.pm
index d312880..37c48d9 100644
--- a/src/test/ssl/ServerSetup.pm
+++ b/src/test/ssl/ServerSetup.pm
@@ -96,7 +96,7 @@ sub configure_test_server_for_ssl
 	close HBA;
 }
 
-# Change the configuration to use given server cert file, and restart
+# Change the configuration to use given server cert file, and reload
 # the server so that the configuration takes effect.
 sub switch_server_cert
 {
@@ -115,6 +115,6 @@ sub switch_server_cert
 	print SSLCONF "ssl_crl_file='root+client.crl'\n";
 	close SSLCONF;
 
-	# Stop and restart server to reload the new config.
-	$node->restart;
+	# Reload the new configuration set.
+	$node->reload;
 }

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

[HACKERS] Adding in docs the meaning of pg_stat_replication.sync_state

[Michael Paquier]

Hi all,

The documentation does not explain at all what means "sync" or "async"
on pg_stat_replication. The paragraph "Planning for high availability"
mentions "catchup" and "streaming", but it does not say that users can
refer to it directly in pg_stat_replication.

Thoughts about the patch attached to add a couple of sentences in
"Planning for high availability"? We could as well mention it directly
in the page of pg_stat_replication but this information seems more
suited if located in the HA section.

Thanks,
-- 
Michael
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 5bedaf2..5ef6aa9 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -1235,6 +1235,8 @@ synchronous_standby_names = '2 (s1, s2, s3)'
 will increase according to the length of time the standby has been down.
 The standby is only able to become a synchronous standby
 once it has reached streaming state.
+This is directly reported by the system view
+pg_stat_replication with the field state.

 

@@ -1259,6 +1261,12 @@ synchronous_standby_names = '2 (s1, s2, s3)'

 

+Asynchronous standbys are reported as async in the system
+view pg_stat_replication with the field
+sync_state, and synchronous standbys as sync.
+   
+
+   
 If the primary is isolated from remaining standby servers you should
 fail over to the best candidate of those other remaining standby servers.


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

Re: [HACKERS] Do we need use more meaningful variables to replace 0 in catalog head files?

[Michael Paquier]

On Wed, Nov 9, 2016 at 1:44 PM, Tom Lane  wrote:
> I don't think we need "named constants", especially not
> manually-maintained ones.  The thing that would help in pg_proc.h is for
> numeric type OIDs to be replaced by type names.  We talked awhile back
> about introducing some sort of preprocessing step that would allow doing
> that --- ie, it would look into some precursor file for pg_type.h and
> extract the appropriate OID automatically.  I'm too tired to go find the
> thread right now, but it was mostly about building the long-DATA-lines
> representation from something easier to edit.

You mean that I guess:
https://www.postgresql.org/message-id/4d191a530911041228v621286a7q6a98d9ab8a2ed...@mail.gmail.com
-- 
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] Dumb mistakes in WalSndWriteData()

[Michael Paquier]

On Mon, Oct 31, 2016 at 10:54 PM, Andres Freund  wrote:
> On 2016-10-31 09:44:16 -0400, Robert Haas wrote:
>> On Mon, Oct 31, 2016 at 4:59 AM, Andres Freund  wrote:
>> > I^Wsomebody appears to have made a number of dumb mistakes in
>> > WalSndWriteData(), namely:
>> > 1) The timestamp is set way too late, after
>> >pq_putmessage_noblock(). That'll sometimes work, sometimes not.  I
>> >have no idea how that ended up happening. It's eye-wateringly dumb.
>> >
>> > 2) We only do WalSndKeepaliveIfNecessary() if we're blocked on socket
>> >IO. But on a long-lived connection that might be a lot of data, we
>> >should really do that once *before* trying to send the payload in the
>> >first place.
>> >
>> > 3) Similarly to 2) it might be worthwhile checking for interrupts
>> >everytime, not just when blocked on network IO.
>> >
>> > See also:
>> > http://archives.postgresql.org/message-id/CAMsr%2BYE2dSfHVr7iEv1GSPZihitWX-PMkD9QALEGcTYa%2Bsdsgg%40mail.gmail.com
>>
>> Do you intend to do something about these problems?
>
> At least 1) and 2), yes. I basically wrote this email to have something
> to reference in my todo list...

Just looking at this thread, 1) and 2) is actually something like the attached?
-- 
Michael
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index bc5e508..8b3207c 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1073,9 +1073,6 @@ static void
 WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid,
 bool last_write)
 {
-	/* output previously gathered data in a CopyData packet */
-	pq_putmessage_noblock('d', ctx->out->data, ctx->out->len);
-
 	/*
 	 * Fill the send timestamp last, so that it is taken as late as possible.
 	 * This is somewhat ugly, but the protocol's set as it's already used for
@@ -1086,6 +1083,9 @@ WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid,
 	memcpy(>out->data[1 + sizeof(int64) + sizeof(int64)],
 		   tmpbuf.data, sizeof(int64));
 
+	/* output previously gathered data in a CopyData packet */
+	pq_putmessage_noblock('d', ctx->out->data, ctx->out->len);
+
 	/* fast path */
 	/* Try to flush pending output to the client */
 	if (pq_flush_if_writable() != 0)
@@ -1120,6 +1120,11 @@ WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid,
 			SyncRepInitConfig();
 		}
 
+		now = GetCurrentTimestamp();
+
+		/* Send keepalive if the time has come */
+		WalSndKeepaliveIfNecessary(now);
+
 		/* Check for input from the client */
 		ProcessRepliesIfAny();
 
@@ -1131,14 +1136,9 @@ WalSndWriteData(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid,
 		if (!pq_is_send_pending())
 			break;
 
-		now = GetCurrentTimestamp();
-
 		/* die if timeout was reached */
 		WalSndCheckTimeOut(now);
 
-		/* Send keepalive if the time has come */
-		WalSndKeepaliveIfNecessary(now);
-
 		sleeptime = WalSndComputeSleeptime(now);
 
 		wakeEvents = WL_LATCH_SET | WL_POSTMASTER_DEATH |

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

Re: [HACKERS] Do we need use more meaningful variables to replace 0 in catalog head files?

[Tom Lane]

Robert Haas  writes:
> Most of these files don't have that many entries, and they're not
> modified that often.  The elephant in the room is pg_proc.h, which is
> huge, frequently-modified, and hard to decipher.  But I think that's
> going to need more surgery than just introducing named constants -
> which would also have the downside of making the already-long lines
> even longer.

I don't think we need "named constants", especially not
manually-maintained ones.  The thing that would help in pg_proc.h is for
numeric type OIDs to be replaced by type names.  We talked awhile back
about introducing some sort of preprocessing step that would allow doing
that --- ie, it would look into some precursor file for pg_type.h and
extract the appropriate OID automatically.  I'm too tired to go find the
thread right now, but it was mostly about building the long-DATA-lines
representation from something easier to edit.

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] WIP: About CMake v2

[Tom Lane]

Craig Ringer  writes:
> On 9 Nov. 2016 06:37, "Yury Zhuravlev"  wrote:
>> This approach I see only in Postgres project and not fully understood.
>> Can you explain me more what reasons led to this approach?

> It's predictable. The default has the same result for everyone. I quite
> like it myself.

It's advantageous from a packaging standpoint, precisely because it's
predictable: either you get the set of features you expect, or you get
a build failure.  Years ago we were more on the "opportunistic" side
of things, and we had problems with packages sometimes silently losing
features.  A pretty-recent example is that OpenSSL changed their APIs
in 1.1.0 so that our configure probe failed.  With an opportunistic
approach, that would have meant builds silently going from "ssl yes"
to "ssl no".  That's not good.

So this is really not open for negotiation.  As Peter said upthread,
what we are looking for in a CMake reimplementation is that it behaves
exactly like the Autoconf version does.  To the extent that you are unable
or unwilling to duplicate that behavior, you increase the odds that
we'll reject this work.

regards, tom lane


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

Re: [HACKERS] Do we need use more meaningful variables to replace 0 in catalog head files?

[Craig Ringer]

On 9 November 2016 at 10:20, Hao Lee  wrote:
> yes, i agree with you. These catalogs are not modified often. As your said,
> the pg_proc modified often, therefore, there are another issues, the
> dependency between these system catalogs and system views. it's hard to gain
> maintenance the consistency between these catalogs and views. It's need more
> cares when do modifying. So that i think that whether there are some more
> smarter approaches to make it smarter or not.
>
> On Wed, Nov 9, 2016 at 6:33 AM, Robert Haas  wrote:
>>
>> On Mon, Nov 7, 2016 at 9:10 PM, Michael Paquier
>>  wrote:
>> > On Tue, Nov 8, 2016 at 10:57 AM, Hao Lee  wrote:
>> >> It's a tedious work to figure out these numbers real meaning. for
>> >> example,
>> >> if i want to know the value of '71'  represent what it is. I should go
>> >> back
>> >> to refer to definition of pg_class struct. It's a tedious work and it's
>> >> not
>> >> maintainable or readable.  I THINK WE SHOULD USE a meaningful variable
>> >> instead of '71'. For Example:
>> >>
>> >> #define PG_TYPE_RELTYPE 71
>> >
>> > You'd need to make genbki.pl smarter regarding the way to associate
>> > those variables with the defined variables, greatly increasing the
>> > amount of work it is doing as well as its maintenance (see for PGUID
>> > handling for example). I am not saying that this is undoable, just
>> > that the complexity may not be worth the potential readability gains.
>>
>> Most of these files don't have that many entries, and they're not
>> modified that often.  The elephant in the room is pg_proc.h, which is
>> huge, frequently-modified, and hard to decipher.  But I think that's
>> going to need more surgery than just introducing named constants -
>> which would also have the downside of making the already-long lines
>> even longer.

I'd be pretty happy to see pg_proc.h in particular replaced with some
pg_proc.h.in with something sane doing the preprocessing. It's a
massive pain right now.

-- 
 Craig Ringer   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] WIP: About CMake v2

[Craig Ringer]

On 9 November 2016 at 10:12, Michael Paquier  wrote:
> On Wed, Nov 9, 2016 at 7:54 AM, Craig Ringer
>  wrote:
>> On 9 Nov. 2016 06:37, "Yury Zhuravlev"  wrote:
>>> This approach I see only in Postgres project and not fully understood.
>>> Can you explain me more what reasons led to this approach?
>>
>> It's predictable. The default has the same result for everyone. I quite like
>> it myself.
>
> +1. Let's tell to the system what we want him to do and not let him
> guess what we'd like to be done or it will get harder to test and
> develop code for all kind of code paths with #ifdef's. That's one step
> away from Skynet.

Er... ok then. (Backs away slowly).

More seriously, I like it for development where a stable and
predictable default is great.

For users it slightly sucks, as most users will want us to find
whatever is on the system without being manually told to enable each
feature. "Of course I want SSL, I have openssl installed don't I?"
It's not like we require users to specify --enable-largefile
--enable-atomics --enable-getopt --enable-ipv6   we do detect a
lot automatically.

So personally I think it'd be fine if a cmake build defaulted to
finding and using what it could, but offered a --minimal mode or
whatever that gets us just core postgres + whatever we enable
explicitly. But our current behaviour is OK too.

-- 
 Craig Ringer   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] Bug in comparison of empty jsonb arrays to scalars

[Michael Paquier]

On Wed, Nov 9, 2016 at 8:31 AM, Nikita Glukhov  wrote:
> Hi hackers.
>
> While working on jsonbstatistics, I found the following bug:
> an empty jsonb array is considered to be lesser than any scalar,
> but it is expected that objects > arrays > scalars.

Sources? Does the JSON spec contain any information regarding
comparison operators? I don't think so, so that would be up to the
implementation to decide that, no?

Btw I would agree with you that's quite unintuitive, but that's not
wrong either to keep the current comparison algorithm because that's
harmless for btree. We could have more regression tests to make the
current behavior clear though. Thoughts from others are welcome.
-- 
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] Do we need use more meaningful variables to replace 0 in catalog head files?

[Hao Lee]

yes, i agree with you. These catalogs are not modified often. As your said,
the pg_proc modified often, therefore, there are another issues, the
dependency between these system catalogs and system views. it's hard to
gain maintenance the consistency between these catalogs and views. It's
need more cares when do modifying. So that i think that whether there are
some more smarter approaches to make it smarter or not.

On Wed, Nov 9, 2016 at 6:33 AM, Robert Haas  wrote:

> On Mon, Nov 7, 2016 at 9:10 PM, Michael Paquier
>  wrote:
> > On Tue, Nov 8, 2016 at 10:57 AM, Hao Lee  wrote:
> >> It's a tedious work to figure out these numbers real meaning. for
> example,
> >> if i want to know the value of '71'  represent what it is. I should go
> back
> >> to refer to definition of pg_class struct. It's a tedious work and it's
> not
> >> maintainable or readable.  I THINK WE SHOULD USE a meaningful variable
> >> instead of '71'. For Example:
> >>
> >> #define PG_TYPE_RELTYPE 71
> >
> > You'd need to make genbki.pl smarter regarding the way to associate
> > those variables with the defined variables, greatly increasing the
> > amount of work it is doing as well as its maintenance (see for PGUID
> > handling for example). I am not saying that this is undoable, just
> > that the complexity may not be worth the potential readability gains.
>
> Most of these files don't have that many entries, and they're not
> modified that often.  The elephant in the room is pg_proc.h, which is
> huge, frequently-modified, and hard to decipher.  But I think that's
> going to need more surgery than just introducing named constants -
> which would also have the downside of making the already-long lines
> even longer.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] WIP: About CMake v2

[Michael Paquier]

On Wed, Nov 9, 2016 at 7:54 AM, Craig Ringer
 wrote:
> On 9 Nov. 2016 06:37, "Yury Zhuravlev"  wrote:
>> This approach I see only in Postgres project and not fully understood.
>> Can you explain me more what reasons led to this approach?
>
> It's predictable. The default has the same result for everyone. I quite like
> it myself.

+1. Let's tell to the system what we want him to do and not let him
guess what we'd like to be done or it will get harder to test and
develop code for all kind of code paths with #ifdef's. That's one step
away from Skynet.
-- 
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] commitfest 2016-11 status summary

[Michael Paquier]

On Wed, Nov 9, 2016 at 10:53 AM, Robert Haas  wrote:
> Well, then it should be marked "Waiting on Author" or "Needs Review",
> not "Ready for Committer".
>
> Or maybe just "Returned with Feedback", if the reporter hasn't poked
> his head up for a few weeks.

Since yesterday with the help of Tsunakawa-san, we found a simple way
to test things by patching pg_ctl.c so "Ready for Committer"
definitely applies now. This has allowed us to draft patches that fix
the original issue as well as another issue we bumped on the way:
Postgres being started by the Local System user does not ensure that
it is a service. The instance may have been started by another service
expecting Postgres to report to stderr.

There is a refactoring patch that simplifies the code of
win32security.c for HEAD, as well as a simple back-patchable fix for
other branches.
-- 
Michael


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

[HACKERS] Bug in comparison of empty jsonb arrays to scalars

[Nikita Glukhov]

Hi hackers.

While working on jsonbstatistics, I found the following bug:
an empty jsonb array is considered to be lesser than any scalar,
but it is expected that objects > arrays > scalars.

# select '[]'::jsonb < 'null'::jsonb;
 ?column?
--
 t
(1 row)

Attached patch contains:
 1. bug fix (added the missing "else" in compareJsonbContainers())
 2. regression test
 3. pg_upgrade: invalidation of btree indexes on jsonb columns and 
REINDEX-script generation


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index ddc34ce..43934bf 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -232,7 +232,7 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b)
 		 */
 		if (va.val.array.rawScalar != vb.val.array.rawScalar)
 			res = (va.val.array.rawScalar) ? -1 : 1;
-		if (va.val.array.nElems != vb.val.array.nElems)
+		else if (va.val.array.nElems != vb.val.array.nElems)
 			res = (va.val.array.nElems > vb.val.array.nElems) ? 1 : -1;
 		break;
 	case jbvObject:
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 42bf499..81c1616 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -115,6 +115,11 @@ check_and_dump_old_cluster(bool live_check)
 	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 804)
 		new_9_0_populate_pg_largeobject_metadata(_cluster, true);
 
+	/* Pre-PG 10.0 had bug in jsonb comparison operator  */
+	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 906 &&
+		GET_MAJOR_VERSION(old_cluster.major_version) >= 904)
+		old_9_6_invalidate_jsonb_btree_indexes(_cluster, true);
+
 	/*
 	 * While not a check option, we do this now because this is the only time
 	 * the old server is running.
@@ -166,11 +171,26 @@ report_clusters_compatible(void)
 void
 issue_warnings(void)
 {
-	/* Create dummy large object permissions for old < PG 9.0? */
-	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 804)
+	bool need_new_9_0_populate_pg_largeobject_metadata =
+			GET_MAJOR_VERSION(old_cluster.major_version) <= 804;
+
+	bool need_old_9_6_invalidate_jsonb_btree_indexes =
+			GET_MAJOR_VERSION(old_cluster.major_version) <= 906 &&
+			GET_MAJOR_VERSION(old_cluster.major_version) >= 904;
+
+	if (need_new_9_0_populate_pg_largeobject_metadata ||
+		need_old_9_6_invalidate_jsonb_btree_indexes)
 	{
 		start_postmaster(_cluster, true);
-		new_9_0_populate_pg_largeobject_metadata(_cluster, false);
+
+		/* Create dummy large object permissions for old < PG 9.0? */
+		if (need_new_9_0_populate_pg_largeobject_metadata)
+			new_9_0_populate_pg_largeobject_metadata(_cluster, false);
+
+		/* invalidate jsonb btree indexes for old < PG 10.0  */
+		if (need_old_9_6_invalidate_jsonb_btree_indexes)
+			old_9_6_invalidate_jsonb_btree_indexes(_cluster, false);
+
 		stop_postmaster(false);
 	}
 }
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 19dca83..07e0ca6 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -442,6 +442,8 @@ void		pg_putenv(const char *var, const char *val);
 void new_9_0_populate_pg_largeobject_metadata(ClusterInfo *cluster,
 		 bool check_mode);
 void		old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster);
+void old_9_6_invalidate_jsonb_btree_indexes(ClusterInfo *cluster,
+			bool check_mode);
 
 /* parallel.c */
 void parallel_exec_prog(const char *log_file, const char *opt_log_file,
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index 3c7c5fa..b1a3b89 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -10,6 +10,7 @@
 #include "postgres_fe.h"
 
 #include "pg_upgrade.h"
+#include "catalog/pg_type.h"
 #include "fe_utils/string_utils.h"
 
 
@@ -185,3 +186,116 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
 	else
 		check_ok();
 }
+
+/*
+ * old_9_6_invalidate_jsonb_btree_indexes()
+ *	9.4-9.6 -> 10.0
+ *	Btree index ordering for jsonb had been fixed in 10.0
+ */
+void
+old_9_6_invalidate_jsonb_btree_indexes(ClusterInfo *cluster, bool check_mode)
+{
+	int			dbnum;
+	FILE	   *script = NULL;
+	bool		found = false;
+	char		output_path[MAXPGPATH];
+
+	prep_status("Checking for jsonb btree indexes existence");
+
+	snprintf(output_path, sizeof(output_path), "reindex_jsonb_btree.sql");
+
+	for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+	{
+		PGresult   *res;
+		bool		db_used = false;
+		int			ntups;
+		int			rowno;
+		int			i_nspname,
+	i_relname;
+		DbInfo	   *active_db = >dbarr.dbs[dbnum];
+		PGconn	   *conn = connectToServer(cluster, active_db->db_name);
+
+		/* find jsonb btree indexes */
+		res = executeQueryOrDie(conn,
+"SELECT DISTINCT n.nspname, c.relname "
+"FROM	pg_catalog.pg_class c, "
+"		pg_catalog.pg_index i, "
+"		pg_catalog.pg_am am, "
+	

Re: [HACKERS] commitfest 2016-11 status summary

[Robert Haas]

On Sun, Nov 6, 2016 at 3:33 AM, Michael Paquier
 wrote:
> On Fri, Nov 4, 2016 at 12:36 AM, Robert Haas  wrote:
>> On Tue, Nov 1, 2016 at 3:48 PM, Haribabu Kommi  
>> wrote:
>> https://commitfest.postgresql.org/11/604/ - pgwin32_is_service not
>> checking if SECURITY_SERVICE_SID is disabled
>
> This is quite a particular fix in a particular context. This is as
> well waiting some input from the reporter to be sure that the patch
> proposed is fixing what his case. The patch should fix it, but that's
> a matter to be sure now.

Well, then it should be marked "Waiting on Author" or "Needs Review",
not "Ready for Committer".

Or maybe just "Returned with Feedback", if the reporter hasn't poked
his head up for a few weeks.

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


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

Re: [HACKERS] pgbench - allow backslash continuations in \set expressions

[Robert Haas]

On Tue, Nov 8, 2016 at 5:33 AM, Fabien COELHO  wrote:
> I do not think that having both inferred continuations and explicit
> backslash continuations is desirable, it should be one or the other.

+1.  My vote is for backslash continuations.  Inferred continuations
require you to end your expressions in places where they can't legally
stop, so you can't do

\set x 2
+3

will not do the same thing as

\set x 2+
3

I don't want to get into a situation where every future bit of pgbench
syntax we want to introduce has to worry about what the interaction
with inferred continuations might be.  Backslash continuations are
kind of ugly, but it's a simple rule and virtually everybody who is
likely to be writing pgbench scripts will understand it immediately.

-- 
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] Do we need use more meaningful variables to replace 0 in catalog head files?

[Robert Haas]

On Mon, Nov 7, 2016 at 9:10 PM, Michael Paquier
 wrote:
> On Tue, Nov 8, 2016 at 10:57 AM, Hao Lee  wrote:
>> It's a tedious work to figure out these numbers real meaning. for example,
>> if i want to know the value of '71'  represent what it is. I should go back
>> to refer to definition of pg_class struct. It's a tedious work and it's not
>> maintainable or readable.  I THINK WE SHOULD USE a meaningful variable
>> instead of '71'. For Example:
>>
>> #define PG_TYPE_RELTYPE 71
>
> You'd need to make genbki.pl smarter regarding the way to associate
> those variables with the defined variables, greatly increasing the
> amount of work it is doing as well as its maintenance (see for PGUID
> handling for example). I am not saying that this is undoable, just
> that the complexity may not be worth the potential readability gains.

Most of these files don't have that many entries, and they're not
modified that often.  The elephant in the room is pg_proc.h, which is
huge, frequently-modified, and hard to decipher.  But I think that's
going to need more surgery than just introducing named constants -
which would also have the downside of making the already-long lines
even longer.

-- 
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: RV: [HACKERS] Compilation warning on 9.5

[Robert Haas]

On Mon, Nov 7, 2016 at 2:34 PM, Vicky Vergara 
wrote:

> Hello,
>
> Posting an update to this issue (which by the way also shows up on 9.6)
>
> Maybe this information is useful for extension developers (that have all
> the warnings flags on while developing using GNUC)
>
>
> By wrapping the files as follows, any warnings generated by the postgres's
> header files are ignored.
>
>
> #ifdef __GNUC__
> #pragma GCC diagnostic ignored "-pedantic"
> #endif
>
> #include "postgres.h"
>
> #ifdef __GNUC__
> #pragma GCC diagnostic pop
> #endif
>
>
> #ifdef __GNUC__
> #pragma GCC diagnostic ignored "-Wsign-conversion"
> #pragma GCC diagnostic ignored "-Wunused-parameter"
> #endif
>
> #include "executor/spi.h"
>
> #ifdef __GNUC__
> #pragma GCC diagnostic pop
> #pragma GCC diagnostic pop
> #endif
>
>
> #ifdef __GNUC__
> #pragma GCC diagnostic ignored "-Wunused-parameter"
> #endif
>
> #include "funcapi.h"
>
> #ifdef __GNUC__
> #pragma GCC diagnostic pop
> #endif
>

Wow, that's pretty ugly.  But it's useful to know that it is available in a
pinch.  Thanks!

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

Re: [HACKERS] patch proposal

[Venkata B Nagothi]

Attached is the 2nd version of the patch with some enhancements.

*Scenario 2 :*
>
> Generates Errors, Hints when the specified recovery target is prior to the
> backup's current position (xid, time and lsn). This behaviour is integrated
> with the parameters "recovery_target_time","recovery_target_lsn" and
> "recovery_target_xid".
>

In the second version of the patch, recovery_target_xid will be compared
with the "latestCompletedXid" instead of "oldestXid" of the backup and if
the specified recovery_target_xid is prior to the "latestCompletedXid" of
the backup, then errors/hints would be generated indicating the DBA to use
the appropriate backup. Now, with this patch, pg_controldata also displays
the "latestCompletedXid" of the cluster.


> Backup started at: 4/22D3B8E0, time: 2016-08-26 08:29:08
>>> Backup completed (consistency) at: 4/22D3B8EF, 2016-08-26 08:30:00
>>> recovery_target is not set
>>> recovery_target_time = 2016-08-26 08:29:30
>>> recovery_target_inclusive = false
>>>
>>> In such a case, we will necessairly commit transactions which happened
>>> between 8:29:30 and 8:30:00 and only stop (I believe..) once we've
>>> reached consistency.  We could possibly detect that case and throw an
>>> error instead.  The same could happen with xid.
>>>
>>
>> Yes, currently, PG just recovers until the consistency is reached. It
>> makes sense to throw an error instead.
>>
>
> This has not been addressed yet. Currently, the patch only considers
> generating an error if the recovery target position is prior the current
> backup's position and this is irrespective of weather backup_label is
> present or not.
> I will share my updates on how this can be addressed.
>

This does not seem to be a possibility as the information in the
backup_label is not enough to handle this scenario in specific cases like
xid or time and it does not seem possible
to add the backup stop info to the backup_label. I would prefer leaving
this to the original behaviour at the moment which is : PostgreSQL
generates
errors and exits when *EndOfLog < minRecoveryPoint *during recovery

Documentation is still pending from my end and will be submitting the same
when the patch design/approach is agreed.

Regards,

Venkata B N
Database Consultant

Fujitsu Australia


recoveryTargetIncomplete.patch-version2
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] WIP: About CMake v2

[Craig Ringer]

On 9 Nov. 2016 06:37, "Yury Zhuravlev"  wrote:
>
>> When I run cmake without options, it seems to do opportunistic feature
>> checking.  For example, it turns on OpenSSL or Python support if it can
>> find it, otherwise it turns it off.  We need this to be deterministic.
>> Without options, choose the basic feature set, require all other
>> features to be turned on explicitly, fail if they can't be found.
>> Whatever the Autoconf-based build does now has been fairly deliberately
>> tuned, so there should be very little reason to deviate from that.
>
> This approach I see only in Postgres project and not fully understood.
> Can you explain me more what reasons led to this approach?

It's predictable. The default has the same result for everyone. I quite
like it myself.

Re: [HACKERS] C based plugins, clocks, locks, and configuration variables

[Craig Ringer]

On 9 Nov. 2016 02:48, "Clifford Hammerschmidt" 
wrote:
>
> Looking closer at the bit math, I screwed it up it should be 64 bits
time, 6 bit uuid version, 8 node, 8 seq, and the rest random ... which is
42 bits of random. I'll find the code in a bit.

Huh, so that's what you are doing.

I just added the same thing to the 9.6 BDR development tree last week,
though using 64-bit values, based on a draft Petr wrote. Feel free to take
a look. bdr-plugin/dev-bdr96 branch in 2ndQuadrant/bdr github repo. The
main file is seq2.c .

Re: [HACKERS] pg_dump, pg_dumpall and data durability

[Tom Lane]

Robert Haas  writes:
> First question: Do we even want this?  Generally, when a program
> writes to a file, we rely on the operating system to decide when that
> data should be written back to disk.  We have to override that
> distinction for things internal to PostgreSQL because we need certain
> bits of data to reach the disk in a certain order, but it's unclear to
> me how far outside the core database system we want to extend that.
> Are we going to have psql fsync() data it writes to a file when \o is
> used, for example?  That would seem to me to be beyond insane, because
> we have no idea whether the user actually needs that file to be
> durable.  It is a better bet that a pg_dump command's output needs
> durability, of course, but I feel that we shouldn't just go disabling
> the filesystem cache one program at a time without some guiding
> principle.

FWIW, I find the premise pretty dubious.  People don't normally expect
programs to fsync their standard output, and the argument that pg_dump's
output is always critical data doesn't withstand inspection.  Also,
I don't understand what pg_dump should do if it fails to fsync.  There
are too many cases where that would fail (eg, output piped to a program)
for it to be treated as an error condition.  But if it isn't reported as
an error, then how much durability guarantee are we really adding?

I think this might be better addressed by adding something to backup.sgml
pointing out that you'd better fsync or sync your backups before assuming
that they can't be lost.

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 to implement pg_current_logfile() function

[Robert Haas]

On Thu, Nov 3, 2016 at 7:34 PM, Karl O. Pinc  wrote:
> To me, the CURRENT_LOG_FILENAME symbol should contain the name
> of the current log file.  It does not.  The CURRENT_LOG_FILENAME symbol
> holds the name of the file which itself contains the name of
> the log file(s) being written, plus the log file
> structure of each log file.

I agree that this is confusing.

> IMO, the name of the log files being written, as well as
> the type of data structure written into each log file,
> are meta-information about the logging data.  So maybe
> the right name is LOG_METAINFO_DATAFILE.

+1 for something like 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] SERIALIZABLE with parallel query

[Peter Geoghegan]

On Tue, Nov 8, 2016 at 1:51 PM, Thomas Munro
 wrote:
> Currently we don't generate parallel plans in SERIALIZABLE.  What
> problems need to be solved to be able to do that?

FWIW, parallel CREATE INDEX works at SERIALIZABLE isolation level by
specially asking the parallel infrastructure to not care. I think that
this works fine, given the limited scope of the problem, but it would
be nice to have that confirmed.


-- 
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] pg_sequence catalog

[Andreas Karlsson]

Review of the pg_sequences view.

This seems like a useful addition to me, making life easier for 
administrators and monitoring tools. While there is already a view in 
information_schema it is missing cache_size and last_value.


= Functional review

- The patch applies and passes the test suite without any issue.

- A worry is that it might get a bit confusing to have both the future 
catalog pg_sequence and the view pg_sequences.


- I think it would be useful to include is_cycled in the view.

- When creating a temporary sequences and then running "SELECT * FROM 
pg_sequences" in another session I get the following error.


ERROR:  cannot access temporary tables of other sessions

- Shouldn't last_value be NULL directly after we have created the 
sequence but nobody has called nextval() yet?


- I noticed that last_value includes the cached values, but that also 
seems to me like the correct thing to do.


- I do not like the name of the new function, lastval(regclass). I think 
like you suggested it would be better with something more verbose. 
sequence_lastval()? sequence_last_value()?


= Code

- There is an XXX comment still in the code. It is about the name of the 
lastval1() function.


= Documentation

- The documentation does not mention the last_value column.

- The extra empty line after "" does not fit with the formatting 
of the rest of the SGML file.


Andreas


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

Re: [HACKERS] commitfest 2016-11 status summary

[Haribabu Kommi]

Hi All,


The commitfest status summary after one week of progress:

Needs review: 78
Waiting on author: 16
Ready for Commiter: 19
Commited: 28
Moved to next CF: 0
Rejected: 3
Returned with feedback: 3
TOTAL: 147


Overall progress of completion - 23%.
week-1 progress of completion - 9%.


There are many patches that are in "ready for committer" state, committers
are working hard to move the patches, but there are many patches that are
getting added into "ready for committer" state.

There are some patches on "waiting on author" with no updates from author
for a long time, please take some action within 1-3 days, otherwise the
patch
status can be marked as "returned with feedback" for the smoother operation
of the commitfest.

There are patches with the reviewer assigned, but no review is shared yet.
Please try to share your review related to the patch in 1-3 days. In case
if the
reviewer is busy with some other work, you can remove yourself as a
 reviewer,
so that at least someone can review that patch. I will try to ask the
reviewer
in the corresponding thread if the patch didn't receive any review after 3
days.
This is not to blame some one, just to ensure the smooth progress of
commitfest.

There are still some patches with no reviewer assigned. Please consider
reviewing some patches. The review can be a code, test and etc, whichever is
comfortable to you. We need your help.

There are some authors who submitted patches to the commitfest but not
reviewing
any patch as a reviewer. Please try to review a patch that is suitable to
you. We need
your help.

Thanks everyone for smoother operation of commitfest.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] pg_dump, pg_dumpall and data durability

[Michael Paquier]

On Wed, Nov 9, 2016 at 5:48 AM, Robert Haas  wrote:
> Kyotaro Horiguchi set this patch to "Ready for Committer" in the
> CommitFest application, but that seems entirely premature to me
> considering that v2 has had no review at all, or at least not that I
> see on this thread.  I'm setting it back to "Needs Review".

That's indeed too early. Thanks for correcting.
-- 
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] WAL logging problem in 9.4.3?

[Michael Paquier]

On Wed, Nov 9, 2016 at 5:39 AM, Robert Haas  wrote:
> On Thu, Feb 4, 2016 at 7:24 AM, Heikki Linnakangas  wrote:
>> I dropped the ball on this one back in July, so here's an attempt to revive
>> this thread.
>>
>> I spent some time fixing the remaining issues with the prototype patch I
>> posted earlier, and rebased that on top of current git master. See attached.
>>
>> Some review of that would be nice. If there are no major issues with it, I'm
>> going to create backpatchable versions of this for 9.4 and below.
>
> Are you going to do commit something here?  This thread and patch are
> now 14 months old, which is a long time to make people wait for a bug
> fix.  The status in the CF is "Ready for Committer" although I am not
> sure if that's accurate.

"Needs Review" is definitely a better definition of its current state.
The last time I had a look at this patch I thought that it was in
pretty good shape (not Horiguchi-san's version, but the one in
https://www.postgresql.org/message-id/CAB7nPqR+3JjS=JB3R=axxkxcyeb-q77u-erw7_ukajctwnt...@mail.gmail.com).
With some of the recent changes, surely it needs a second look, things
related to heap handling tend to rot quickly.

I'll look into it once again by the end of this week if Heikki does
not show up, the rest will be on him I am afraid...
-- 
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] pg_dump, pg_dumpall and data durability

[Michael Paquier]

On Wed, Nov 9, 2016 at 8:18 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> First question: Do we even want this?  Generally, when a program
>> writes to a file, we rely on the operating system to decide when that
>> data should be written back to disk.  We have to override that
>> distinction for things internal to PostgreSQL because we need certain
>> bits of data to reach the disk in a certain order, but it's unclear to
>> me how far outside the core database system we want to extend that.
>> Are we going to have psql fsync() data it writes to a file when \o is
>> used, for example?  That would seem to me to be beyond insane, because
>> we have no idea whether the user actually needs that file to be
>> durable.  It is a better bet that a pg_dump command's output needs
>> durability, of course, but I feel that we shouldn't just go disabling
>> the filesystem cache one program at a time without some guiding
>> principle.
>
> FWIW, I find the premise pretty dubious.  People don't normally expect
> programs to fsync their standard output, and the argument that pg_dump's
> output is always critical data doesn't withstand inspection.  Also,
> I don't understand what pg_dump should do if it fails to fsync.  There
> are too many cases where that would fail (eg, output piped to a program)
> for it to be treated as an error condition.  But if it isn't reported as
> an error, then how much durability guarantee are we really adding?

If the output is piped to a program, there is no way to guarantee that
the data will be flushed and the user is responsible for that. We
cannot control all the use cases. The same applies for example with
pg_basebackup where the data is sent to stdout. IMO, the limit set is
that tools aimed at taking physical and logical backups should do a
better effort in the cases where they can do it. That's a cheap
insurance.

Based on this past thread, it seems to me that Magnus, Andres and Jim
Nasby are of the opinion that making things is useful:
https://www.postgresql.org/message-id/20160327233033.gd20...@awork2.anarazel.de
And so do I.

> I think this might be better addressed by adding something to backup.sgml
> pointing out that you'd better fsync or sync your backups before assuming
> that they can't be lost.

Perhaps. That would be better than nothing at least, but that won't
help for cases where we can help a bit.
-- 
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] Minor code improvement to postgresGetForeignJoinPaths

[Tatsuro Yamada]

On 2016/11/04 0:29, Robert Haas wrote:

Committed.


Thanks!

Tatsuro Yamada
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] Radix tree for character conversion

[Daniel Gustafsson]

> On 08 Nov 2016, at 17:37, Peter Eisentraut  
> wrote:
> 
> On 10/31/16 12:11 PM, Daniel Gustafsson wrote:
>> I took a small stab at doing some cleaning of the Perl scripts, mainly around
>> using the more modern (well, modern as in +15 years old) form for open(..),
>> avoiding global filehandles for passing scalar references and enforcing use
>> strict.  Some smaller typos and fixes were also included.  It seems my Perl 
>> has
>> become a bit rusty so I hope the changes make sense.  The produced files are
>> identical with these patches applied, they are merely doing cleaning as 
>> opposed
>> to bugfixing.
>> 
>> The attached patches are against the 0001-0006 patches from Heikki and you in
>> this series of emails, the separation is intended to make them easier to 
>> read.
> 
> Cool.  See also here:
> https://www.postgresql.org/message-id/55E52225.4040305%40gmx.net

Nice, not having hacked much Perl in quite a while I had all but forgotten
about perlcritic.

Running it on the current version of the patchset yields mostly warnings on
string values used in the require “convutils.pm” statement.  There were however
two more interesting reports: one more open() call not using the three
parameter form and an instance of map which alters the input value.  The latter
is not causing an issue since we don’t use the input list past the map but
fixing it seems like good form.

Attached is a patch that addresses the perlcritic reports (running without any
special options).

cheers ./daniel



fix_perlcritic.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] Typo in event_trigger.c

[Robert Haas]

On Tue, Nov 8, 2016 at 8:28 AM, Michael Paquier
 wrote:
> Hi all,
>
> I just bumped into the following:
> --- a/src/backend/commands/event_trigger.c
> +++ b/src/backend/commands/event_trigger.c
> @@ -742,7 +742,7 @@ EventTriggerCommonSetup(Node *parsetree,
>
> /*
>  * Filter list of event triggers by command tag, and copy them into our
> -* memory context.  Once we start running the command trigers, or indeed
> +* memory context.  Once we start running the command triggers, or indeed
>  * once we do anything at all that touches the catalogs, an invalidation
>  * might leave cachelist pointing at garbage, so we must do this before we
>  * can do much else.
>
> Thanks to David Steele for pointing out a similar typo in one of my patches :p

Committed.

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


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

Re: [HACKERS] WAL logging problem in 9.4.3?

[Robert Haas]

On Thu, Feb 4, 2016 at 7:24 AM, Heikki Linnakangas  wrote:
> I dropped the ball on this one back in July, so here's an attempt to revive
> this thread.
>
> I spent some time fixing the remaining issues with the prototype patch I
> posted earlier, and rebased that on top of current git master. See attached.
>
> Some review of that would be nice. If there are no major issues with it, I'm
> going to create backpatchable versions of this for 9.4 and below.

Heikki:

Are you going to do commit something here?  This thread and patch are
now 14 months old, which is a long time to make people wait for a bug
fix.  The status in the CF is "Ready for Committer" although I am not
sure if that's accurate.

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


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

Re: [HACKERS] WIP: About CMake v2

[Yury Zhuravlev]

Hello.

Peter Eisentraut wrote:

I tried this out.  Because of some file moves in initdb and
pg_basebackup, the build fails:
Yes, I need rebase patch to latest master. I do it as soon as possible. 
Also I have some new achievements. 


I suggest you use git format-patch to produce patches.  This is easier
to apply, especially when there are a lot of new files involved.  Also
use the git facilities to check for whitespace errors.
I agree. 


I suggest for now you could put this into a README.cmake file in your
patch.  We don't need to commit it that way, but it would help in the
meantime.
Good idea, currently I write all documentation on github. I will try to 
move it to patch. 


When I run cmake without options, it seems to do opportunistic feature
checking.  For example, it turns on OpenSSL or Python support if it can
find it, otherwise it turns it off.  We need this to be deterministic.
Without options, choose the basic feature set, require all other
features to be turned on explicitly, fail if they can't be found.
Whatever the Autoconf-based build does now has been fairly deliberately
tuned, so there should be very little reason to deviate from that.

This approach I see only in Postgres project and not fully understood.
Can you explain me more what reasons led to this approach?
Not big deal to make behavior like postgres Autoconf but I think it's 
important clear view here. 


The Python check appears to be picking up pieces from two different
Python installations:

Ooops you right. For Python detecting I use standard CMake module and
his behavior depending on CMake version. We should really careful here.


The check results otherwise look OK, but I'm a bit confused about the
order.  It checks for some functions before all the header files are
checked for.  Is that intentional?
I have plans to change order checks. 


There are a number of changes in .[ch] and .pl files that are unclear
and not explained.  Please explain them.  You can also submit separate
preliminary patches if you need to do some refactoring.  Ultimately, I
would expect this patch not to require C code changes.

I suppose separate patches with comments will be best way. I will do it.
I think we can talks about details after that.

Big thanks for your remarks it's very important for me and this "small" 
project.

I will try to do all tasks quickly.

Best regards. 


--
Yury Zhuravlev
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] pg_dump, pg_dumpall and data durability

[Robert Haas]

On Mon, Nov 7, 2016 at 7:52 PM, Michael Paquier
 wrote:
> On Tue, Nov 8, 2016 at 1:24 AM, Albe Laurenz  wrote:
>> The patch does not apply, I had to change the hunk for
>> src/include/common/file_utils.h.
>
> Yes, the patch has rotten a bit because of f82ec32a. 5d58c07a has also
> made the --noxxx option names appearing as --no-xxx.
>
>> Also, compilation fails because function "pre_fsync_fname" cannot be
>> resolved during linking. Is that a typo for "pre_fsync_fname" or is
>> the patch incomplete?
>
> A typo s/pre_fsync_fname/pre_sync_fname, and a mistake from me because
> I did not compile with -DPG_FLUSH_DATA_WORKS to check this code.
>
> v2 is attached, fixing those issues.

Kyotaro Horiguchi set this patch to "Ready for Committer" in the
CommitFest application, but that seems entirely premature to me
considering that v2 has had no review at all, or at least not that I
see on this thread.  I'm setting it back to "Needs Review".

First question: Do we even want this?  Generally, when a program
writes to a file, we rely on the operating system to decide when that
data should be written back to disk.  We have to override that
distinction for things internal to PostgreSQL because we need certain
bits of data to reach the disk in a certain order, but it's unclear to
me how far outside the core database system we want to extend that.
Are we going to have psql fsync() data it writes to a file when \o is
used, for example?  That would seem to me to be beyond insane, because
we have no idea whether the user actually needs that file to be
durable.  It is a better bet that a pg_dump command's output needs
durability, of course, but I feel that we shouldn't just go disabling
the filesystem cache one program at a time without some guiding
principle.

-- 
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] [bug fix] Cascading standby cannot catch up and get stuck emitting the same message repeatedly

[Robert Haas]

On Thu, Aug 25, 2016 at 10:33 PM, Tsunakawa, Takayuki
 wrote:
> The processing went as follows.
>
> 1. node1's timeline is 140.  It wrote a WAL record at the end of WAL segment 
> 117.  The WAL record didn't fit the last page, so it was split across 
> segments 117 and 118.
>
> 2. WAL segment 117 was archived.
>
> 3. node1 got down, and node2 was promoted.
>
> 4. As part of the recovery process, node2 retrieves WAL segment 117 from 
> archive.  It found a WAL record fragment at the end of the segment but could 
> not find the remaining fragment in segment 118, so node2 stops recovery there.
>
> LOG:  restored log file "008C028C0075" from archive
> LOG:  received promote request
> LOG:  redo done at 28C/75FFF738
>
> 5. node2 becomes the primary, and its timeline becomes 118.  node3 is 
> disconnected by node2 (but later reconnectes to node2).
>
> LOG:  terminating all walsender processes to force cascaded standby(s) to 
> update timeline and reconnect
>
> 6. node3 retrieves and applies WAL segment 117 from archive.
>
> LOG:  restored log file "008C028C0075" from archive
>
> 7. node3 found .history file for time line 141 and renews its timeline to 141.
>
> 8. Because node3 found a WAL record fragment at the end of segment 117, it 
> expects to find the remaining fragment at the beginning of WAL segment 118 
> streamed from node2.  But there was a fragment of a different WAL record, 
> because node2 overwrote a different WAL record at the end of segment 117 
> across to 118.
>
> LOG:  invalid contrecord length 5892 in log file 652, segment 118, offset 0
>
> 9. node3 then retrieves segment 117 from archive again to get the WAL record 
> at the end of segment 117.  However, as node3's timeline is already 141, it 
> complains about the older timeline when it sees the timeline 140 at the 
> beginning of segment 117.
>
> LOG:  out-of-sequence timeline ID 140 (after 141) in log file 652, segment 
> 117, offset 0

OK.  I agree that's a problem.  However, your patch adds zero new
comment text while removing some existing comments, so I can't easily
tell how it solves that problem or whether it does so correctly.  Even
if I were smart enough to figure it out, I wouldn't want to rely on
the next person also being that smart.  This is obviously a subtle
problem in tricky code, so a clear explanation of the fix seems like a
very good idea.

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


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

Re: [HACKERS] Remove "Source Code" column from \df+ ?

[Robert Haas]

On Wed, Oct 12, 2016 at 1:48 PM, Peter Eisentraut
 wrote:
> On 10/12/16 11:16 AM, Tom Lane wrote:
>> I'm not sure that Peter was voting for retaining "internal name", but
>> personally I prefer that to deleting prosrc entirely, so +1.
>
> I'm not sure what the point of showing the internal name would be if we
> have already declared that the source code of non-C functions is not
> that interesting.  But I don't have a strong feeling about it.

There is still an open CommitFest entry for this patch, which is
marked "Ready for Committer", but it looks to me like there's no
consensus position here.  Different people have different preferences,
and every option that is somebody's first preference seems to be
somebody else's last preference.  So I suggest that we give this one
up for a lost cause and mark it Rejected.

-- 
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] Remove "Source Code" column from \df+ ?

[Tom Lane]

Robert Haas  writes:
> There is still an open CommitFest entry for this patch, which is
> marked "Ready for Committer", but it looks to me like there's no
> consensus position here.  Different people have different preferences,
> and every option that is somebody's first preference seems to be
> somebody else's last preference.  So I suggest that we give this one
> up for a lost cause and mark it Rejected.

Yeah, that's about where I'm at on it 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

[HACKERS] Re: [HACKERS] Re: [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …

[Robert Haas]

On Tue, Nov 8, 2016 at 8:53 AM, Dagfinn Ilmari Mannsåker
 wrote:
> Thank you very much.  I just looked at the patch again and realised the
> completion of "TO" after RENAME VALUE  can be merged with the one
> for RENAME ATTRIBUTE .  Attached is an updated and patch

Committed.

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


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

Re: [HACKERS] Remove "Source Code" column from \df+ ?

[Robert Haas]

On Tue, Nov 8, 2016 at 4:18 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> There is still an open CommitFest entry for this patch, which is
>> marked "Ready for Committer", but it looks to me like there's no
>> consensus position here.  Different people have different preferences,
>> and every option that is somebody's first preference seems to be
>> somebody else's last preference.  So I suggest that we give this one
>> up for a lost cause and mark it Rejected.
>
> Yeah, that's about where I'm at on it too.

Done.  We can reopen this if a vigorous consensus emerges.

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


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

[HACKERS] SERIALIZABLE with parallel query

[Thomas Munro]

Hi hackers,

Currently we don't generate parallel plans in SERIALIZABLE.  What
problems need to be solved to be able to do that?  I'm probably
steamrolling over a ton of subtleties and assumptions here, but it
occurred to me that a first step might be something like this:

1.  Hand the leader's SERIALIZABLEXACT to workers.
2.  Make sure that workers don't release predicate locks.
3.  Make sure that the leader doesn't release predicate locks until
after the workers have finished accessing the leader's
SERIALIZABLEXACT.  I think this is already the case.

See attached 5 minute hack.  Need to audit predicate.c for cases where
MySerializableXact might be modified without suitable locking, and
probably sprinkle assertions all over the place that workers don't
reach certain places etc.  I wonder what horrible things might happen
as a result of workers running with a SERIALIZABLEXACT that contains
the leader's vxid and other such things.  I'd love to figure all this
out in time for one of the later CFs in this cycle.  Any thoughts?

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


ssi-parallel-hack.patch
Description: Binary data

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

Re: [HACKERS] Re: [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …

[Dagfinn Ilmari Mannsåker]

Robert Haas  writes:

> On Tue, Nov 8, 2016 at 8:53 AM, Dagfinn Ilmari Mannsåker
>  wrote:
>> Thank you very much.  I just looked at the patch again and realised the
>> completion of "TO" after RENAME VALUE  can be merged with the one
>> for RENAME ATTRIBUTE .  Attached is an updated and patch
>
> Committed.

Thanks!

-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law


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

Re: [HACKERS] Typo in event_trigger.c

[Michael Paquier]

On Wed, Nov 9, 2016 at 5:35 AM, Robert Haas  wrote:
> Committed.

Thanks.
-- 
Michael


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

[HACKERS] pg_serial early wraparound

[Thomas Munro]

Hi hackers,

The SLRU managed by predicate.c can wrap around and overwrite data if
you have more than 1 billion active XIDs.  That's because when SSI was
implemented, slru.c was limited to four digit segment names, which
implied a page limit that wasn't enough for pg_serial to have space
for every possible XID.  We should probably rip that code out, because
SLRUs now support five digit segment names.  Something like the
attached.  I'll post a test script to demonstrate correct wraparound
behaviour around in time for one of the later CFs.

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


ssi-slru-wraparound-v1.patch
Description: Binary data

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

Re: [HACKERS] Hash Indexes

[Robert Haas]

On Mon, Nov 7, 2016 at 9:51 PM, Amit Kapila  wrote:
> [ new patches ]

Attached is yet another incremental patch with some suggested changes.

+ * This expects that the caller has acquired a cleanup lock on the target
+ * bucket (primary page of a bucket) and it is reponsibility of caller to
+ * release that lock.

This is confusing, because it makes it sound like we retain the lock
through the entire execution of the function, which isn't always true.
I would say that caller must acquire a cleanup lock on the target
primary bucket page before calling this function, and that on return
that page will again be write-locked.  However, the lock might be
temporarily released in the meantime, which visiting overflow pages.
(Attached patch has a suggested rewrite.)

+ * During scan of overflow pages, first we need to lock the next bucket and
+ * then release the lock on current bucket.  This ensures that any concurrent
+ * scan started after we start cleaning the bucket will always be behind the
+ * cleanup.  Allowing scans to cross vacuum will allow it to remove tuples
+ * required for sanctity of scan.

This comment says that it's bad if other scans can pass our cleanup
scan, but it doesn't explain why.  I think it's because we don't have
page-at-a-time mode yet, and cleanup might decrease the TIDs for
existing index entries.  (Attached patch has a suggested rewrite, but
might need further adjustment if my understanding of the reasons is
incomplete.)

+if (delay)
+vacuum_delay_point();

You don't really need "delay".  If we're not in a cost-accounted
VACUUM, vacuum_delay_point() just turns into CHECK_FOR_INTERRUPTS(),
which should be safe (and a good idea) regardless.  (Fixed in
attached.)

+if (callback && callback(htup, callback_state))
+{
+/* mark the item for deletion */
+deletable[ndeletable++] = offno;
+if (tuples_removed)
+*tuples_removed += 1;
+}
+else if (bucket_has_garbage)
+{
+/* delete the tuples that are moved by split. */
+bucket = _hash_hashkey2bucket(_hash_get_indextuple_hashkey(itup
),
+  maxbucket,
+  highmask,
+  lowmask);
+/* mark the item for deletion */
+if (bucket != cur_bucket)
+{
+/*
+ * We expect tuples to either belong to curent bucket or
+ * new_bucket.  This is ensured because we don't allow
+ * further splits from bucket that contains garbage. See
+ * comments in _hash_expandtable.
+ */
+Assert(bucket == new_bucket);
+deletable[ndeletable++] = offno;
+}
+else if (num_index_tuples)
+*num_index_tuples += 1;
+}
+else if (num_index_tuples)
+*num_index_tuples += 1;
+}

OK, a couple things here.  First, it seems like we could also delete
any tuples where ItemIdIsDead, and that seems worth doing. In fact, I
think we should check it prior to invoking the callback, because it's
probably quite substantially cheaper than the callback.  Second,
repeating deletable[ndeletable++] = offno and *num_index_tuples += 1
doesn't seem very clean to me; I think we should introduce a new bool
to track whether we're keeping the tuple or killing it, and then use
that to drive which of those things we do.  (Fixed in attached.)

+if (H_HAS_GARBAGE(bucket_opaque) &&
+!H_INCOMPLETE_SPLIT(bucket_opaque))

This is the only place in the entire patch that use
H_INCOMPLETE_SPLIT(), and I'm wondering if that's really correct even
here. Don't you really want H_OLD_INCOMPLETE_SPLIT() here?  (And
couldn't we then remove H_INCOMPLETE_SPLIT() itself?) There's no
garbage to be removed from the "new" bucket until the next split, when
it will take on the role of the "old" bucket.

I think it would be a good idea to change this so that
LH_BUCKET_PAGE_HAS_GARBAGE doesn't get set until
LH_BUCKET_OLD_PAGE_SPLIT is cleared.  The current way is confusing,
because those tuples are NOT garbage until the split is completed!
Moreover, both of the places that care about
LH_BUCKET_PAGE_HAS_GARBAGE need to make sure that
LH_BUCKET_OLD_PAGE_SPLIT is clear before they do anything about
LH_BUCKET_PAGE_HAS_GARBAGE, so the change I am proposing would
actually simplify the code very slightly.

+#define H_OLD_INCOMPLETE_SPLIT(opaque)  ((opaque)->hasho_flag &
LH_BUCKET_OLD_PAGE_SPLIT)
+#define H_NEW_INCOMPLETE_SPLIT(opaque)  ((opaque)->hasho_flag &
LH_BUCKET_NEW_PAGE_SPLIT)

The code isn't consistent about the use of these macros, or at least
not in a good way.  When you care about 

Re: [HACKERS] WIP: About CMake v2

[Peter Eisentraut]

On 9/17/16 1:21 PM, Yury Zhuravlev wrote:
> Now, I published the first version of the patch. 

I tried this out.  Because of some file moves in initdb and
pg_basebackup, the build fails:

[ 74%] Linking C executable initdb
 Undefined symbols for architecture x86_64:
   "_fsync_pgdata", referenced from:
   _main in initdb.c.o
 ld: symbol(s) not found for architecture x86_64
 collect2: error: ld returned 1 exit status
 make[2]: *** [src/bin/initdb/CMakeFiles/initdb.dir/build.make:177:
src/bin/initdb/initdb] Error 1
 make[1]: *** [CMakeFiles/Makefile2:2893:
src/bin/initdb/CMakeFiles/initdb.dir/all] Error 2
 make: *** [Makefile:128: all] Error 2

Please submit an updated patch.

I suggest you use git format-patch to produce patches.  This is easier
to apply, especially when there are a lot of new files involved.  Also
use the git facilities to check for whitespace errors.

Please supply some documentation, such as

- what are the basic commands
- how to set include/library paths, choose configure options
- how to set CFLAGS
- how to see raw build commands
- what are the targets for all/world/check/docs etc.
- explain directory structure

I suggest for now you could put this into a README.cmake file in your
patch.  We don't need to commit it that way, but it would help in the
meantime.

When I run cmake without options, it seems to do opportunistic feature
checking.  For example, it turns on OpenSSL or Python support if it can
find it, otherwise it turns it off.  We need this to be deterministic.
Without options, choose the basic feature set, require all other
features to be turned on explicitly, fail if they can't be found.
Whatever the Autoconf-based build does now has been fairly deliberately
tuned, so there should be very little reason to deviate from that.

The Python check appears to be picking up pieces from two different
Python installations:

 -- Found PythonInterp: /usr/local/bin/python (found version "2.7.12")
 -- Found PythonLibs: /usr/lib/libpython2.7.dylib (found version "2.7.10")

The check results otherwise look OK, but I'm a bit confused about the
order.  It checks for some functions before all the header files are
checked for.  Is that intentional?

There are a number of changes in .[ch] and .pl files that are unclear
and not explained.  Please explain them.  You can also submit separate
preliminary patches if you need to do some refactoring.  Ultimately, I
would expect this patch not to require C code changes.

-- 
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] Logical Replication WIP

[Peter Eisentraut]

On 11/4/16 9:00 AM, Andres Freund wrote:
> +  
> +   The pg_publication_rel catalog contains
> +   mapping between tables and publications in the database. This is many to
> +   many mapping.
> +  
> 
> I wonder if we shouldn't abstract this a bit away from relations to
> allow other objects to be exported to. Could structure it a bit more
> like pg_depend.

I think we can add/change that when we have use for it.

> +   
> +FOR TABLE ALL IN SCHEMA
> +
> + 
> +  Specifies optional schema for which all logged tables will be added to
> +  publication.
> + 
> +
> +   
> 
> "FOR TABLE ALL IN SCHEMA" sounds weird.

That clause no longer exists anyway.

> +  
> +   This operation does not reserve any resources on the server. It only
> +   defines grouping and filtering logic for future subscribers.
> +  
> 
> That's strictly speaking not true, maybe rephrase a bit?

Maybe the point is that it does not initiate any contact with remote nodes.

> +/*
> + * Create new publication.
> + * TODO ACL check
> + */
> 
> Hm?

The first patch is going to be just superuser and replication role.  I'm
working on a patch set for later that adds proper ACLs, owners, and all
that.  So I'd suggest to ignore these details for now, unless of course
you find permission checks *missing*.

> +/*
> + * Drop publication by OID
> + */
> +void
> +DropPublicationById(Oid pubid)
> +
> +/*
> + * Remove relation from publication by mapping OID.
> + */
> +void
> +RemovePublicationRelById(Oid proid)
> +{
> 
> Permission checks?
> 
> +}
> 
> Hm. Neither of these does dependency checking, wonder if that can be
> argued to be problematic.

The dependency checking is done before it gets to these functions, no?

> /*
> + * Check if command can be executed with current replica identity.
> + */
> +static void
> +CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
> +{
> + PublicationActions *pubactions;
> +
> + /* We only need to do checks for UPDATE and DELETE. */
> + if (cmd != CMD_UPDATE && cmd != CMD_DELETE)
> + return;
> +
> + /* If relation has replica identity we are always good. */
> + if (rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL ||
> + OidIsValid(RelationGetReplicaIndex(rel)))
> + return;
> +
> + /*
> +  * This is either UPDATE OR DELETE and there is no replica identity.
> +  *
> +  * Check if the table publishes UPDATES or DELETES.
> +  */
> + pubactions = GetRelationPublicationActions(rel);
> + if (pubactions->pubupdate || pubactions->pubdelete)
> 
> I think that leads to spurious errors. Consider a DELETE with a
> publication that replicates updates but not deletes.

Yeah, it needs to check the pubactions against the specific command.

> +} FormData_pg_publication;
> 
> Shouldn't this have an owner?

Yes, see above.

> I also wonder if we want an easier to
> extend form of pubinsert/update/delete (say to add pubddl, pubtruncate,
> pub ... without changing the schema).

Maybe, but how?  (without using weird array constructs that are a pain
to parse in psql and pg_dump, for example)

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


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

Re: [HACKERS] Logical Replication WIP

[Peter Eisentraut]

Review of v7 0003-Add-PUBLICATION-catalogs-and-DDL.patch:

This appears to address previous reviews and is looking pretty solid.  I
have some comments that are easily addressed:

[still from previous review] The code for OCLASS_PUBLICATION_REL in
getObjectIdentityParts() does not fill in objname and objargs, as it is
supposed to.

catalog.sgml: pg_publication_rel column names must be updated after renaming

alter_publication.sgml and elsewhere: typos PuBLISH_INSERT etc.

create_publication.sgml: FOR TABLE ALL IN SCHEMA does not exist anymore

create_publication.sgml: talks about not-yet-existing SUBSCRIPTION role

DropPublicationById maybe name RemovePublicationById for consistency

system_views.sql: C.relkind = 'r' unnecessary

CheckCmdReplicaIdentity: error message says "cannot update", should
distinguish between update and delete

relcache.c: pubactions->pubinsert |= pubform->pubinsert; etc. should be ||=

RelationData.rd_pubactions could be a bitmap, simplifying some memcpy
and context management.  But RelationData appears to favor rich data
structures, so maybe that is fine.

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


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

Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

[Andreas Karlsson]

On 11/08/2016 01:22 PM, Michael Banck wrote:

Thanks! I couldn't find furhter faults in my testing. I guess the
question what to do about this on Windows is possibly still open, but as
I am not familiar with the Windows port at all I've marked it Ready for
Committer for now.


Thanks again for the review!

Andreas




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

Re: [HACKERS] C based plugins, clocks, locks, and configuration variables

[Clifford Hammerschmidt]

Looking closer at the bit math, I screwed it up it should be 64 bits
time, 6 bit uuid version, 8 node, 8 seq, and the rest random ... which is
42 bits of random. I'll find the code in a bit.

-- 
Clifford Hammerschmidt, P.Eng.

On Tue, Nov 8, 2016 at 9:42 AM, Clifford Hammerschmidt <
tanglebo...@gmail.com> wrote:

> Hi Jim,
>
> The values are still globally unique. The odds of a collision are very
> very low. Two instances with the same node_id generating on the same
> millisecond (in their local view of time) have a 1:2^34 chance of
> collision. node_id only repeats every 256 machines in a cluster (assuming
> you're configured correctly), and the probability of the same millisecond
> being used on both machines is also low (depends on generation rate and
> machine speed). The only real concern is with clock replays (i.e. something
> sets the clock backwards, like an admin or a badly implemented time sync
> system), which does happen in rare instances and is why seq is there to
> extend that space out and reduce the chance of a collision in that
> millisecond. (time replays are a real problem with id systems like
> snowflake.)
>
> Also, the point of the timestamp isn't uniqueness, it's the generally
> monotonically ascending aspect I want. This causes inserts to append to the
> index (much faster than random inserts in large indexes because of cache
> coherency), and causes data generated around the same time to occupy near
> nodes in the index (again, cache benefits, as related data tends to be
> generated bunched up in time).
>
> Thanks,
> -Cliff.
>
> --
> Clifford Hammerschmidt, P.Eng.
>
> On Tue, Nov 8, 2016 at 6:27 AM, Jim Nasby 
> wrote:
>
>> On 11/3/16 7:14 PM, Craig Ringer wrote:
>>
>>> 1) getting microseconds (or nanoseconds) from UTC epoch in a plugin

>>>
>>> GetCurrentIntegerTimestamp()
>>>
>>
>> Since you're serializing generation anyway you might want to just forgo
>> the timestamp completely. It's not like the values your generating are
>> globally unique anymore, or hard to guess.
>> --
>> 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
>> 855-TREBLE2 (855-873-2532)   mobile: 512-569-9461
>>
>
>

Re: [HACKERS] Fix bug in handling of dropped columns in pltcl triggers

[Tom Lane]

Jim Nasby  writes:
> On 11/4/16 4:28 PM, Tom Lane wrote:
>> My proposal therefore is for SPI_fnumber to ignore (not match to)
>> dropped columns, and to remove any caller-side attisdropped tests that
>> thereby become redundant.

> Yeah, SPI users certainly shouldn't need to worry about attisdropped, 
> and exposing attnum doesn't seem like a good idea either.

OK, I pushed a patch to centralize this in SPI_fnumber.

regards, tom lane


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

[HACKERS] Make pg_basebackup -x stream the default

[Magnus Hagander]

Per some discussions with a number of different people at pgconfeu, here is
a patch that changes the default mode of pg_basebackup to be streaming the
wal, as this is what most users would want -- and those that don't want it
have to make other changes as well. Doing the "most safe" thing by default
is a good idea.

The new option "-x none" is provided to turn this off and get the previous
behavior back.

I will add this to the next (January) commitfest.

//Magnus



-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 1f15a17..0db0ffb 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -302,10 +302,10 @@ PostgreSQL documentation

 Includes the required transaction log files (WAL files) in the
 backup. This will include all transaction logs generated during
-the backup. If this option is specified, it is possible to start
-a postmaster directly in the extracted directory without the need
-to consult the log archive, thus making this a completely standalone
-backup.
+the backup. Unless the option none is specified,
+it is possible to start a postmaster directly in the extracted
+directory without the need to consult the log archive, thus
+making this a completely standalone backup.


 The following methods for collecting the transaction logs are
@@ -313,6 +313,16 @@ PostgreSQL documentation
 
 
  
+  n
+  none
+  
+   
+Don't include transaction log in the backup.
+   
+  
+ 
+
+ 
   f
   fetch
   
@@ -349,6 +359,9 @@ PostgreSQL documentation
 named pg_wal.tar (if the server is a version
 earlier than 10, the file will be named pg_xlog.tar).

+   
+This value is the default.
+   
   
  
 
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index e2875df..7e294d8 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -71,8 +71,9 @@ static bool noclean = false;
 static bool showprogress = false;
 static int	verbose = 0;
 static int	compresslevel = 0;
-static bool includewal = false;
-static bool streamwal = false;
+static bool includewal = true;
+static bool streamwal = true;
+static bool set_walmode = false;
 static bool fastcheckpoint = false;
 static bool writerecoveryconf = false;
 static bool do_sync = true;
@@ -326,7 +327,7 @@ usage(void)
 	printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
 	  " relocate tablespace in OLDDIR to NEWDIR\n"));
 	printf(_("  -x, --xlog include required WAL files in backup (fetch mode)\n"));
-	printf(_("  -X, --xlog-method=fetch|stream\n"
+	printf(_("  -X, --xlog-method=none|fetch|stream\n"
 			 " include required WAL files with specified method\n"));
 	printf(_("  --xlogdir=XLOGDIR  location for the transaction log directory\n"));
 	printf(_("  -z, --gzip compress tar output\n"));
@@ -1700,7 +1701,11 @@ BaseBackup(void)
 	 */
 	if (streamwal && !CheckServerVersionForStreaming(conn))
 	{
-		/* Error message already written in CheckServerVersionForStreaming() */
+		/*
+		 * Error message already written in CheckServerVersionForStreaming(),
+		 * but add a hint about using -X none.
+		 */
+		fprintf(stderr, _("HINT: use -X none or -X fetch to disable log streaming\n"));
 		disconnect_and_exit(1);
 	}
 
@@ -2112,7 +2117,7 @@ main(int argc, char **argv)
 tablespace_list_append(optarg);
 break;
 			case 'x':
-if (includewal)
+if (set_walmode)
 {
 	fprintf(stderr,
 	 _("%s: cannot specify both --xlog and --xlog-method\n"),
@@ -2120,11 +2125,12 @@ main(int argc, char **argv)
 	exit(1);
 }
 
+set_walmode = true;
 includewal = true;
 streamwal = false;
 break;
 			case 'X':
-if (includewal)
+if (set_walmode)
 {
 	fprintf(stderr,
 	 _("%s: cannot specify both --xlog and --xlog-method\n"),
@@ -2132,13 +2138,30 @@ main(int argc, char **argv)
 	exit(1);
 }
 
-includewal = true;
-if (strcmp(optarg, "f") == 0 ||
+/*
+ * Indicate that this has been configured, so we can complain
+ * if it's configured more than once.
+ */
+set_walmode = true;
+
+if (strcmp(optarg, "n") == 0 ||
+	strcmp(optarg, "none") == 0)
+{
+	includewal = false;
+	streamwal = false;
+}
+else if (strcmp(optarg, "f") == 0 ||
 	strcmp(optarg, "fetch") == 0)
+{
+	includewal = true;
 	streamwal = false;
+}
 else if (strcmp(optarg, "s") == 0 ||
 		 strcmp(optarg, "stream") == 0)
+{
+	includewal = true;
 	streamwal = 

Re: [HACKERS] C based plugins, clocks, locks, and configuration variables

[Clifford Hammerschmidt]

Hi Jim,

The values are still globally unique. The odds of a collision are very very
low. Two instances with the same node_id generating on the same millisecond
(in their local view of time) have a 1:2^34 chance of collision. node_id
only repeats every 256 machines in a cluster (assuming you're configured
correctly), and the probability of the same millisecond being used on both
machines is also low (depends on generation rate and machine speed). The
only real concern is with clock replays (i.e. something sets the clock
backwards, like an admin or a badly implemented time sync system), which
does happen in rare instances and is why seq is there to extend that space
out and reduce the chance of a collision in that millisecond. (time replays
are a real problem with id systems like snowflake.)

Also, the point of the timestamp isn't uniqueness, it's the generally
monotonically ascending aspect I want. This causes inserts to append to the
index (much faster than random inserts in large indexes because of cache
coherency), and causes data generated around the same time to occupy near
nodes in the index (again, cache benefits, as related data tends to be
generated bunched up in time).

Thanks,
-Cliff.

-- 
Clifford Hammerschmidt, P.Eng.

On Tue, Nov 8, 2016 at 6:27 AM, Jim Nasby  wrote:

> On 11/3/16 7:14 PM, Craig Ringer wrote:
>
>> 1) getting microseconds (or nanoseconds) from UTC epoch in a plugin
>>>
>>
>> GetCurrentIntegerTimestamp()
>>
>
> Since you're serializing generation anyway you might want to just forgo
> the timestamp completely. It's not like the values your generating are
> globally unique anymore, or hard to guess.
> --
> 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
> 855-TREBLE2 (855-873-2532)   mobile: 512-569-9461
>

Re: [HACKERS] Write Ahead Logging for Hash Indexes

[Jeff Janes]

On Sat, Sep 24, 2016 at 10:00 PM, Amit Kapila 
wrote:

> On Fri, Sep 23, 2016 at 5:34 PM, Amit Kapila 
> wrote:
> >
> > I think here I am slightly wrong.  For the full page writes, it do use
> > RBM_ZERO_AND_LOCK mode to read the page and for such mode we are not
> > doing page verification check and rather blindly setting the page to
> > zero and then overwrites it with full page image.  So after my fix,
> > you will not see the error of checksum failure.  I have a fix ready,
> > but still doing some more verification.  If everything passes, I will
> > share the patch in a day or so.
> >
>
> Attached patch fixes the problem, now we do perform full page writes
> for bitmap pages.  Apart from that, I have rebased the patch based on
> latest concurrent index patch [1].  I have updated the README as well
> to reflect the WAL logging related information for different
> operations.
>
> With attached patch, all the review comments or issues found till now
> are addressed.
>

This needs to be updated to apply over concurrent_hash_index_v10.patch.

Unless we want to wait until that work is committed before doing more
review and testing on this.

Thanks,

Jeff

Re: [HACKERS] Incorrect overflow check condition for WAL segment size

[Robert Haas]

On Tue, Nov 8, 2016 at 1:01 AM, Michael Paquier
 wrote:
> On Tue, Nov 8, 2016 at 2:33 PM, Kuntal Ghosh  
> wrote:
>> Either the comment is wrongly written or the check for overflow
>> condition has to be fixed. Assuming the overflow check condition to be
>> erroneous, I've attached a patch to fix this.
>
> Good catch. Interesting copy-pasto from 88e9823.

Committed.

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


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

Re: [HACKERS] Improving RLS planning

[Robert Haas]

On Thu, Nov 3, 2016 at 6:23 PM, Tom Lane  wrote:
>> I think that ordering might be sub-optimal if you had a mix of
>> leakproof quals and security quals and the cost of some security quals
>> were significantly higher than the cost of some other quals. Perhaps
>> all leakproof quals should be assigned security_level 0, to allow them
>> to be checked earlier if they have a lower cost (whether or not they
>> are security quals), and only leaky quals would have a security_level
>> greater than zero. Rule 1 would then not need to check whether the
>> qual was leakproof, and you probably wouldn't need the separate
>> "leakproof" bool field on RestrictInfo.
>
> Hm, but it would also force leakproof quals to be evaluated in front
> of potentially-cheaper leaky quals, whether or not that's semantically
> necessary.
>
> I experimented with ignoring security_level altogether for leakproof
> quals, but I couldn't make it work properly, because that didn't lead to
> a comparison rule that satisfies transitivity.  For instance, consider
> three quals:
> A: cost 1, security_level 1, leaky
> B: cost 2, security_level 1, leakproof
> C: cost 3, security_level 0, leakproof
> A should sort before B, since same security_level and lower cost;
> B should sort before C, since lower cost and leakproof;
> but A must sort after C, since higher security_level and leaky.

Yeah, this is pretty thorny.  IIUC, all leaky quals of a given
security level must be evaluated before any quals of the next higher
security level, or we have a security problem.  Beyond that, we'd
*prefer* to evaluate cheaper quals first (though perhaps we ought to
be also thinking about how selective they are) but that's "just" a
matter of how good the query plan is.  So in this example, security
dictates that C must precede A, but that's it.  We can pick between
C-A-B, C-B-A, and B-C-A based on cost.  C-B-A is clearly inferior to
either of the other two, but it's less obvious whether C-A-B or B-C-A
is better.  If you expect each predicate to have a selectivity of 50%,
then C-A-B costs 3+(0.5*1)+(0.25*2) = 4 while B-C-A costs
2+(0.5*3)+(0.25*1) = 3.75, so B-C-A is better.  But now make the cost
of B and C 18 and 20 while keeping the cost of A at 1.  Now C-A-B
costs 20+(0.5*1)+(0.25*18) = 25 while B-C-A costs 18+(0.5*20)+(0.25*1)
= 28.25, so now C-A-B is better.

So I think any attempt to come up with a transitive comparison rule is
doomed.  We could do something like: sort by cost then security level;
afterwards, allow leakproof qual to migrate forward as many position
as is possible without passing a qual that is either higher-cost or
(non-leakproof and lower security level).  So in the above example we
would start by sorting the like C-A-B and then check whether B can
move forward; it can't, so we're done.  If all operators were
leakproof, this would essentially turn into an insertion-sort that
orders them strictly by cost, whereas if they're all leaky, it orders
strictly by security level and then by cost.  With a mix of leaky and
non-leaky operators you get something in the middle.

I'm not sure that this is practically better than the hack you
proposed, but I wanted to take the time to comment on the theory here,
as I see it anyway.

-- 
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] Radix tree for character conversion

[Peter Eisentraut]

On 10/31/16 12:11 PM, Daniel Gustafsson wrote:
> I took a small stab at doing some cleaning of the Perl scripts, mainly around
> using the more modern (well, modern as in +15 years old) form for open(..),
> avoiding global filehandles for passing scalar references and enforcing use
> strict.  Some smaller typos and fixes were also included.  It seems my Perl 
> has
> become a bit rusty so I hope the changes make sense.  The produced files are
> identical with these patches applied, they are merely doing cleaning as 
> opposed
> to bugfixing.
> 
> The attached patches are against the 0001-0006 patches from Heikki and you in
> this series of emails, the separation is intended to make them easier to read.

Cool.  See also here:
https://www.postgresql.org/message-id/55E52225.4040305%40gmx.net

-- 
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] Hash Indexes

[Robert Haas]

On Mon, Nov 7, 2016 at 9:51 PM, Amit Kapila  wrote:
>> Both the places _hash_squeezebucket() and  _hash_splitbucket can use
>> this optimization irrespective of rest of the patch.  I will prepare a
>> separate patch for these and send along with main patch after some
>> testing.
>
> Done as a separate patch skip_dead_tups_hash_index-v1.patch.

Thanks.  Committed.

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


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

Re: [HACKERS] unchecked chdir warning

[Tom Lane]

Jeff Janes  writes:
> I get the warning:

> ipc.c: In function 'proc_exit':
> ipc.c:137:3: warning: ignoring return value of 'chdir', declared with
> attribute warn_unused_result [-Wunused-result]
>chdir(gprofDirName);
>^

> I don't know if there is anything useful that can be done if the chdir
> fails.  What is the best way to silence the warning?

See, eg, aa90e148c.

It used to be considered good enough to explicitly cast the function
result to void, but the pedants who invented -Wunused-result refused
to honor that convention.

regards, tom lane


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

[HACKERS] unchecked chdir warning

[Jeff Janes]

When compiling with  --enable-profiling using:

gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.2)

I get the warning:

ipc.c: In function 'proc_exit':
ipc.c:137:3: warning: ignoring return value of 'chdir', declared with
attribute warn_unused_result [-Wunused-result]
   chdir(gprofDirName);
   ^

I don't know if there is anything useful that can be done if the chdir
fails.  What is the best way to silence the warning?

Cheers,

Jeff

Re: [HACKERS] Danger of automatic connection reset in psql

[Oleksandr Shulgin]

On Mon, Nov 7, 2016 at 9:31 PM, Jim Nasby  wrote:

> On 11/4/16 4:04 AM, Oleksandr Shulgin wrote:
>
>> The psql process even exits with an error code 2, which might be not
>> that expected.  We could stop reading the file and reset connection
>> afterwards, but this is probably not that easy to achieve (think of
>> nested \i calls).
>>
>
> Well, if you stop reading from the file then I don't think more \i's will
> matter, no? I'd certainly like to see improvement here, because the
> difference in behavior with \i is annoying.
>

What I mean is you need a longjump out of the innermost \i back to the
toplevel interactive prompt.  This might be not a problem since this is
what already happens upon receiving SIGINT, I believe.

On the bigger question of how to better protect all these cases (cut &
> paste, etc), I think the only robust way to do that is for psql to track
> intended transaction status itself. With the amount of parsing it's already
> doing, maybe that wouldn't be that hard to add. It looks like this might
> require extra libpq calls to constantly check in on server status; I'm a
> bit surprised that result objects don't include that info.


This doesn't have to be solely about transaction status, though for
something immediately destructive such as DELETE or UPDATE one should
expect a transaction guard.  But for example, pasting something like the
following two lines

SET search_path = 'fancy_new_schema', 'old_boring_schema', public;
SELECT * FROM get_item_ids_to_delete(...);

can give slightly different results depending on whether the first
statement had it effect or not.  And since psql is trying to be very
helpful here by resetting the connection, it makes it all too easy to
overlook the problem.

What do you think about trying to read everything we can from the terminal
using non-blocking IO and only if that gives EWOULDBLOCK, starting the
interactive prompt?

--
Alex

Re: [HACKERS] Copying Permissions

[Stephen Frost]

Corey,

* Corey Huinker (corey.huin...@gmail.com) wrote:
> Craig's post yesterday about exposing syntax for disabling indexes reminded
> me of another feature I think we're lacking in areas where we have to do
> table management.
> 
> The issue is to create a *something* that has the exact permissions of
> another *something*. Usually it's creating a table related to (but not yet
> inheriting) a parent, but it could also be to drop and recreate a
> *something*, making sure it has the same permissions it had before.

Agreed, that seems like a very sensible use-case.

> BEGIN;
> 
> CREATE VIEW dummy AS SELECT 1::text as dummy;
> 
> UPDATE pg_class
> SET relacl = ( SELECT relacl FROM pg_class
>WHERE oid = 'foo'::regclass)
> WHERE oid = 'dummy'::regclass;

Yikes, let's not suggest folks go updating catalog tables, ever, please.
:)

> So it'd be nice to:
> ALTER TABLE bar SET PERMISSIONS FROM foo;
> or maybe even
> GRANT SAME PERMISSIONS ON VIEW bar FROM foo;
> 
> Thoughts?

I like the general idea and suspect many others would also, but there's
two big questions:

Should we have a way for users to define an ACL ala the ALTER DEFAULT
PERMISSIONS approach where the ACL is not attached to any particular
object and is, instead, something which can be assigned to a table.
Further, if we support that, what happens if that is changed later?  I
could see use-cases for both having that change impact all objects and
for not having that happen.

Second, as always, what's the syntax going to actually be?  I don't
think GRANT SAME PERMISSIONS is going to work out too well in the
parser, and it seems a bit grotty to me anyway.  I do think this should
be associated with GRANT rather than ALTER TABLE- GRANT is what we use
for managing privileges on an object.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Fix bug in handling of dropped columns in pltcl triggers

[Tom Lane]

Jim Nasby  writes:
> On 11/4/16 4:28 PM, Tom Lane wrote:
>> It's possible that it'd make sense for pltcl_trigger_handler to ignore
>> empty-string column names in the returned list, so that the behavior
>> with stupid trigger functions would be a bit more forgiving; but that
>> is more or less independent of this patch.

> I'm a bit reluctant to do that since it'd be nice to be consistent with 
> regular pltcl functions returning composites. The same kind of issue 
> exists with the holes in $TG_relatts; we shouldn't be exposing the 
> details of attnum that way. Any code that's expecting those holes is 
> going to blow up after a dump/restore anyway.

Hm.  Offhand it seems like the functions that pltcl itself exposes don't
really do anything that would depend on $TG_relatts indexes matching
physical column numbers.  The only way you could write a pltcl function
that would depend on that would be to have it do some catalog queries that
expect the indexes to match pg_attribute.attnum.  That's possible I guess
but it seems neither likely nor good practice.

I think I'd be in favor of trying to remove the business about having
empty-string entries in $TG_relatts.  Do you want to draft a patch
for 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] Improving RLS planning

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> * Since the planner is now depending on Query.hasRowSecurity to be set
>> whenever there are any securityQuals, I put in an Assert about that,
>> and promptly found three places in prepjointree.c and the rewriter where
>> we'd been failing to set it.  I have not looked to see if these represent
>> live bugs in existing releases, but they might.  Or am I misunderstanding
>> what the flag is supposed to mean?

> They're independent, actually.  securityQuals can be set via either
> security barrier view or from RLS, while hasRowSecurity is specifically
> for the RLS case.  The reason for the distinction is that changing your
> role isn't going to impact security barrier views at all, while it could
> impact what RLS policies are used.  See extract_query_dependencies().

OK.  In that case I'll need to adjust the patch so that the planner keeps
its own flag about whether the query contains any securityQuals; that's
easy enough.  But I'm still suspicious that the three places I found may
represent bugs in the management of Query.hasRowSecurity.

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] Row level security implementation in Foreign Table in Postgres

[Stephen Frost]

Tom, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Wed, Nov 2, 2016 at 10:46 PM, Sounak Chakraborty  
> > wrote:
> >> But my doubt is why this feature is not enabled in case of Foreign Table. 
> >> (ALTER FOREIGN TABLE doesn't have a option of enabling Row Level Security).
> >> Is this is not implemented due to some limitations in the current design?
> >> Because from a quick view it looks like the security subquery can also be 
> >> easily attached to the main query and passed for processing in foreign 
> >> database.
> 
> > Yeah, I don't see why that couldn't be made to work.
> 
> Once the patch at <30304.1478211...@sss.pgh.pa.us> gets in, the major
> issue will be that FDWs will have to be careful not to select quals for
> optimization (ie pushing down to a remote server) unless they satisfy
> restriction_is_securely_promotable().  In most cases that should be
> about a one-line change in the FDW, but I'm not sure that it'd be a good
> idea to just blindly assume that FDWs are doing that.  We could perhaps
> add some sort of "supports RLS" flag to the FDW API, which would not
> get set unless the FDW author takes positive action to do so.

That sounds like an entirely reasonable approach to me.  Other than
that, I agree that FDWs shouldn't be too difficult to add RLS support
for as it seems pretty clear what the semantics there should be.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] C based plugins, clocks, locks, and configuration variables

[Jim Nasby]

On 11/3/16 7:14 PM, Craig Ringer wrote:

1) getting microseconds (or nanoseconds) from UTC epoch in a plugin


GetCurrentIntegerTimestamp()


Since you're serializing generation anyway you might want to just forgo 
the timestamp completely. It's not like the values your generating are 
globally unique anymore, or hard to guess.

--
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
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
Sent 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 SRF and returning composites to pl/tcl

[Tom Lane]

Jim Nasby  writes:
> Hrm, I completely spaced on the fact that composite returns are 
> essentially the same thing as trigger returns. ISTM we should be able to 
> use the same code for both. IIRC those magic elements could end up in 
> any SPI result, so that handling certainly needs to be the same.

> Have you had a chance to look at this or should I?

As things stand in HEAD, the behavior is about the same, but the error
messages are not --- in one case they mention triggers and of course the
other doesn't.  There are a couple of other minor things in the way of
unifying the two hunks of code, so I concluded it probably wasn't worth
the trouble.  But feel free to take another look if it bugs you.

regards, tom lane


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

Re: [HACKERS] Improving RLS planning

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Dean Rasheed  writes:
> > On 25 October 2016 at 22:58, Tom Lane  wrote:
> >> The alternative I'm now thinking about pursuing is to get rid of the
> >> conversion of RLS quals to subqueries.  Instead, we can label individual
> >> qual clauses with security precedence markings.
> 
> > +1 for this approach. It looks like it could potentially be much
> > simpler. There's some ugly code in the inheritance planner (and
> > probably one or two other places) that it might be possible to chop
> > out, which would probably also speed up planning times.
> 
> Here's a draft patch for this.  I've only addressed the RLS use-case
> so far, so this doesn't get into managing the order of application of
> join quals, only restriction quals.

Thanks much for working on this.

Just a few relatively quick comments:

> >> 2. In order_qual_clauses, sort first by security_level and second by cost.
> 
> > I think that ordering might be sub-optimal if you had a mix of
> > leakproof quals and security quals and the cost of some security quals
> > were significantly higher than the cost of some other quals. Perhaps
> > all leakproof quals should be assigned security_level 0, to allow them
> > to be checked earlier if they have a lower cost (whether or not they
> > are security quals), and only leaky quals would have a security_level
> > greater than zero. Rule 1 would then not need to check whether the
> > qual was leakproof, and you probably wouldn't need the separate
> > "leakproof" bool field on RestrictInfo.
> 
> Hm, but it would also force leakproof quals to be evaluated in front
> of potentially-cheaper leaky quals, whether or not that's semantically
> necessary.

I agree that this is a concern.

> I experimented with ignoring security_level altogether for leakproof
> quals, but I couldn't make it work properly, because that didn't lead to
> a comparison rule that satisfies transitivity.  For instance, consider
> three quals:
>   A: cost 1, security_level 1, leaky
>   B: cost 2, security_level 1, leakproof
>   C: cost 3, security_level 0, leakproof
> A should sort before B, since same security_level and lower cost;
> B should sort before C, since lower cost and leakproof;
> but A must sort after C, since higher security_level and leaky.
> 
> So what I ended up doing was using your idea of forcing the security
> level to 0 for leakproof quals, but only if they have cost below a
> threshold (which I set at 10X cpu_operator_cost, which should take in
> most built-in functions).  That at least limits the possible damage
> from forcing early evaluation of a leakproof qual.  There may be
> some better way to do it, though, so I didn't go so far as to remove
> the separate leakproof flag.

I'm not a huge fan of these kinds of magic values, particularly when
they can't be independently managed, but, from a practical perspective,
I have a hard time seeing a real problem with this approach.

> Some other notes:
> 
> * This creates a requirement on scan-planning code (and someday on
> join-planning code) to be sure it doesn't create violations of the qual
> ordering rule.  Currently only indxpath.c and tidpath.c have to worry
> about that AFAICS.  FDWs would need to worry about it too, except that
> we don't currently allow RLS to be enabled on foreign tables.  I'm a
> little concerned about whether FDWs could create security holes by
> not accounting for this, but it's moot for now.  Custom scan providers
> will need to pay attention as well.

I'd certainly like to support RLS on FDWs (and views...), but we need to
hash out what the exact semantics of that will be and this looks like
something we should be able to address when we look at adding that
support.

> * prepsecurity.c is now dead code and should be removed, but I did not
> include that in this patch, since it would just bloat the patch.

Sure.

> * Accounting for the removal of prepsecurity.c, this is actually a net
> savings of about 300 lines of code.  So I feel pretty good about that.
> It also gets rid of some really messy kluges, particularly the behavior
> of generating new subquery RTEs as late as halfway through
> grouping_planner.  I find it astonishing that that worked at all.

I'm a bit surprised to hear that as we've been doing that for quite a
while, though looking back on it, I can see why you bring it up.

> * Since the planner is now depending on Query.hasRowSecurity to be set
> whenever there are any securityQuals, I put in an Assert about that,
> and promptly found three places in prepjointree.c and the rewriter where
> we'd been failing to set it.  I have not looked to see if these represent
> live bugs in existing releases, but they might.  Or am I misunderstanding
> what the flag is supposed to mean?

They're independent, actually.  securityQuals can be set via either
security barrier view or from RLS, while hasRowSecurity is specifically
for the RLS 

Re: [HACKERS] Re: [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …

[Dagfinn Ilmari Mannsåker]

Artur Zakirov  writes:

> Hello,

Hi Artur,

> 2016-09-12 16:16 GMT+03:00 Dagfinn Ilmari Mannsåker :
>>
>> I've added it to the 2016-11 commit fest:
>> https://commitfest.postgresql.org/11/795/
>>
>> - ilmari
>
> I've tested your patch.
[...]
> It seems that the patch can be commited without any fixes. So I marked
> it as "Ready for Committer".

Thank you very much.  I just looked at the patch again and realised the
completion of "TO" after RENAME VALUE  can be merged with the one
for RENAME ATTRIBUTE .  Attached is an updated and patch, which
only differs from the previous one as follows:

$ interdiff 0001-Add-psql-tab-completion-for-ALTER-TYPE-RENAME-VALUE{,-v2}.patch
diff -u b/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1918,11 +1918,8 @@ psql_completion(const char *text, int start, int end)
/* ALTER TYPE  RENAME  */
else if (Matches4("ALTER", "TYPE", MatchAny, "RENAME"))
COMPLETE_WITH_LIST3("ATTRIBUTE", "TO", "VALUE");
-   /* ALTER TYPE xxx RENAME ATTRIBUTE yyy */
-   else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", "ATTRIBUTE", 
MatchAny))
-   COMPLETE_WITH_CONST("TO");
-   /* ALTER TYPE xxx RENAME VALUE yyy */
-   else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", "VALUE", MatchAny))
+   /* ALTER TYPE xxx RENAME (ATTRIBUTE|VALUE) yyy */
+   else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", "ATTRIBUTE|VALUE", 
MatchAny))
COMPLETE_WITH_CONST("TO");
/*
 * If we have ALTER TYPE  ALTER/DROP/RENAME ATTRIBUTE, provide list

>From d8454b66a544d91bf779ca3bfb38d18e4cd504f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Mon, 12 Sep 2016 12:17:37 +0100
Subject: [PATCH] =?UTF-8?q?Add=20psql=20tab=20completion=20for=20ALTER=20T?=
 =?UTF-8?q?YPE=20=E2=80=A6=20RENAME=20VALUE?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Modelled on the completion for attributes, tweaked to return string
literals intead of identifiers.
---
 src/bin/psql/tab-complete.c | 58 +
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a43bbc5..b556c00 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -202,6 +202,31 @@ do { \
 	matches = completion_matches(text, complete_from_query); \
 } while (0)
 
+#define COMPLETE_WITH_ENUM_VALUE(type) \
+do { \
+	char   *_completion_schema; \
+	char   *_completion_type; \
+\
+	_completion_schema = strtokx(type, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+	(void) strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+   false, false, pset.encoding); \
+	_completion_type = strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+			   false, false, pset.encoding);  \
+	if (_completion_type == NULL)\
+	{ \
+		completion_charp = Query_for_list_of_enum_values; \
+		completion_info_charp = type; \
+	} \
+	else \
+	{ \
+		completion_charp = Query_for_list_of_enum_values_with_schema; \
+		completion_info_charp = _completion_type; \
+		completion_info_charp2 = _completion_schema; \
+	} \
+	matches = completion_matches(text, complete_from_query); \
+} while (0)
+
 #define COMPLETE_WITH_FUNCTION_ARG(function) \
 do { \
 	char   *_completion_schema; \
@@ -598,6 +623,26 @@ static const SchemaQuery Query_for_list_of_matviews = {
 "   AND (pg_catalog.quote_ident(nspname)='%s' "\
 "OR '\"' || nspname || '\"' ='%s') "
 
+#define Query_for_list_of_enum_values \
+"SELECT pg_catalog.quote_literal(enumlabel) "\
+"  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t "\
+" WHERE t.oid = e.enumtypid "\
+"   AND substring(pg_catalog.quote_literal(enumlabel),1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(typname)='%s' "\
+"OR '\"' || typname || '\"'='%s') "\
+"   AND pg_catalog.pg_type_is_visible(t.oid)"
+
+#define Query_for_list_of_enum_values_with_schema \
+"SELECT pg_catalog.quote_literal(enumlabel) "\
+"  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t, pg_catalog.pg_namespace n "\
+" WHERE t.oid = e.enumtypid "\
+"   AND n.oid = t.typnamespace "\
+"   AND substring(pg_catalog.quote_literal(enumlabel),1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(typname)='%s' "\
+"OR '\"' || typname || '\"'='%s') "\
+"   AND (pg_catalog.quote_ident(nspname)='%s' "\
+"OR '\"' || nspname || '\"' ='%s') "
+
 #define Query_for_list_of_template_databases \
 "SELECT pg_catalog.quote_ident(d.datname) "\
 "  FROM pg_catalog.pg_database d "\
@@ -1872,11 +1917,10 @@ psql_completion(const char *text, int start, int end)
 		COMPLETE_WITH_LIST2("ATTRIBUTE", "VALUE");
 	/* ALTER TYPE  RENAME	*/
 	else if (Matches4("ALTER", "TYPE", MatchAny, "RENAME"))
-		COMPLETE_WITH_LIST2("ATTRIBUTE", "TO");
-	/* ALTER TYPE xxx RENAME ATTRIBUTE yyy */
-	else if (Matches6("ALTER", "TYPE", MatchAny, "RENAME", 

[HACKERS] Typo in event_trigger.c

[Michael Paquier]

Hi all,

I just bumped into the following:
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -742,7 +742,7 @@ EventTriggerCommonSetup(Node *parsetree,

/*
 * Filter list of event triggers by command tag, and copy them into our
-* memory context.  Once we start running the command trigers, or indeed
+* memory context.  Once we start running the command triggers, or indeed
 * once we do anything at all that touches the catalogs, an invalidation
 * might leave cachelist pointing at garbage, so we must do this before we
 * can do much else.

Thanks to David Steele for pointing out a similar typo in one of my patches :p
-- 
Michael
diff --git a/src/backend/commands/event_trigger.c 
b/src/backend/commands/event_trigger.c
index ac4c4ec..e87fce7 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -742,7 +742,7 @@ EventTriggerCommonSetup(Node *parsetree,
 
/*
 * Filter list of event triggers by command tag, and copy them into our
-* memory context.  Once we start running the command trigers, or indeed
+* memory context.  Once we start running the command triggers, or 
indeed
 * once we do anything at all that touches the catalogs, an invalidation
 * might leave cachelist pointing at garbage, so we must do this before 
we
 * can do much else.

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

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Michael Paquier]

On Tue, Nov 8, 2016 at 9:32 PM, David Steele  wrote:
> I had a bit of trouble parsing this paragraph:
>
> [...]
>
> So I did a little reworking:
>
> [...]
>
> If that still says what you think it should, then I believe it is clearer.

Thanks! I have included your suggestion.

> Also:
>
> +* last time a segment has switched because of a timeout.
> Segment
> +* switching because of other reasons, like manual
> trigerring of
>
> typo, should be "triggering".

Right.

> I don't see any further issues with this patch unless there are performance
> concerns about the locks taken in GetProgressRecPtr().  The locks seem
> reasonable to me but I'd like to see this committed so there's plenty of
> time to detect any regression before 10.0.
>
> As such, my vote is to mark this "Ready for Committer."  I'm fine with
> waiting a few days as Kyotaro suggested, or we can consider my review
> "additional comments" and do it now.

Thanks for the review! Waiting for a couple of days more is fine for
me. This won't change much. Attached is v15 with the fixes you
mentioned.
-- 
Michael
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index adab2f8..38c2385 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2826,12 +2826,9 @@ include_dir 'conf.d'
 parameter is greater than zero, the server will switch to a new
 segment file whenever this many seconds have elapsed since the last
 segment file switch, and there has been any database activity,
-including a single checkpoint.  (Increasing
-checkpoint_timeout will reduce unnecessary
-checkpoints on an idle system.)
-Note that archived files that are closed early
-due to a forced switch are still the same length as completely full
-files.  Therefore, it is unwise to use a very short
+including a single checkpoint.  Note that archived files that are
+closed early due to a forced switch are still the same length as
+completely full files.  Therefore, it is unwise to use a very short
 archive_timeout  it will bloat your archive
 storage.  archive_timeout settings of a minute or so are
 usually reasonable.  You should consider using streaming replication,
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index b019bc1..ac40731 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2507,7 +2507,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId 
cid,
heaptup->t_len - 
SizeofHeapTupleHeader);
 
/* filtering by origin on a row level is much more efficient */
-   XLogIncludeOrigin();
+   XLogSetFlags(XLOG_INCLUDE_ORIGIN);
 
recptr = XLogInsert(RM_HEAP_ID, info);
 
@@ -2846,7 +2846,7 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, 
int ntuples,
XLogRegisterBufData(0, tupledata, totaldatalen);
 
/* filtering by origin on a row level is much more 
efficient */
-   XLogIncludeOrigin();
+   XLogSetFlags(XLOG_INCLUDE_ORIGIN);
 
recptr = XLogInsert(RM_HEAP2_ID, info);
 
@@ -3308,7 +3308,7 @@ l1:
}
 
/* filtering by origin on a row level is much more efficient */
-   XLogIncludeOrigin();
+   XLogSetFlags(XLOG_INCLUDE_ORIGIN);
 
recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_DELETE);
 
@@ -6035,7 +6035,7 @@ heap_finish_speculative(Relation relation, HeapTuple 
tuple)
XLogBeginInsert();
 
/* We want the same filtering on this as on a plain insert */
-   XLogIncludeOrigin();
+   XLogSetFlags(XLOG_INCLUDE_ORIGIN);
 
XLogRegisterData((char *) , SizeOfHeapConfirm);
XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
@@ -7703,7 +7703,7 @@ log_heap_update(Relation reln, Buffer oldbuf,
}
 
/* filtering by origin on a row level is much more efficient */
-   XLogIncludeOrigin();
+   XLogSetFlags(XLOG_INCLUDE_ORIGIN);
 
recptr = XLogInsert(RM_HEAP_ID, info);
 
diff --git a/src/backend/access/transam/xact.c 
b/src/backend/access/transam/xact.c
index e11b229..9130816 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5232,7 +5232,7 @@ XactLogCommitRecord(TimestampTz commit_time,
XLogRegisterData((char *) (_origin), sizeof(xl_xact_origin));
 
/* we allow filtering by xacts */
-   XLogIncludeOrigin();
+   XLogSetFlags(XLOG_INCLUDE_ORIGIN);
 
return XLogInsert(RM_XACT_ID, info);
 }
diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index 6cec027..37ecf9c 100644
--- a/src/backend/access/transam/xlog.c
+++ 

Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

[Michael Paquier]

On Tue, Nov 8, 2016 at 9:22 PM, Michael Banck  wrote:
> Thanks! I couldn't find furhter faults in my testing. I guess the
> question what to do about this on Windows is possibly still open, but as
> I am not familiar with the Windows port at all I've marked it Ready for
> Committer for now.

The conclusion on Windows per upthread is "do nothing", because there
is no way to get a good balance between usability and security. To be
honest, I am fine with that.
-- 
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] Quorum commit for multiple synchronous replication.

[Michael Paquier]

On Tue, Nov 8, 2016 at 12:25 AM, Masahiko Sawada  wrote:
> On Tue, Oct 25, 2016 at 10:35 PM, Michael Paquier
>  wrote:
>> +   if (SyncRepConfig->sync_method == SYNC_REP_PRIORITY)
>> +   return SyncRepGetSyncStandbysPriority(am_sync);
>> +   else /* SYNC_REP_QUORUM */
>> +   return SyncRepGetSyncStandbysQuorum(am_sync);
>> Both routines share the same logic to detect if a WAL sender can be
>> selected as a candidate for sync evaluation or not, still per the
>> selection they do I agree that it is better to keep them as separate.
>>
>> +   /* In quroum method, all sync standby priorities are always 1 */
>> +   if (found && SyncRepConfig->sync_method == SYNC_REP_QUORUM)
>> +   priority = 1;
>> Honestly I don't understand why you are enforcing that. Priority can
>> be important for users willing to switch from ANY to FIRST to have a
>> look immediately at what are the standbys that would become sync or
>> potential.
>
> I thought that since all standbys appearing in s_s_names list are
> treated equally in quorum method, these standbys should have same
> priority.
> If these standby have different sync_priority, it looks like that
> master server replicates to standby server based on priority.

No actually, because we know that they are a quorum set, and that they
work in the same set. The concept of priorities has no real meaning
for quorum as there is no ordering of the elements. Another, perhaps
cleaner idea may be to mark the field as NULL actually.

>> It is not possible to guess from how many standbys this needs to wait
>> for. One idea would be to mark the sync_state not as "quorum", but
>> "quorum-N", or just add a new column to indicate how many in the set
>> need to give a commit confirmation.
>
> As Simon suggested before, we could support another feature that
> allows the client to control the quorum number.
> Considering adding that feature, I thought it's better to have and
> control that information as a GUC parameter.
> Thought?

Similarly that would be a SIGHUP parameter? Why not. Perhaps my worry
is not that much legitimate, users could just look at s_s_names to
guess how many in hte set a commit needs to wait for.

+
+FIRST and ANY are case-insensitive word
+and the standby name having these words are must be double-quoted.
+
s/word/words/.

+FIRST and ANY specify the method of
+that how master server controls the standby servers.
A little bit hard to understand, I would suggest:
FIRST and ANY specify the method used by the master to control the
standby servers.

+The keyword FIRST, coupled with an integer
+number N higher-priority standbys and makes transaction commit
+when their WAL records are received.
This is unclear to me. Here is a correction:
The keyword FIRST, coupled with an integer N, makes transaction commit
wait until WAL records are received fron the N standbys with higher
priority number.

+synchronous_standby_names. For example, a setting
+of ANY 3 (s1, s2, s3, s4) makes transaction commits
+wait until receiving receipts from at least any three standbys
+of four listed servers s1, s2, s3,
This could just mention WAL records instead of "receipts".

Instead of saying "an integer number N", we could use num_sync.

+ If FIRST or ANY are not specified,
this parameter
+ behaves as ANY. Note that this grammar is
incompatible with
+ PostgresSQL 9.6, where no keyword specified
is equivalent
+ as if FIRST was specified. In short, there is no
real need to
+ specify num_sync as this
+ behavior does not have changed, as well as it is not
necessary to mention
+ pre-9.6 versions are the multi-sync grammar has been added in 9.6.
This paragraph could be reworked, say:
if FIRST or ANY are not specified this parameter behaves as if ANY is
used. Note that this grammar is incompatible with PostgreSQL 9.6 which
is the first version supporting multiple standbys with synchronous
replication, where no such keyword FIRST or ANY can be used. Note that
the grammar behaves as if FIRST is used, which is incompatible with
the post-9.6 behavior.

+ Synchronous state of this standby server. quorum-N
+ , where N is the number of synchronous standbys that transactions
+ need to wait for replies from, when standby is considered as a
+ candidate of quorum commit.
Nitpicking: I think that the comma goes to the previous line if it is
the first character of a line.

+   if (SyncRepConfig->sync_method == SYNC_REP_PRIORITY)
+   return SyncRepGetSyncStandbysPriority(am_sync);
+   else /* SYNC_REP_QUORUM */
+   return SyncRepGetSyncStandbysQuorum(am_sync)
Or that?
if (PRIORITY)
return StandbysPriority();
else if (QUORUM)
return StandbysQuorum();
else
elog(ERROR, "Boom");

+ * In priority method, we need the oldest these positions among sync
+ * standbys. In quorum 

Re: [HACKERS] Radix tree for character conversion

[Daniel Gustafsson]

> On 08 Nov 2016, at 12:21, Kyotaro HORIGUCHI  
> wrote:
> 
> Hello, this is the revising patch applies on top of the previous
> patch.
> 
> ...
> 
> Finally the attached patch contains most of (virtually all of)
> Daniel's suggestion and some modification by pgperltidy.

Reading over this it looks good to me.  I did spot one thing I had missed
before though, the error message below should be referencing the scalar
variable ‘direction' unless I’m missing something:

-   die "unacceptable direction : %direction"
+   die "unacceptable direction : $direction"
  if ($direction ne "to_unicode" && $direction ne "from_unicode");

With this, I would consider this ready for committer.

>> Addition to this, I'll remove existing authority files and modify
>> radix generator so that it can read plain map files in the next
>> patch.
> 
> So, I think the attached are in rather modern shape.

+1, nice work!

cheers ./daniel

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

Re: [HACKERS] Fwd: Re: [CORE] temporal tables (SQL2011)

[Robert Haas]

On Sun, Nov 6, 2016 at 4:08 PM, Stefan Scheid  wrote:

> are there plans to introduce temporal tables?
>
I don't know of anyone who is actually working on it, but I agree that it
would probably attract some users if we did.

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

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[David Steele]

On 10/5/16 7:18 AM, Michael Paquier wrote:


Note: I am moving this patch to next CF.


And I am back on it more seriously... And I am taking back what I said upthread.

I looked at the v12 that Horiguchi-san has written, and that seems
correct to me. So I have squashed everything into a single patch,
including the log entry that gets logged with log_checkpoints. Testing
with archive_timeout to 10s, checkpoint_timeout to 30s, sometimes
triggering manual activity with CREATE TABLE/whatever and manual
pg_switch_xlog(), I am able to see that checkpoints can be correctly
skipped or generated.

There was as well a compilation error with ereport(). Not sure how I
missed that... Likely too many patches handled these days.

I have also updated the description of archive_timeout that increasing
checkpoint_timeout would reduce unnecessary checkpoints on a idle
system. With this patch, on an idle system checkpoints are just
skipped as they should.

How does that look?


This looks much better now and exhibits exactly the behavior that I 
expect.


In theory it would be nice if the checkpoint records did not cause 
rotation, but this can be mitigated in the way you have described and 
perhaps for safety's sake it's best.


I had a bit of trouble parsing this paragraph:

+   /*
+* Update the progress LSN positions. At least one WAL insertion lock
+* is already taken appropriately before doing that, and it is just more
+* simple to do that here where WAL record data and type is at hand.
+* The progress is set at the start position of the record tracked that
+* is being added, making easier checkpoint progress tracking as the
+* control file already saves the start LSN position of the last
+* checkpoint run. If an exclusive lock is taken for WAL insertion,
+* there is actually no need to update all the progression fields, so

So I did a little reworking:

Update the LSN progress positions. At least one WAL insertion lock is 
already taken appropriately before doing that, and it is simpler to do 
that here when the WAL record data and type are at hand. Progress is set 
at the start position of the tracked record that is being added, making 
checkpoint progress tracking easier as the control file already saves 
the start LSN position of the last checkpoint. If an exclusive lock is 
taken for WAL insertion there is no need to update all the progress 
fields, only the first one.


If that still says what you think it should, then I believe it is 
clearer.  Also:


+* last time a segment has switched because of a timeout. 
Segment
+* switching because of other reasons, like manual trigerring of

typo, should be "triggering".

I don't see any further issues with this patch unless there are 
performance concerns about the locks taken in GetProgressRecPtr().  The 
locks seem reasonable to me but I'd like to see this committed so 
there's plenty of time to detect any regression before 10.0.


As such, my vote is to mark this "Ready for Committer."  I'm fine with 
waiting a few days as Kyotaro suggested, or we can consider my review 
"additional comments" and do it now.


--
-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] Reload SSL certificates on SIGHUP

[Michael Banck]

Hi,

On Mon, Nov 07, 2016 at 02:36:00AM +0100, Andreas Karlsson wrote:
> Thanks for the review! I have fixed all your feedback in the attached
> patch.

Thanks! I couldn't find furhter faults in my testing. I guess the
question what to do about this on Windows is possibly still open, but as
I am not familiar with the Windows port at all I've marked it Ready for
Committer for now.


Thanks,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
Sent 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: Change the way that LWLocks for extensions are allocated.

[Amit Kapila]

On Tue, Nov 8, 2016 at 4:23 PM, Ashutosh Sharma  wrote:
> Hi,
>
>> I had also thought about it while preparing patch, but I couldn't find
>> any clear use case.  I think it could be useful during development of
>> a module, but not sure if it is worth to add a target.  I think there
>> is no harm in adding such a target, but lets take an opinion from
>> committer or others as well before adding this target.  What do you
>> say?
>
> Ok. We can do that.
>
> I have verified the updated v2 patch. The patch looks good to me. I am
> marking it  as ready for committer.

Thanks for the review.



-- 
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] PassDownLimitBound for ForeignScan/CustomScan [take-2]

[Jeevan Chalke]

Hi,

I have reviewed the patch.

Patch applies cleanly. make/"make install"/"make check" all are fine.

I too see a good performance in execution time when LIMIT is pushed with
cursor query in postgres_fdw at execution time

However here are few comments on the changes:

1. ps_numTuples is declared as long, however offset and count members in
LimitState struct and bound member in SortState struct is int64.  However
long on 32 bit machine may be 32 bits and thus I think tuples_needed which
is long may have overflow hazards as it may store int64 + int64.  I think
ps_numTuples should be int64.

2. Robert suggested following in the previous discussion:
"For example, suppose we add a new PlanState member "long
numTuples" where 0 means that the number of tuples that will be needed
is unknown (so that most node types need not initialize it), a
positive value is an upper bound on the number of tuples that will be
fetched, and -1 means that it is known for certain that we will need
all of the tuples."

We should have 0 for the default case so that we don't need to initialize it
at most of the places.  But I see many such changes in the patch.  I think
this is not possible here since 0 can be a legal user provided value which
cannot be set as a default (default is all rows).

However do you think, can we avoid that? Is there any other way so that we
don't need every node having ps_numTuples to be set explicitly?

Apart from this patch look good to me.


Regards,
-- 
Jeevan Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Kyotaro HORIGUCHI]

Hello,

At Tue, 8 Nov 2016 14:45:59 +0900, Michael Paquier  
wrote in 
> On Tue, Nov 1, 2016 at 8:31 PM, Kyotaro HORIGUCHI
>  wrote:
> > Could you let me struggle a bit more to avoid LWLocks in
> > GetProgressRecPtr?
> 
> Be my guest :)
> 
> > I considered two alternatives for updating logic of progressAt
> > more seriously. One is, as Amit suggested, replacing progressAt
> > within the SpinLock section in
> > ReserverXLogInsertLocation. Another is using pg_atomic_u64 for
> > progressAt. The attached two patches rouhgly implement the aboves
> > respectively. (But I've not tested them. The patches are to show
> > the two alternatives concretely.)
> 
> Okay.
> 
> > I found that the former requires to take insertpos_lck also on
> > reading. I have to admit that this is too bad. (Even I saw no
> > degradation by pgbench on my poor environment. It marks 118tr/s
> > by 10 threads and that doesn't seem make any stress on xlog
> > logics...)
> 
> Interesting...
> 
> > For the latter, it is free from locks and doesn't reduce parallel
> > degree but I'm not sure it is proper to use it there and I'm not
> > sure about actual overheads.  In the worst case, it makes another
> > SpinLock section for every call to pg_atmoic_* functions.
> 
> The WAL insert slots have been introduced in 9.4, and the PG atomics
> in 9.5, so perhaps the first implementation of the WAL insert slots
> would have used it. Still that looks quite promising. At the same time
> we may be able to do something for insertingAt to make the locks held
> more consistent, and just remove WALInsertLocks, even if this makes me
> wonder about torn reads and about how we may break things if we rely

If I understand you correctly, atomics prevents torn reads by its
definition on cache management and bus arbitration level. It
should be faster than LWlocks but as I said in the previous mail,
on some platforms, if any, it will fallbacks to individual
spinlocks. (atomics.c)

> on something else than LW_EXCLUSIVE compared to now. To keep things
> more simple I' would still favor using WALInsertLocks for this patch,
> that looks more consistent, and also because there is no noticeable
> difference.

Ok, the patch looks fine. So there's nothing for me but to accept
the current shape since the discussion about performance seems
not to be settled with out performance measurement with machines
with many cores.

> > All except the above point looks good for me. Maybe it is better
> > that XLOG_INCLUDE_ORIGIN stuff is in a separate patch.
> 
> I have kept that grouped bas XLOG_INCLUDE_ORIGIN is the only
> XLogInsert flag present on HEAD. Could the patch be marked as "ready
> for committer" then?

Ok, that is not so siginificant point. Well, I'd like to wait for
a couple of days for anyone wants to comment, then mark this
'ready for committer'.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[Magnus Hagander]

On Tue, Nov 8, 2016 at 1:28 PM, David Steele  wrote:

> Hi Magnus,
>
> On 11/7/16 2:07 PM, Magnus Hagander wrote:
>
>> On Sat, Oct 29, 2016 at 4:12 PM, Michael Paquier
>> Indeed, giving the attached for REL9_6_STABLE. You could as well have
>> a test for pg_stat_tmp but honestly that's not worth it. One thing I
>> have noticed is that the patch does not use _tarWriteDir() for
>> pg_xlog. I think it should even if that's not addressing directly a
>> bug...
>>
>> Applied and backported, thanks. Backported to 9.4, as this is where that
>> exclusion code appeared.
>>
>
> I reviewed the three back-patches and they look sensible to me.


Thanks!



> I did not backport the tests, as we don't have the $node stuff available
>> in 9.5 and earlier.
>>
>
> That's unfortunate but the changes are pretty straightforward and the
> testing is as good as it was before...


Yeah, that's my thinking as well. It would be nice, but not worth the
effort.

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

Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[David Steele]

Hi Magnus,

On 11/7/16 2:07 PM, Magnus Hagander wrote:

On Sat, Oct 29, 2016 at 4:12 PM, Michael Paquier
Indeed, giving the attached for REL9_6_STABLE. You could as well have
a test for pg_stat_tmp but honestly that's not worth it. One thing I
have noticed is that the patch does not use _tarWriteDir() for
pg_xlog. I think it should even if that's not addressing directly a
bug...

Applied and backported, thanks. Backported to 9.4, as this is where that
exclusion code appeared.


I reviewed the three back-patches and they look sensible to me.


I did not backport the tests, as we don't have the $node stuff available
in 9.5 and earlier.


That's unfortunate but the changes are pretty straightforward and the 
testing is as good as it was before...


--
-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] Radix tree for character conversion

[Kyotaro HORIGUCHI]

Hello, this is the revising patch applies on top of the previous
patch.

Differences on map files are enormous but useless for discussion
so they aren't included in this. (but can be generated)

This still doesn't remove three .txt/.xml files since it heavily
bloats the patch. I'm planning that they are removed in the final
shape. All authority files including the removed files are
automatically downloaded by the Makefile in this patch.

At Tue, 08 Nov 2016 10:43:56 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20161108.104356.265607041.horiguchi.kyot...@lab.ntt.co.jp>
> https://www.postgresql.org/docs/devel/static/install-requirements.html
> 
> | Perl 5.8 or later is needed to build from a Git checkout, or if
> | you changed the input files for any of the build steps that use
> | Perl scripts. If building on Windows you will need Perl in any
> | case. Perl is also required to run some test suites.
> 
> So, we should assume Perl 5.8 (released in 2002!) on build
> time. And actually 5.10 on RedHat 6.4, 5.16 on my
> environment(ContOS 7.2), and the official doc is at 5.24. Active
> perl is 5.24. According to this, we should use syntax supported
> as of 5.8 and/but not obsolete until 5.24, then to follow the
> latest convention. But not OO. (But I can't squeeze out a
> concrete syntax set out of this requirements :( )
...(forget this for a while..)

Finally the attached patch contains most of (virtually all of)
Daniel's suggestion and some modification by pgperltidy.

> Addition to this, I'll remove existing authority files and modify
> radix generator so that it can read plain map files in the next
> patch.

So, I think the attached are in rather modern shape.

At Tue, 08 Nov 2016 11:02:58 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20161108.110258.59832499.horiguchi.kyot...@lab.ntt.co.jp>
> Hmm.  Somehow perl-mode on my Emacs is stirring with
> ununderstandable indentation and I manually correct them so it is
> highly probable that the style of this patch is not compatible
> with the defined style. Anyway it is better that pgindent
> generates smaller patch so I'll try it.

The attached are applied pgperltidy. Several regions such like
additional character list are marked not to be edited.

One concern is what to leave by 'make distclen' and 'make
maintainer-clean'. The former should remove authority *.TXT files
since it shouldn't be in source archive. On the other hand it is
more convenient that the latter leaves them. This seems somewhat
strange but I can't come up with better behavior for now.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 02a19deb3eb74069936ced0dbea4693322ad9ec8 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Tue, 8 Nov 2016 18:22:36 +0900
Subject: [PATCH 1/2] Edit perl scripts into modern style

Part of existing and added scripts are in obsolete style. This path
edits them into modern style as the suggestion by Daniel Gustafsson.
---
 src/backend/utils/mb/Unicode/UCS_to_BIG5.pl|  31 +-
 src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl  |  33 +-
 .../utils/mb/Unicode/UCS_to_EUC_JIS_2004.pl|  66 ++-
 src/backend/utils/mb/Unicode/UCS_to_EUC_JP.pl  |  36 +-
 src/backend/utils/mb/Unicode/UCS_to_EUC_KR.pl  |   9 +-
 src/backend/utils/mb/Unicode/UCS_to_EUC_TW.pl  |  20 +-
 src/backend/utils/mb/Unicode/UCS_to_GB18030.pl |  29 +-
 src/backend/utils/mb/Unicode/UCS_to_JOHAB.pl   |   7 +-
 .../utils/mb/Unicode/UCS_to_SHIFT_JIS_2004.pl  |  92 ++--
 src/backend/utils/mb/Unicode/UCS_to_SJIS.pl|  27 +-
 src/backend/utils/mb/Unicode/UCS_to_UHC.pl |  35 +-
 src/backend/utils/mb/Unicode/UCS_to_most.pl|  14 +-
 src/backend/utils/mb/Unicode/convutils.pm  | 579 +++--
 src/backend/utils/mb/Unicode/make_mapchecker.pl|  18 +-
 14 files changed, 529 insertions(+), 467 deletions(-)

diff --git a/src/backend/utils/mb/Unicode/UCS_to_BIG5.pl b/src/backend/utils/mb/Unicode/UCS_to_BIG5.pl
index 7b14817..0412723 100755
--- a/src/backend/utils/mb/Unicode/UCS_to_BIG5.pl
+++ b/src/backend/utils/mb/Unicode/UCS_to_BIG5.pl
@@ -24,10 +24,10 @@
 #		 UCS-2 code in hex
 #		 # and Unicode name (not used in this script)
 
-
+use strict;
 require "convutils.pm";
 
-$this_script = $0;
+my $this_script = $0;
 
 # Load BIG5.TXT
 my $all = _source("BIG5.TXT");
@@ -35,9 +35,10 @@ my $all = _source("BIG5.TXT");
 # Load CP950.TXT
 my $cp950txt = _source("CP950.TXT");
 
-foreach my $i (@$cp950txt) {
+foreach my $i (@$cp950txt)
+{
 	my $code = $i->{code};
-	my $ucs = $i->{ucs};
+	my $ucs  = $i->{ucs};
 
 	# Pick only the ETEN extended characters in the range 0xf9d6 - 0xf9dc
 	# from CP950.TXT
@@ -46,20 +47,22 @@ foreach my $i (@$cp950txt) {
 		&& $code >= 0xf9d6
 		&& $code <= 0xf9dc)
 	{
-		push @$all, {code => $code,
-	 ucs => $ucs,
-	 comment => $i->{comment},
-	 direction 

Re: [HACKERS] Improving executor performance

[Andres Freund]

Hi,

On 2016-11-04 15:25:58 -0700, Doug Doole wrote:
> On 07/13/2016 06:18 PM, Andres Freund wrote:
> > Attached (in patch 0003) is a proof-of-concept implementing an
> > expression evalution framework that doesn't use recursion. Instead
> > ExecInitExpr2 computes a number of 'steps' necessary to compute an
> > expression. These steps are stored in a linear array, and executed one
> > after another (save boolean expressions, which can jump to later steps).
> > E.g. to compute colname = 1 the steps are 1) fetch var, 2) evaluate
> > const, 3) call function.
>
> We've been having trouble with the performance of simple expressions in
> PLpgSQL so I started playing with this patch. (No sense re-inventing the
> wheel after all.) It was straightforward to extend to simple expressions and
> showed an immediate improvement (~10% faster on a simple test).

That's cool.


> Running in our full environment highlighted a few areas that I think
> are worth more investigation.

There indeed should be many of those.


> However, before I tackle that, is the posted proof-of-concept still the
> latest and greatest? If not, any chance of getting the latest?

It's definitely *not* the latest and greatest. My latest version
actually passes all regression tests, and has a few additional
expression types handled natively (NULL tests, CASE IIRC), and more than
few bugs fixed.

I'm away at the moment, but I plan to post a new version end of this or
beginning of next week.


> Going forward, I'd like to collaborate on our efforts if you're interested.

That sounds good!

Regards,

Andres


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Change the way that LWLocks for extensions are allocated.

[Ashutosh Sharma]

Hi,

> I had also thought about it while preparing patch, but I couldn't find
> any clear use case.  I think it could be useful during development of
> a module, but not sure if it is worth to add a target.  I think there
> is no harm in adding such a target, but lets take an opinion from
> committer or others as well before adding this target.  What do you
> say?

Ok. We can do that.

I have verified the updated v2 patch. The patch looks good to me. I am
marking it  as ready for committer. Thanks.

With Regards,
Ashutosh Sharma
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] pgbench - allow backslash continuations in \set expressions

[Fabien COELHO]

Hello Rafia,

Please keep copying to the list.


Though I find the first version of this patch acceptable in terms that it
would be helpful on enhancing the readability of expressions as you
mentioned. However, the second version is not clear as I mentioned before
also, there has to be detailed documentation regarding this, still as an
end-user one needs to be too careful to decide which statements to split
lines based on the patch, for example following
\set bid
CASE WHEN random(0, 99) < 85
  THEN :tbid
  ELSE :abid + (:abid >= :tbid)
END

should be written as

\set bid CASE WHEN random(0, 99) < 85
  THEN :tbid
  ELSE :abid + (:abid >= :tbid)
END


I do not understand the "should". It is a matter of style, and both cases 
would work with the second version of the patch.


(and a few more variants of splitting lines just after an operator or 
open parenthesis) which does not look like much enhancement in 
readability to me.


As I said, I will not fight over this one. I like inferred continuations 
in scala, but I guess it would be surprising for such an old school script 
like pgbench, and it would be hard to do that consistently for pgsql 
because the backslash command syntax may depends on the command itself (eg 
does it have to respect strings and parenthesis, or some other 
structures...).


So this patch needs to add the support to read the next line even when 
"\set a" and other such similar expressions are encountered.


The scala convention is that if the line is not finished the user must 
either use explicit parenthesis are use an unambiguous operator which 
expects an operand, so that the lexer knows it has to go on.


I do not think that having both inferred continuations and explicit 
backslash continuations is desirable, it should be one or the other.


--
Fabien.


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

Re: [HACKERS] Specifying the log file name of pgbench -l option

[Fabien COELHO]

[ ... v4 ]


I checked. It works as expected. I have no more comments.
If its okay with Fabien, we can mark it ready for committer.


Patch applies, compiles (including the documentation), make check ok and 
features works for me. Code could be a little simpler, but it is okay. 
I've switched the status to "ready for committers".


--
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] A bug in UCS_to_most.pl

[Kyotaro HORIGUCHI]

Hello,

I was surprised to find that
src/backend/utils/mb/Unicode/Makefile got changed (3a47c70) to
have download feature of character conversion authority files. It
seems fine according to the previous discussion as the commnet of
that is saying.

So, I did 'make maintainer-clean; make' on it but got the
following error.

> Hash %filename missing the % in argument 1 of keys() at UCS_to_most.pl line 
> 51.'/usr/bin/perl' UCS_to_most.pl

What is more surprising to find that it is there since
2006. Maybe Heikki unintentionally fixed that in the patch in the
thread, or older perls might not have complained about it (my
perl is 5.16).

Anyway, the attached small patch fixes it.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/mb/Unicode/UCS_to_most.pl b/src/backend/utils/mb/Unicode/UCS_to_most.pl
index d7ec8ef..125378f 100755
--- a/src/backend/utils/mb/Unicode/UCS_to_most.pl
+++ b/src/backend/utils/mb/Unicode/UCS_to_most.pl
@@ -48,7 +48,7 @@ require "ucs2utf.pl";
 	'UHC'=> 'CP949.TXT',
 	'JOHAB'  => 'JOHAB.TXT',);
 
-@charsets = keys(filename);
+@charsets = keys(%filename);
 @charsets = @ARGV if scalar(@ARGV);
 foreach $charset (@charsets)
 {

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