Re: [HACKERS] GSoC 2017 : Patch for predicate locking in Gist index

[Alexander Korotkov]

Hi, Shubham!

On Wed, Nov 1, 2017 at 12:10 AM, Shubham Barai 
wrote:

> On 9 October 2017 at 18:57, Alexander Korotkov 
> wrote:
>
>> Now, ITSM that predicate locks and conflict checks are placed right for
>> now.
>> However, it would be good to add couple comments to gistdoinsert() whose
>> would state why do we call CheckForSerializableConflictIn() in these
>> particular places.
>>
>> I also take a look at isolation tests.  You made two separate test specs:
>> one to verify that serialization failures do fire, and another to check
>> there are no false positives.
>> I wonder if we could merge this two test specs into one, but use more
>> variety of statements with different keys for both inserts and selects.
>>
>
> Please find the updated version of patch here. I have made suggested
> changes.
>

In general, patch looks good for me now.  I just see some cosmetic issues.

  /*
> + *Check for any r-w conflicts (in serialisation isolation level)
> + *just before we intend to modify the page
> + */
> + CheckForSerializableConflictIn(r, NULL, stack->buffer);
> + /*


Formatting doesn't look good here.  You've missed space after star sign in
the comment.  You also missed newline after
CheckForSerializableConflictIn() call.

Also, you've long comment lines in predicate-gist.spec.  Please, break long
comments into multiple lines.

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

Re: [HACKERS] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

[Michael Paquier]

On Thu, Nov 2, 2017 at 3:47 AM, Peter Eisentraut
 wrote:
> So to summarize, I think there is interest in this functionality, but
> the patch needs some work.

This patch has been waiting for input from its author for three weeks,
so I am marking it as returned with feedback.
-- 
Michael

Re: [HACKERS] Fix bloom WAL tap test

[Michael Paquier]

On Mon, Nov 13, 2017 at 7:13 PM, Alexander Korotkov
 wrote:
> On Fri, Nov 10, 2017 at 9:12 PM, Tom Lane  wrote:
>> I wrote:
>> > Is there anything we can do to cut the runtime of the TAP test to
>> > the point where running it by default wouldn't be so painful?
>>
>> As an experiment, I tried simply cutting the size of the test table 10X:
>>
>> diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
>> index 1b319c9..566abf9 100644
>> --- a/contrib/bloom/t/001_wal.pl
>> +++ b/contrib/bloom/t/001_wal.pl
>> @@ -57,7 +57,7 @@ $node_standby->start;
>>  $node_master->safe_psql("postgres", "CREATE EXTENSION bloom;");
>>  $node_master->safe_psql("postgres", "CREATE TABLE tst (i int4, t
>> text);");
>>  $node_master->safe_psql("postgres",
>> -"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series(1,10) i;"
>> +"INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series(1,1) i;"
>>  );
>>  $node_master->safe_psql("postgres",
>> "CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 =
>> 3);");
>> @@ -72,7 +72,7 @@ for my $i (1 .. 10)
>> test_index_replay("delete $i");
>> $node_master->safe_psql("postgres", "VACUUM tst;");
>> test_index_replay("vacuum $i");
>> -   my ($start, $end) = (11 + ($i - 1) * 1, 10 + $i *
>> 1);
>> +   my ($start, $end) = (10001 + ($i - 1) * 1000, 1 + $i * 1000);
>> $node_master->safe_psql("postgres",
>>  "INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM
>> generate_series($start,$end) i;"
>> );
>>
>> This about halved the runtime of the TAP test, and it changed the coverage
>> footprint not at all according to lcov.  (Said coverage is only marginally
>> better than what we get without running the bloom TAP test, AFAICT.)
>>
>> It seems like some effort could be put into both shortening this test
>> and improving the amount of code it exercises.
>
> Thank you for committing patch which fixes tap test.
> I'll try to improve coverage of this test and reduce its run time.

I am marking this CF entry as committed, as the switch to psql_safe
has been committed. In order to improve the run time and coverage of
the tests. Let's spawn a new thread.
-- 
Michael

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Michael Paquier]

On Mon, Oct 2, 2017 at 3:29 PM, Michael Paquier
 wrote:
> On Mon, Oct 2, 2017 at 8:13 AM, Daniel Gustafsson  wrote:
>> I’ve moved this to the next CF, but since this no longer applies cleanly I’ve
>> reset it to Waiting for author.
>
> Thanks Daniel for the reminder. Attached are rebased patches. This
> thing rots easily...

This set of patches still applies, and is marked as ready for
committer. Are any of the committers cited on this thread, aka Magnus,
Heikki, Tom interested in this patch set? Or not? We are close to the
end of CF 2017-11, so I am bumping it to the next one.
-- 
Michael

[PATCH] Atomic pgrename on Windows

[Alexander Korotkov]

Hi!

It's assumed in PostgreSQL codebase that pgrename atomically replaces
target file with source file even if target file is open and being read by
another process.  And this assumption is true on Linux, but it's false on
Windows.  MoveFileEx() triggers an error when target file is open (and
accordingly locked).  Some our customers has been faced such errors while
operating heavily loaded PostgreSQL instance on Windows.

LOG could not rename temporary statistics file "pg_stat_tmp/global.tmp" to
"pg_stat_tmp/global.stat": Permission denied

I've managed to reproduce this situation.  Reliable reproducing of this
issue required patch to PostgreSQL core.  I've written
slow-read-statfiles.patch for artificial slowdown
of pgstat_read_statsfiles() – sleep 100 ms after each statfile entry.  If
you run make-100-dbs.sql on patched version, and then few times execute

select pg_stat_get_tuples_inserted('t1'::regclass);

in psql, then you would likely get the error above on Windows.

Attached patch atomic-pgrename-windows-1.patch fixes this problem.  It
appears to be possible to atomically replace file on Windows –
ReplaceFile() does that.  ReplaceFiles() requires target file to exist,
this is why we still need to call MoveFileEx() when it doesn't exist.

This patch is based on work of Victor Spirin who was asked by Postgres Pro
to research this problem.

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


slow-read-statfiles.patch
Description: Binary data


make-100-dbs.sql
Description: Binary data


atomic-pgrename-windows-1.patch
Description: Binary data

Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

[Jing Wang]

A few general comments.

+   FreeSpaceMapVacuum(onerel, 64);

Just want to know why '64' is used here? It's better to give a description.

+else
+   {
+   newslot = fsm_get_avail(page, 0);
+   }

Since there is only one line in the else the bracket will not be needed.
And there in one more space ahead of else which should be removed.


+   if (nindexes == 0 &&
+   (vacuumed_pages_at_fsm_vac - vacuumed_pages) >
vacuum_fsm_every_pages)
+   {
+   /* Vacuum the Free Space Map */
+   FreeSpaceMapVacuum(onerel, 0);
+   vacuumed_pages_at_fsm_vac = vacuumed_pages;
+   }

vacuumed_pages_at_fsm_vac is initialised with value zero and seems no
chance to go into the bracket.


Regards,
Jing Wang
Fujitsu Australia

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Mon, Nov 27, 2017 at 10:28 AM, Andreas Karlsson  wrote:
> Hm, after reading more of our MSVC code it seems like building with MSVC
> does not really use switch, people rather have to create a config.pl. Is
> that correct? The MSVC scripts also seems to only support uuid-ossp which it
> just calls $config->{uuid}.
>
> If so we could either have:
>
> $config->{ssl} = "openssl";
> $config->{sslpath} = "/path/to/openssl";

Using this one may actually finish by being cleaner as there is no
need to cross-check for both options defined.
-- 
Michael

Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

[Etsuro Fujita]

(2017/11/27 7:56), Tom Lane wrote:

Etsuro Fujita  writes:

[ fix-rewrite-tlist-v4.patch ]


I started reviewing this patch.


Great!


I did not much like the fact that it
effectively moved rewriteTargetListUD to a different file and renamed it.
That seems like unnecessary code churn, plus it breaks the analogy with
rewriteTargetListIU, plus it will make back-patching harder (since that
code isn't exactly the same in back branches).  I see little reason why
we can't leave it where it is and just make it non-static.  It's not like
there's no other parts of the rewriter that the planner calls.


Agreed.

Best regards,
Etsuro Fujita

Re: [HACKERS] pg_stat_wal_write statistics view

[Haribabu Kommi]

On Wed, Nov 8, 2017 at 8:46 AM, Robert Haas  wrote:

> On Tue, Nov 7, 2017 at 4:31 AM, Haribabu Kommi 
> wrote:
> >> Updated patch attached.
> > Patch rebased.
>
> I think the earlier concerns about the performance impact of this are
> probably very valid concerns, and I don't see how the new version of
> the patch gets us much closer to solving them.
>

I will check the performance with the changes of removing the stats
collector
usage and provide the details.


> I am also not sure I understand how the backend_write_blocks column is
> intended to work.  The only call to pgstat_send_walwrites() is in
> WalWriterMain, so where do the other backends report anything?
>

With the current patch, All the backends update the stats in shared memory
structure and only WAL writer process gathers the stats and share with the
stats collector.


> Also, if there's only ever one global set of counters (as opposed to
> one per table, say) then why use the stats collector machinery for
> this at all, vs. having a structure in shared memory that can be
> updated directly?  It seems like adding a lot of overhead for no
> functional benefit.
>

Yes, I agree that using stats collector for these stats is an overhead.
I change the patch to use just the shared memory structure and
gather the performance results and post it to the next commitfest.

Currently I marked the patch as "returned with feedback" in the
ongoing commitfest.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Jing Wang]

Hi All,

This is a patch for current_database working on ALTER ROLE/GRANT/REVOKE
statements which should be applied after the previous patch
"comment_on_current_database_no_pgdump_v4.4.patch".

By using the patch the CURRENT_DATABASE can working in the following SQL
statements:

ALTER ROLE ... IN DATABASE CURRENT_DATABASE SET/RESET
configuration_parameter
GRANT ... ON DATABASE CURRENT_DATABASE TO role_specification ...
REVOKE ... ON DATABASE CURRENT_DATABASE FROM ...


Regards,
Jing Wang
Fujitsu Australia


current_database_on_grant_revoke_role_v4.4.patch
Description: Binary data

Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Masahiko Sawada]

On Wed, Nov 22, 2017 at 11:32 AM, Masahiko Sawada  wrote:
> On Wed, Nov 22, 2017 at 5:25 AM, Robert Haas  wrote:
>> On Mon, Nov 20, 2017 at 5:19 PM, Masahiko Sawada  
>> wrote:
>>> Attached updated version patch. I've moved only relation extension
>>> locks out of heavy-weight lock as per discussion so far.
>>>
>>> I've done a write-heavy benchmark on my laptop; loading 24kB data to
>>> one table using COPY by 1 client, for 10 seconds. The through-put of
>>> patched is 10% better than current HEAD. The result of 5 times is the
>>> following.
>>>
>>> - PATCHED -
>>> tps = 178.791515 (excluding connections establishing)
>>> tps = 176.522693 (excluding connections establishing)
>>> tps = 168.705442 (excluding connections establishing)
>>> tps = 158.158009 (excluding connections establishing)
>>> tps = 161.145709 (excluding connections establishing)
>>>
>>> - HEAD -
>>> tps = 147.079803 (excluding connections establishing)
>>> tps = 149.079540 (excluding connections establishing)
>>> tps = 149.082275 (excluding connections establishing)
>>> tps = 148.255376 (excluding connections establishing)
>>> tps = 145.542552 (excluding connections establishing)
>>>
>>> Also I've done a micro-benchmark; calling LockRelationForExtension and
>>> UnlockRelationForExtension tightly in order to measure the number of
>>> lock/unlock cycles per second. The result is,
>>> PATCHED = 3.95892e+06 (cycles/sec)
>>> HEAD = 1.15284e+06 (cycles/sec)
>>> The patched is 3 times faster than current HEAD.
>>>
>>> Attached updated patch and the function I used for micro-benchmark.
>>> Please review it.
>>
>> That's a nice speed-up.
>>
>> How about a preliminary patch that asserts that we never take another
>> heavyweight lock while holding a relation extension lock?
>>
>
> Agreed. Also, since we disallow to holding more than one locks of
> different relations at once I'll add an assertion for it as well.
>
> I think we no longer need to pass the lock level to
> UnloclRelationForExtension(). Now that relation extension lock will be
> simple we can release the lock in the mode that we used to acquire
> like LWLock.
>

Attached latest patch incorporated all comments so far. Please review it.

Regards,

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


Moving_extension_lock_out_of_heavyweight_lock_v7.patch
Description: Binary data

Re: [HACKERS] GnuTLS support

[Andreas Karlsson]

On 11/27/2017 02:20 AM, Michael Paquier wrote:

On Mon, Nov 27, 2017 at 10:05 AM, Andreas Karlsson  wrote:

The script for the windows version takes the
--with-openssl= switch so that cannot just be translated to a single
--with-ssl switch. Should to have both --with-openssl and --with-gnutls or
--with-ssl=(openssl|gnutls) and --with-ssl-path=? I also do not know
the Windows build code very well (or really at all).


By default --with-openssl does not take a path with ./configure. When
using gnutls, do the name of the libraries and the binaries generated
change compared to openssl? If yes, and I guess that it is the case,
you will need to be able to make the difference between gnutls and
openssl anyway as the set of perl scripts in src/tools/msvc need to
make the difference with deliverables at name-level. I would be
personally fine with just listing gnutls in the list of options and
comment it as --with-ssl=, and change the openssl comment to
match that.


Hm, after reading more of our MSVC code it seems like building with MSVC 
does not really use switch, people rather have to create a config.pl. Is 
that correct? The MSVC scripts also seems to only support uuid-ossp 
which it just calls $config->{uuid}.


If so we could either have:

$config->{ssl} = "openssl";
$config->{sslpath} = "/path/to/openssl";

or

$config->{ssl} = "openssl";
$config->{openssl} = "/path/to/openssl";

or

$config->{openssl} = "/path/to/openssl";
vs
$config->{gnutls} = "/path/to/gnutls";

Andreas

Typo in config_default.pl comment

[Andreas Karlsson]

Hi,

There is a --with-tls in the comments in config_default.pl which should 
be --with-tcl.


Andreas

diff --git a/src/tools/msvc/config_default.pl b/src/tools/msvc/config_default.pl
index 4d69dc2a2e..d7a9fc5039 100644
--- a/src/tools/msvc/config_default.pl
+++ b/src/tools/msvc/config_default.pl
@@ -18,7 +18,7 @@ our $config = {
 	icu   => undef,# --with-icu=
 	nls   => undef,# --enable-nls=
 	tap_tests => undef,# --enable-tap-tests
-	tcl   => undef,# --with-tls=
+	tcl   => undef,# --with-tcl=
 	perl  => undef,# --with-perl
 	python=> undef,# --with-python=
 	openssl   => undef,# --with-openssl=

Re: [HACKERS] GnuTLS support

[Michael Paquier]

On Mon, Nov 27, 2017 at 10:05 AM, Andreas Karlsson  wrote:
> The script for the windows version takes the
> --with-openssl= switch so that cannot just be translated to a single
> --with-ssl switch. Should to have both --with-openssl and --with-gnutls or
> --with-ssl=(openssl|gnutls) and --with-ssl-path=? I also do not know
> the Windows build code very well (or really at all).

By default --with-openssl does not take a path with ./configure. When
using gnutls, do the name of the libraries and the binaries generated
change compared to openssl? If yes, and I guess that it is the case,
you will need to be able to make the difference between gnutls and
openssl anyway as the set of perl scripts in src/tools/msvc need to
make the difference with deliverables at name-level. I would be
personally fine with just listing gnutls in the list of options and
comment it as --with-ssl=, and change the openssl comment to
match that.
-- 
Michael

Re: [HACKERS] More stats about skipped vacuums

[Michael Paquier]

On Mon, Nov 27, 2017 at 5:19 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Sat, Nov 25, 2017 at 12:09 PM, Tom Lane  wrote:
>>> Mumble.  It's a property I'm pretty hesitant to give up, especially
>>> since the stats views have worked like that since day one.  It's
>>> inevitable that weakening that guarantee would break peoples' queries,
>>> probably subtly.
>
>> You mean, queries against the stats views, or queries in general?  If
>> the latter, by what mechanism would the breakage happen?
>
> Queries against the stats views, of course.

There has been much discussion on this thread, and the set of patches
as presented may hurt performance for users with a large number of
tables, so I am marking it as returned with feedback.
-- 
Michael

Re: [HACKERS] More stats about skipped vacuums

[Michael Paquier]

On Mon, Nov 27, 2017 at 2:49 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> ... Would you think
>> that it is acceptable to add the number of index scans that happened
>> with the verbose output then?
>
> I don't have an objection to it, but can't you tell that from VACUUM
> VERBOSE already?  There should be a "INFO:  scanned index" line for
> each scan.

Of course, sorry for the noise.
-- 
Michael

Re: has_sequence_privilege() never got the memo

[Joe Conway]

On 11/23/2017 07:16 AM, Peter Eisentraut wrote:
> On 11/22/17 22:58, Tom Lane wrote:
>> Joe Conway  writes:
>>> I just noticed that has_sequence_privilege() never got the memo about
>>> "WITH GRANT OPTION". Any objections to the attached going back to all
>>> supported versions?
>> 
>> That looks odd.  Patch certainly makes this case consistent with the
>> rest of acl.c, but maybe there's some deeper reason?  Peter?
> 
> No I think it was just forgotten.

Pushed.

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Fix crash in int8_avg_combine().

[Andres Freund]

Hi Hadi,


On 2017-11-25 22:43:49 -0500, Hadi Moshayedi wrote:
> While doing some tests on REL_10_STABLE, I was getting run-time exceptions
> at int8_avg_combine() at the following line:
> 
> state1->sumX = state2->sumX;
> 
> After some debugging, I noticed that palloc()’s alignment is 8-bytes, while
> this statement (which moves a __int128 from one memory location to another
> memory location) expects 16-byte memory alignments. So when either state1
> or state2 is not 16-byte aligned, this crashes.
> 
> When I disassemble the code, the above statement is translated to a pair of
> movdqa and movaps assignments when compiled with -O2:
> 
> movdqa  c(%rdx), %xmm0
> movaps  %xmm0, c(%rcx)
> 
> Looking at “Intel 64 and IA-32 Architectures Software Developer’s Manual,
> Volume 2”, both of these instructions expect 16-byte aligned memory
> locations, or a general-protection exception will be generated.

Nicely analyzed. [Un]fortunately the bug has already been found and
fixed:
https://git.postgresql.org/pg/commitdiff/619a8c47da7279c186bb57cc16b26ad011366b73

Will be included in the next set of back branch releases.

> diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
> index 869c59dc85..2dc59e89cd 100644
> --- a/src/include/utils/memutils.h
> +++ b/src/include/utils/memutils.h
> @@ -189,7 +189,7 @@ extern MemoryContext SlabContextCreate(MemoryContext 
> parent,
>   * Few callers should be interested in this, but tuplesort/tuplestore need
>   * to know it.
>   */
> -#define ALLOCSET_SEPARATE_THRESHOLD  8192
> +#define ALLOCSET_SEPARATE_THRESHOLD  16384

Huh, what's that about in this context?

Greetings,

Andres Freund

Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Tue, Nov 21, 2017 at 1:36 PM, Michael Paquier
 wrote:
> So attached are rebased patches:
> - 0001 to introduce the connection parameter saslchannelbinding, which
> allows libpq to enforce the type of channel binding used during an
> exchange.
> - 0002 to add tls-endpoint as channel binding type, which is where 0001 
> shines.

Attached is a rebased patch set, documentation failing to compile. I
am moving at the same time this patch set to the next commit fest.
-- 
Michael
From bdd25121ba7c1916d280f97c8e1a280ad26ea60c Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 10 Oct 2017 22:04:22 +0900
Subject: [PATCH 2/2] Implement channel binding tls-server-end-point for SCRAM

As referenced in RFC 5929, this channel binding is not the default value
and uses a hash of the certificate as binding data. On the frontend, this
can be resumed in getting the data from SSL_get_peer_certificate() and
on the backend SSL_get_certificate().

The hashing algorithm needs also to switch to SHA-256 if the signature
algorithm is MD5 or SHA-1, so let's be careful about that.
---
 doc/src/sgml/protocol.sgml   |  5 +-
 src/backend/libpq/auth-scram.c   | 26 ---
 src/backend/libpq/auth.c |  8 +++-
 src/backend/libpq/be-secure-openssl.c| 61 +
 src/include/libpq/libpq-be.h |  1 +
 src/include/libpq/scram.h|  4 +-
 src/interfaces/libpq/fe-auth-scram.c | 24 +++---
 src/interfaces/libpq/fe-auth.c   | 12 -
 src/interfaces/libpq/fe-auth.h   |  4 +-
 src/interfaces/libpq/fe-secure-openssl.c | 78 
 src/interfaces/libpq/libpq-int.h |  1 +
 src/test/ssl/t/002_scram.pl  |  5 +-
 12 files changed, 210 insertions(+), 19 deletions(-)

diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 8174e3defa..365f72b51d 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1576,8 +1576,9 @@ the password is in.
   
 Channel binding is supported in PostgreSQL builds with
 SSL support. The SASL mechanism name for SCRAM with channel binding
-is SCRAM-SHA-256-PLUS.  The only channel binding type
-supported at the moment is tls-unique, defined in RFC 5929.
+is SCRAM-SHA-256-PLUS.  Two channel binding types are
+supported at the moment: tls-unique, which is the default,
+and tls-server-end-point, both defined in RFC 5929.
   
 
 
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 22103ce479..8f96e3927e 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -113,6 +113,8 @@ typedef struct
 	bool		ssl_in_use;
 	const char *tls_finished_message;
 	size_t		tls_finished_len;
+	const char *certificate_hash;
+	size_t		certificate_hash_len;
 	char	   *channel_binding_type;
 
 	int			iterations;
@@ -175,7 +177,9 @@ pg_be_scram_init(const char *username,
  const char *shadow_pass,
  bool ssl_in_use,
  const char *tls_finished_message,
- size_t tls_finished_len)
+ size_t tls_finished_len,
+ const char *certificate_hash,
+ size_t certificate_hash_len)
 {
 	scram_state *state;
 	bool		got_verifier;
@@ -186,6 +190,8 @@ pg_be_scram_init(const char *username,
 	state->ssl_in_use = ssl_in_use;
 	state->tls_finished_message = tls_finished_message;
 	state->tls_finished_len = tls_finished_len;
+	state->certificate_hash = certificate_hash;
+	state->certificate_hash_len = certificate_hash_len;
 	state->channel_binding_type = NULL;
 
 	/*
@@ -852,13 +858,15 @@ read_client_first_message(scram_state *state, char *input)
 }
 
 /*
- * Read value provided by client; only tls-unique is supported
- * for now.  (It is not safe to print the name of an
- * unsupported binding type in the error message.  Pranksters
- * could print arbitrary strings into the log that way.)
+ * Read value provided by client; only tls-unique and
+ * tls-server-end-point are supported for now.  (It is
+ * not safe to print the name of an unsupported binding
+ * type in the error message.  Pranksters could print
+ * arbitrary strings into the log that way.)
  */
 channel_binding_type = read_attr_value(, 'p');
-if (strcmp(channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) != 0)
+if (strcmp(channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) != 0 &&
+	strcmp(channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_ENDPOINT) != 0)
 	ereport(ERROR,
 			(errcode(ERRCODE_PROTOCOL_VIOLATION),
 			 (errmsg("unsupported SCRAM channel-binding type";
@@ -1116,6 +1124,12 @@ read_client_final_message(scram_state *state, char *input)
 			cbind_data = state->tls_finished_message;
 			cbind_data_len = state->tls_finished_len;
 		}
+		else if (strcmp(state->channel_binding_type,
+		SCRAM_CHANNEL_BINDING_TLS_ENDPOINT) == 0)
+		{
+			cbind_data = state->certificate_hash;
+			cbind_data_len 

Re: [HACKERS] [POC] Faster processing at Gather node

[Amit Kapila]

On Sat, Nov 25, 2017 at 9:13 PM, Robert Haas  wrote:
> On Wed, Nov 22, 2017 at 8:36 AM, Amit Kapila  wrote:
>>> remove-memory-leak-protection-v1.patch removes the memory leak
>>> protection that Tom installed upon discovering that the original
>>> version of tqueue.c leaked memory like crazy.  I think that it
>>> shouldn't do that any more, courtesy of
>>> 6b65a7fe62e129d5c2b85cd74d6a91d8f7564608.  Assuming that's correct, we
>>> can avoid a whole lot of tuple copying in Gather Merge and a much more
>>> modest amount of overhead in Gather.  Since my test case exercised
>>> Gather Merge, this bought ~400 ms or so.
>>
>> I think Tom didn't installed memory protection in nodeGatherMerge.c
>> related to an additional copy of tuple.  I could see it is present in
>> the original commit of Gather Merge
>> (355d3993c53ed62c5b53d020648e4fbcfbf5f155).  Tom just avoided applying
>> heap_copytuple to a null tuple in his commit
>> 04e9678614ec64ad9043174ac99a25b1dc45233a.  I am not sure whether you
>> are just referring to the protection Tom added in nodeGather.c,  If
>> so, it is not clear from your mail.
>
> Yes, that's what I mean.  What got done for Gather Merge was motivated
> by what Tom did for Gather.  Sorry for not expressing the idea more
> precisely.
>
>> I think we don't need the additional copy of tuple in
>> nodeGatherMerge.c and your patch seem to be doing the right thing.
>> However, after your changes, it looks slightly odd that
>> gather_merge_clear_tuples() is explicitly calling heap_freetuple for
>> the tuples allocated by tqueue.c, maybe we can add some comment.  It
>> was much clear before this patch as nodeGatherMerge.c was explicitly
>> copying the tuples that it is freeing.
>
> OK, I can add a comment about that.
>

Sure, I think apart from that the patch looks good to me.  I think a
good test of this patch could be to try to pass many tuples via gather
merge and see if there is any leak in memory.  Do you have any other
ideas?

>> Isn't it better to explicitly call gather_merge_clear_tuples in
>> ExecEndGatherMerge so that we can free the memory for tuples allocated
>> in this node rather than relying on reset/free of MemoryContext in
>> which those tuples are allocated?
>
> Generally relying on reset/free of a MemoryContext is cheaper.
> Typically you only want to free manually if the freeing would
> otherwise not happen soon enough.
>

Yeah and I think something like that can happen after your patch
because now the memory for tuples returned via TupleQueueReaderNext
will be allocated in ExecutorState and that can last for long.   I
think it is better to free memory, but we can leave it as well if you
don't feel it important.  In any case, I have written a patch, see if
you think it makes sense.

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


release_memory_at_gather_merge_shutdown_v1.patch
Description: Binary data