Re: [HACKERS] More stats about skipped vacuums

[Kyotaro HORIGUCHI]

This is just a repost as a (true) new thread.

At Mon, 30 Oct 2017 20:57:50 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20171030.205750.246076862.horiguchi.kyot...@lab.ntt.co.jp>
> At Fri, 20 Oct 2017 19:15:16 +0900, Masahiko Sawada  
> wrote in 

Re: [HACKERS] More stats about skipped vacuums

[Kyotaro HORIGUCHI]

At Thu, 26 Oct 2017 15:06:30 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20171026.150630.115694437.horiguchi.kyot...@lab.ntt.co.jp>
> At Fri, 20 Oct 2017 19:15:16 +0900, Masahiko Sawada  
> wrote in 

Re: [HACKERS] More stats about skipped vacuums

[Kyotaro HORIGUCHI]

Mmm. I've failed to create a brand-new thread..

Thank you for the comment.

At Fri, 20 Oct 2017 19:15:16 +0900, Masahiko Sawada  
wrote in 

Re: [HACKERS] More stats about skipped vacuums

[Masahiko Sawada]

On Tue, Oct 10, 2017 at 7:26 PM, Kyotaro HORIGUCHI
 wrote:
> Hello.
> Once in a while I am asked about table bloat. In most cases the
> cause is long lasting transactions and vacuum canceling in some
> cases. Whatever the case users don't have enough clues to why
> they have bloated tables.
>
> At the top of the annoyances list for users would be that they
> cannot know whether autovacuum decided that a table needs vacuum
> or not. I suppose that it could be shown in pg_stat_*_tables.
>
>   n_mod_since_analyze | 2
> + vacuum_requred  | true
>   last_vacuum | 2017-10-10 17:21:54.380805+09
>
> If vacuum_required remains true for a certain time, it means that
> vacuuming stopped halfway or someone is killing it repeatedly.
> That status could be shown in the same view.

Because the table statistics is updated at end of the vacuum I think
that the autovacuum will process the table at the next cycle if it has
stopped halfway or has killed. So you mean that vacuum_required is for
uses who want to reclaim garbage without wait for autovacuum retry?

>   n_mod_since_analyze | 2
> + vacuum_requred  | true
>   last_vacuum | 2017-10-10 17:21:54.380805+09
>   last_autovacuum | 2017-10-10 17:21:54.380805+09
> + last_autovacuum_status  | Killed by lock conflict
>
> Where the "Killed by lock conflict" would be one of the followings.
>
>   - Completed (oldest xmin = 8023)
>   - May not be fully truncated (yielded at 1324 of 6447 expected)
>   - Truncation skipped
>   - Skipped by lock failure
>   - Killed by lock conflict
>
>
> If we want more formal expression, we can show the values in the
> following shape. And adding some more values could be useful.
>
>   n_mod_since_analyze  | 2
> + vacuum_requred   | true
> + last_vacuum_oldest_xid   | 8023
> + last_vacuum_left_to_truncate | 5123
> + last_vacuum_truncated| 387
>   last_vacuum  | 2017-10-10 17:21:54.380805+09
>   last_autovacuum  | 2017-10-10 17:21:54.380805+09
> + last_autovacuum_status   | Killed by lock conflict
> ...
>   autovacuum_count | 128
> + incomplete_autovacuum_count  | 53
>
> # The last one might be needless..

I'm not sure that the above informations will help for users or DBA
but personally I sometimes want to have the number of index scans of
the last autovacuum in the pg_stat_user_tables view. That value
indicates how efficiently vacuums performed and would be a signal to
increase the setting of autovacuum_work_mem for user.

> Where the "Killed by lock conflict" is one of the followings.
>
>- Completed
>- Truncation skipped
>- Partially truncated
>- Skipped
>- Killed by lock conflict
>
> This seems enough to find the cause of a table bloat. The same
> discussion could be applied to analyze but it might be the
> another issue.
>
> There may be a better way to indicate the vacuum soundness. Any
> opinions and suggestions are welcome.
>
> I'm going to make a patch to do the 'formal' one for the time
> being.
>

Regards,

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


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

[HACKERS] More stats about skipped vacuums

[Kyotaro HORIGUCHI]

Hello.
Once in a while I am asked about table bloat. In most cases the
cause is long lasting transactions and vacuum canceling in some
cases. Whatever the case users don't have enough clues to why
they have bloated tables.

At the top of the annoyances list for users would be that they
cannot know whether autovacuum decided that a table needs vacuum
or not. I suppose that it could be shown in pg_stat_*_tables.

  n_mod_since_analyze | 2
+ vacuum_requred  | true
  last_vacuum | 2017-10-10 17:21:54.380805+09

If vacuum_required remains true for a certain time, it means that
vacuuming stopped halfway or someone is killing it repeatedly.
That status could be shown in the same view.

  n_mod_since_analyze | 2
+ vacuum_requred  | true
  last_vacuum | 2017-10-10 17:21:54.380805+09
  last_autovacuum | 2017-10-10 17:21:54.380805+09
+ last_autovacuum_status  | Killed by lock conflict

Where the "Killed by lock conflict" would be one of the followings.

  - Completed (oldest xmin = 8023)
  - May not be fully truncated (yielded at 1324 of 6447 expected)
  - Truncation skipped
  - Skipped by lock failure
  - Killed by lock conflict


If we want more formal expression, we can show the values in the
following shape. And adding some more values could be useful.

  n_mod_since_analyze  | 2
+ vacuum_requred   | true
+ last_vacuum_oldest_xid   | 8023
+ last_vacuum_left_to_truncate | 5123
+ last_vacuum_truncated| 387
  last_vacuum  | 2017-10-10 17:21:54.380805+09
  last_autovacuum  | 2017-10-10 17:21:54.380805+09
+ last_autovacuum_status   | Killed by lock conflict
...
  autovacuum_count | 128
+ incomplete_autovacuum_count  | 53

# The last one might be needless..

Where the "Killed by lock conflict" is one of the followings.

   - Completed
   - Truncation skipped
   - Partially truncated
   - Skipped
   - Killed by lock conflict

This seems enough to find the cause of a table bloat. The same
discussion could be applied to analyze but it might be the
another issue.

There may be a better way to indicate the vacuum soundness. Any
opinions and suggestions are welcome.

I'm going to make a patch to do the 'formal' one for the time
being.

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] More efficient truncation of pg_stat_activity query strings

[Andres Freund]

Hi,

On 2017-09-15 17:43:35 +0530, Kuntal Ghosh wrote:
> The patch looks good to me. I've done some regression testing with a
> custom script on my local system. The script contains the following
> statement:

> SELECT 'aaa..' as col;
> 
> Test 1
> ---
> duration: 300 seconds
> clients/threads: 1

> With the patch TPS:13628 (+3.39%)
> +0.36% 0.21%  postgres  postgres  [.] pgstat_report_activity
> 
> Test 2
> ---
> duration: 300 seconds
> clients/threads: 8

> With the patch TPS: 63949 (+20.4%)
> +0.40% 0.25%  postgres  postgres  [.] pgstat_report_activity
> 
> This shows the significance of this patch in terms of performance
> improvement of pgstat_report_activity. Is there any other tests I
> should do for the same?

Thanks for the test! I think this looks good, no further tests
necessary.

Greetings,

Andres Freund


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

Re: [HACKERS] More efficient truncation of pg_stat_activity query strings

[Robert Haas]

On Fri, Sep 15, 2017 at 4:49 PM, Andres Freund  wrote:
> On 2017-09-15 16:45:47 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>> > Version correcting these is attached. Thanks!
>>
>> I'd vote for undoing the s/st_activity/st_activity_raw/g business.
>> That may have been useful while writing the patch, to ensure you
>> found all the references; but it's just creating a lot of unnecessary
>> delta in the final code, with the attendant hazards for back-patches.
>
> I was wondering about that too (see [1]). My only concern is that some
> extensions out there might be accessing the string expecting it to be
> properly truncated. The rename at least forces them to look for the new
> name...  I'm slightly in favor of keeping the rename, it doesn't seem
> likely to cause a lot of backpatch pain.

I tend to agree with you, but it's not a huge deal either way.  Even
if somebody fails to update third-party code that touches this, a lot
of times it'll work anyway.  But that very fact is, of course, why it
would be slightly better to break it explicitly.

-- 
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] More efficient truncation of pg_stat_activity query strings

[Andres Freund]

On 2017-09-15 16:45:47 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Version correcting these is attached. Thanks!
> 
> I'd vote for undoing the s/st_activity/st_activity_raw/g business.
> That may have been useful while writing the patch, to ensure you
> found all the references; but it's just creating a lot of unnecessary
> delta in the final code, with the attendant hazards for back-patches.

I was wondering about that too (see [1]). My only concern is that some
extensions out there might be accessing the string expecting it to be
properly truncated. The rename at least forces them to look for the new
name...  I'm slightly in favor of keeping the rename, it doesn't seem
likely to cause a lot of backpatch pain.

Regards,

Andres Freund


[1] 
http://archives.postgresql.org/message-id/20170914060329.j7lt7oyyzquxiut6%40alap3.anarazel.de


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

Re: [HACKERS] More efficient truncation of pg_stat_activity query strings

[Tom Lane]

Andres Freund  writes:
> Version correcting these is attached. Thanks!

I'd vote for undoing the s/st_activity/st_activity_raw/g business.
That may have been useful while writing the patch, to ensure you
found all the references; but it's just creating a lot of unnecessary
delta in the final code, with the attendant hazards for back-patches.

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] More efficient truncation of pg_stat_activity query strings

[Andres Freund]

On 2017-09-15 08:25:09 -0400, Robert Haas wrote:
> On Thu, Sep 14, 2017 at 2:03 AM, Andres Freund  wrote:
> > Here's a patch that implements that idea.
> 
> From the department of cosmetic nitpicking:
> 
> + * All supported server-encodings allow to determine the length of a
> 
> make it possible to determine
> 
> + * multi-byte character from it's first byte (not the case for client
> 
> its
> 
> this is not the case
> 
> + * encodings, see GB18030). As st_activity always is stored using server
> 
> is always stored using a server
> 
> + * pgstat_clip_activity() to trunctae correctly.

Version correcting these is attached. Thanks!

Greetings,

Andres Freund
>From a2325a2649da403dc562b8e93df972839823d924 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 13 Sep 2017 19:25:34 -0700
Subject: [PATCH] Speedup pgstat_report_activity by moving mb-aware truncation
 to read side.

Previously multi-byte aware truncation was done on every
pgstat_report_activity() call - proving to be a bottleneck for
workloads with long query strings that execute quickly.

Instead move the truncation to the read side, which is commonly
executed far less frequently. That's possible because all server
encodings allow to determine the length of a multi-byte string from
the first byte.

Author: Andres Freund
Discussion: https://postgr.es/m/20170912071948.pa7igbpkkkvie...@alap3.anarazel.de
---
 src/backend/postmaster/pgstat.c | 63 -
 src/backend/utils/adt/pgstatfuncs.c | 17 +++---
 src/include/pgstat.h| 12 +--
 3 files changed, 72 insertions(+), 20 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index accf302cf7..1ffdac5448 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2701,7 +2701,7 @@ CreateSharedBackendStatus(void)
 		buffer = BackendActivityBuffer;
 		for (i = 0; i < NumBackendStatSlots; i++)
 		{
-			BackendStatusArray[i].st_activity = buffer;
+			BackendStatusArray[i].st_activity_raw = buffer;
 			buffer += pgstat_track_activity_query_size;
 		}
 	}
@@ -2922,11 +2922,11 @@ pgstat_bestart(void)
 #endif
 	beentry->st_state = STATE_UNDEFINED;
 	beentry->st_appname[0] = '\0';
-	beentry->st_activity[0] = '\0';
+	beentry->st_activity_raw[0] = '\0';
 	/* Also make sure the last byte in each string area is always 0 */
 	beentry->st_clienthostname[NAMEDATALEN - 1] = '\0';
 	beentry->st_appname[NAMEDATALEN - 1] = '\0';
-	beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0';
+	beentry->st_activity_raw[pgstat_track_activity_query_size - 1] = '\0';
 	beentry->st_progress_command = PROGRESS_COMMAND_INVALID;
 	beentry->st_progress_command_target = InvalidOid;
 
@@ -3017,7 +3017,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 			pgstat_increment_changecount_before(beentry);
 			beentry->st_state = STATE_DISABLED;
 			beentry->st_state_start_timestamp = 0;
-			beentry->st_activity[0] = '\0';
+			beentry->st_activity_raw[0] = '\0';
 			beentry->st_activity_start_timestamp = 0;
 			/* st_xact_start_timestamp and wait_event_info are also disabled */
 			beentry->st_xact_start_timestamp = 0;
@@ -3034,8 +3034,12 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	start_timestamp = GetCurrentStatementStartTimestamp();
 	if (cmd_str != NULL)
 	{
-		len = pg_mbcliplen(cmd_str, strlen(cmd_str),
-		   pgstat_track_activity_query_size - 1);
+		/*
+		 * Compute length of to-be-stored string unaware of multi-byte
+		 * characters. For speed reasons that'll get corrected on read, rather
+		 * than computed every write.
+		 */
+		len = Min(strlen(cmd_str), pgstat_track_activity_query_size - 1);
 	}
 	current_timestamp = GetCurrentTimestamp();
 
@@ -3049,8 +3053,8 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 
 	if (cmd_str != NULL)
 	{
-		memcpy((char *) beentry->st_activity, cmd_str, len);
-		beentry->st_activity[len] = '\0';
+		memcpy((char *) beentry->st_activity_raw, cmd_str, len);
+		beentry->st_activity_raw[len] = '\0';
 		beentry->st_activity_start_timestamp = start_timestamp;
 	}
 
@@ -3278,8 +3282,8 @@ pgstat_read_current_status(void)
  */
 strcpy(localappname, (char *) beentry->st_appname);
 localentry->backendStatus.st_appname = localappname;
-strcpy(localactivity, (char *) beentry->st_activity);
-localentry->backendStatus.st_activity = localactivity;
+strcpy(localactivity, (char *) beentry->st_activity_raw);
+localentry->backendStatus.st_activity_raw = localactivity;
 localentry->backendStatus.st_ssl = beentry->st_ssl;
 #ifdef USE_SSL
 if (beentry->st_ssl)
@@ -3945,10 +3949,13 @@ pgstat_get_backend_current_activity(int pid, bool checkUser)
 			/* Now it is safe to use the non-volatile pointer */
 			if (checkUser && !superuser() && beentry->st_userid != GetUserId())
 return "";
-			else if (*(beentry->st_activity) == '\0')
+			

Re: [HACKERS] More flexible LDAP auth search filters?

[Peter Eisentraut]

On 9/12/17 19:04, Thomas Munro wrote:
>> Any further thoughts on the test suite?  Otherwise I'll commit it as we
>> have it, for manual use.

done

> I wonder if there is a reasonable way to indicate or determine whether
> you have slapd installed so that check-world could run this test...

The other problem is the open TCP/IP socket, so we can't really run it
by default anyway, like the SSL tests.

-- 
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] More efficient truncation of pg_stat_activity query strings

[Robert Haas]

On Thu, Sep 14, 2017 at 2:03 AM, Andres Freund  wrote:
> Here's a patch that implements that idea.

>From the department of cosmetic nitpicking:

+ * All supported server-encodings allow to determine the length of a

make it possible to determine

+ * multi-byte character from it's first byte (not the case for client

its

this is not the case

+ * encodings, see GB18030). As st_activity always is stored using server

is always stored using a server

+ * pgstat_clip_activity() to trunctae correctly.

truncate

-- 
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] More efficient truncation of pg_stat_activity query strings

[Kuntal Ghosh]

On Thu, Sep 14, 2017 at 11:33 AM, Andres Freund  wrote:
> On 2017-09-12 00:19:48 -0700, Andres Freund wrote:
>> Hi,
>>
>> I've recently seen a benchmark in which pg_mbcliplen() showed up
>> prominently. Which it will basically in any benchmark with longer query
>> strings, but fast queries. That's not that uncommon.
>>
>> I wonder if we could avoid the cost of pg_mbcliplen() from within
>> pgstat_report_activity(), by moving some of the cost to the read
>> side. pgstat values are obviously read far less frequently in nearly all
>> cases that are performance relevant.
>>
>> Therefore I wonder if we couldn't just store a querystring that's
>> essentially just a memcpy()ed prefix, and do a pg_mbcliplen() on the
>> read side.  I think that should work because all *server side* encodings
>> store character lengths in the *first* byte of a multibyte character
>> (at least one clientside encoding, gb18030, doesn't behave that way).
>>
>> That'd necessitate an added memory copy in pg_stat_get_activity(), but
>> that seems fairly harmless.
>>
>> Faults in my thinking?
>
> Here's a patch that implements that idea.  Seems to work well.  I'm a
> bit loathe to add proper regression tests for this, seems awfully
> dependent on specific track_activity_query_size settings.  I did confirm
> using gdb that I see incomplete characters before
> pgstat_clip_activity(), but not after.
>
> I've renamed st_activity to st_activity_raw to increase the likelihood
> that potential external users of st_activity notice and adapt. Increases
> the noise, but imo to a very bareable amount. Don't feel strongly
> though.
>
Hello,

The patch looks good to me. I've done some regression testing with a
custom script on my local system. The script contains the following
statement:
SELECT 'aaa..' as col;

Test 1
---
duration: 300 seconds
clients/threads: 1

On HEAD TPS: 13181
+9.30% 0.27%  postgres  postgres [.] pgstat_report_activity
+8.88% 0.02%  postgres  postgres [.] pg_mbcliplen
+7.76% 4.77%  postgres  postgres [.] pg_encoding_mbcliplen
+4.06% 4.06%  postgres  postgres [.] pg_utf_mblen

With the patch TPS:13628 (+3.39%)
+0.36% 0.21%  postgres  postgres  [.] pgstat_report_activity

Test 2
---
duration: 300 seconds
clients/threads: 8

On HEAD TPS: 53084
+   12.17% 0.20%  postgres  postgres [.]
pgstat_report_activity
+   11.83% 0.02%  postgres  postgres [.] pg_mbcliplen
+   11.19% 8.03%  postgres  postgres [.] pg_encoding_mbcliplen
+3.74% 3.73%  postgres  postgres [.] pg_utf_mblen

With the patch TPS: 63949 (+20.4%)
+0.40% 0.25%  postgres  postgres  [.] pgstat_report_activity

This shows the significance of this patch in terms of performance
improvement of pgstat_report_activity. Is there any other tests I
should do for the same?


-- 
Thanks & Regards,
Kuntal Ghosh
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] More efficient truncation of pg_stat_activity query strings

[Andres Freund]

On 2017-09-12 00:19:48 -0700, Andres Freund wrote:
> Hi,
> 
> I've recently seen a benchmark in which pg_mbcliplen() showed up
> prominently. Which it will basically in any benchmark with longer query
> strings, but fast queries. That's not that uncommon.
> 
> I wonder if we could avoid the cost of pg_mbcliplen() from within
> pgstat_report_activity(), by moving some of the cost to the read
> side. pgstat values are obviously read far less frequently in nearly all
> cases that are performance relevant.
> 
> Therefore I wonder if we couldn't just store a querystring that's
> essentially just a memcpy()ed prefix, and do a pg_mbcliplen() on the
> read side.  I think that should work because all *server side* encodings
> store character lengths in the *first* byte of a multibyte character
> (at least one clientside encoding, gb18030, doesn't behave that way).
> 
> That'd necessitate an added memory copy in pg_stat_get_activity(), but
> that seems fairly harmless.
> 
> Faults in my thinking?

Here's a patch that implements that idea.  Seems to work well.  I'm a
bit loathe to add proper regression tests for this, seems awfully
dependent on specific track_activity_query_size settings.  I did confirm
using gdb that I see incomplete characters before
pgstat_clip_activity(), but not after.

I've renamed st_activity to st_activity_raw to increase the likelihood
that potential external users of st_activity notice and adapt. Increases
the noise, but imo to a very bareable amount. Don't feel strongly
though.

Greetings,

Andres Freund
>From 9c9503f0dfe1babb21e81c1955e996ad06c93608 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 13 Sep 2017 19:25:34 -0700
Subject: [PATCH 1/8] Speedup pgstat_report_activity by moving mb-aware
 truncation to read side.

Previously multi-byte aware truncation was done on every
pgstat_report_activity() call - proving to be a bottleneck for
workloads with long query strings that execute quickly.

Instead move the truncation to the read side, which is commonly
executed far less frequently. That's possible because all server
encodings allow to determine the length of a multi-byte string from
the first byte.

Author: Andres Freund
Discussion: https://postgr.es/m/20170912071948.pa7igbpkkkvie...@alap3.anarazel.de
---
 src/backend/postmaster/pgstat.c | 63 -
 src/backend/utils/adt/pgstatfuncs.c | 17 +++---
 src/include/pgstat.h| 12 +--
 3 files changed, 72 insertions(+), 20 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index accf302cf7..ccb528e627 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2701,7 +2701,7 @@ CreateSharedBackendStatus(void)
 		buffer = BackendActivityBuffer;
 		for (i = 0; i < NumBackendStatSlots; i++)
 		{
-			BackendStatusArray[i].st_activity = buffer;
+			BackendStatusArray[i].st_activity_raw = buffer;
 			buffer += pgstat_track_activity_query_size;
 		}
 	}
@@ -2922,11 +2922,11 @@ pgstat_bestart(void)
 #endif
 	beentry->st_state = STATE_UNDEFINED;
 	beentry->st_appname[0] = '\0';
-	beentry->st_activity[0] = '\0';
+	beentry->st_activity_raw[0] = '\0';
 	/* Also make sure the last byte in each string area is always 0 */
 	beentry->st_clienthostname[NAMEDATALEN - 1] = '\0';
 	beentry->st_appname[NAMEDATALEN - 1] = '\0';
-	beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0';
+	beentry->st_activity_raw[pgstat_track_activity_query_size - 1] = '\0';
 	beentry->st_progress_command = PROGRESS_COMMAND_INVALID;
 	beentry->st_progress_command_target = InvalidOid;
 
@@ -3017,7 +3017,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 			pgstat_increment_changecount_before(beentry);
 			beentry->st_state = STATE_DISABLED;
 			beentry->st_state_start_timestamp = 0;
-			beentry->st_activity[0] = '\0';
+			beentry->st_activity_raw[0] = '\0';
 			beentry->st_activity_start_timestamp = 0;
 			/* st_xact_start_timestamp and wait_event_info are also disabled */
 			beentry->st_xact_start_timestamp = 0;
@@ -3034,8 +3034,12 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	start_timestamp = GetCurrentStatementStartTimestamp();
 	if (cmd_str != NULL)
 	{
-		len = pg_mbcliplen(cmd_str, strlen(cmd_str),
-		   pgstat_track_activity_query_size - 1);
+		/*
+		 * Compute length of to-be-stored string unaware of multi-byte
+		 * characters. For speed reasons that'll get corrected on read, rather
+		 * than computed every write.
+		 */
+		len = Min(strlen(cmd_str), pgstat_track_activity_query_size - 1);
 	}
 	current_timestamp = GetCurrentTimestamp();
 
@@ -3049,8 +3053,8 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 
 	if (cmd_str != NULL)
 	{
-		memcpy((char *) beentry->st_activity, cmd_str, len);
-		beentry->st_activity[len] = '\0';
+		memcpy((char *) beentry->st_activity_raw, cmd_str, len);
+		beentry->st_activity_raw[len] = '\0';
 		

Re: [HACKERS] More flexible LDAP auth search filters?

[Michael Paquier]

On Wed, Sep 13, 2017 at 8:04 AM, Thomas Munro
 wrote:
> I wonder if there is a reasonable way to indicate or determine whether
> you have slapd installed so that check-world could run this test...

Module::Install's requires_external_bin is one:
http://search.cpan.org/~ether/Module-Install-1.18/lib/Module/Install.pod#requires_external_bin
But the bar to add a new module dependency is high.

Another trick that you could use is to attempt to run it, see if it is
present by checking for 127 sounds fragile though, for Windows
particularly, and otherwise skip the test.
-- 
Michael


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Thomas Munro]

On Wed, Sep 13, 2017 at 1:55 AM, Peter Eisentraut
 wrote:
> On 9/11/17 23:58, Thomas Munro wrote:
>> Sounds good.  Here it is with $username.  It's nice not to have to
>> escape any characters in URLs.  I suppose more keywords could be added
>> in follow-up patches if someone thinks that would be useful
>> ($hostname, $dbname, ...?).  I got sick of that buffer sizing code and
>> changed it to use StringInfo.  Here also are your test patches tweaked
>> slightly: 0002 just adds FreeBSD support as per previous fixup and
>> 0003 changes to $username.
>
> Committed the feature patch.

Thanks!

> Any further thoughts on the test suite?  Otherwise I'll commit it as we
> have it, for manual use.

I wonder if there is a reasonable way to indicate or determine whether
you have slapd installed so that check-world could run this test...

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


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

Re: [HACKERS] More efficient truncation of pg_stat_activity query strings

[Tom Lane]

Robert Haas  writes:
> On Tue, Sep 12, 2017 at 3:19 AM, Andres Freund  wrote:
>> Therefore I wonder if we couldn't just store a querystring that's
>> essentially just a memcpy()ed prefix, and do a pg_mbcliplen() on the
>> read side.

> Interesting idea.  I was (ha, ha, what a coincidence) also thinking
> about this problem and was wondering if we couldn't be a lot smarter
> about pg_mbcliplen().  ...

> for UTF-8, we could do that much more directly: if the string is short
> enough, stop, else, look at cmd_str[pgstat_track_activity_query_size].
> If that character has (c & 0xc0) != 0x80, write a '\0' and stop; else,
> back up until you find a character that for which that continuation
> holds, write a '\0', and stop.  This kind of approach only works if we
> have a definitive test for whether something is a "continuation
> character" which probably isn't true in all encodings, but maybe it's
> still worth considering.

Offhand I think it doesn't work in anything but UTF8.  A way that would
work in all encodings is to back up to the first non-high-bit-set
byte and then mbcliplen from there.  Probably, there's enough ASCII
in typical SQL commands that this would often be a win.

> Your idea is probably a lot simpler to implement, though, and I
> definitely agree that shifting the work from the write side to the
> read side makes sense.  Updating pg_stat_activity is a lot more common
> than reading it.

+1.  I kinda doubt that it is worth optimizing further than that,
really.

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] More efficient truncation of pg_stat_activity query strings

[Robert Haas]

On Tue, Sep 12, 2017 at 3:19 AM, Andres Freund  wrote:
> Therefore I wonder if we couldn't just store a querystring that's
> essentially just a memcpy()ed prefix, and do a pg_mbcliplen() on the
> read side.  I think that should work because all *server side* encodings
> store character lengths in the *first* byte of a multibyte character
> (at least one clientside encoding, gb18030, doesn't behave that way).
>
> That'd necessitate an added memory copy in pg_stat_get_activity(), but
> that seems fairly harmless.

Interesting idea.  I was (ha, ha, what a coincidence) also thinking
about this problem and was wondering if we couldn't be a lot smarter
about pg_mbcliplen().  I mean, pg_mbcliplen is basically just being
used here to trim away any partial character that would have to get
chopped off to fit within the length limit.  But right now it's
scanning the whole string to do this, which is unnecessary.  At least
for UTF-8, we could do that much more directly: if the string is short
enough, stop, else, look at cmd_str[pgstat_track_activity_query_size].
If that character has (c & 0xc0) != 0x80, write a '\0' and stop; else,
back up until you find a character that for which that continuation
holds, write a '\0', and stop.  This kind of approach only works if we
have a definitive test for whether something is a "continuation
character" which probably isn't true in all encodings, but maybe it's
still worth considering.

Your idea is probably a lot simpler to implement, though, and I
definitely agree that shifting the work from the write side to the
read side makes sense.  Updating pg_stat_activity is a lot more common
than reading it.

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


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Peter Eisentraut]

On 9/11/17 23:58, Thomas Munro wrote:
> Sounds good.  Here it is with $username.  It's nice not to have to
> escape any characters in URLs.  I suppose more keywords could be added
> in follow-up patches if someone thinks that would be useful
> ($hostname, $dbname, ...?).  I got sick of that buffer sizing code and
> changed it to use StringInfo.  Here also are your test patches tweaked
> slightly: 0002 just adds FreeBSD support as per previous fixup and
> 0003 changes to $username.

Committed the feature patch.

Any further thoughts on the test suite?  Otherwise I'll commit it as we
have it, for manual use.

-- 
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] More efficient truncation of pg_stat_activity query strings

[Tatsuo Ishii]

> Check the information the pg_*_mblen use / how the relevant encodings
> work. Will be something like
> int
> pg_utf_mblen(const unsigned char *s)
> {
>   int len;
> 
>   if ((*s & 0x80) == 0)
>   len = 1;
>   else if ((*s & 0xe0) == 0xc0)
>   len = 2;
>   else if ((*s & 0xf0) == 0xe0)
>   len = 3;
>   else if ((*s & 0xf8) == 0xf0)
>   len = 4;
> #ifdef NOT_USED
>   else if ((*s & 0xfc) == 0xf8)
>   len = 5;
>   else if ((*s & 0xfe) == 0xfc)
>   len = 6;
> #endif
>   else
>   len = 1;
>   return len;
> }
> 
> As you can see, only the first character (*s) is accessed to determine
> the length/width of the multibyte-character.  That's afaict the case for
> all server-side encodings.

So your idea is just storing cmd_str into st_activity, which might be
clipped in the middle of multibyte character. And the reader of the
string will call pg_mbclipen() when it needs to read the string. Yes,
I think it works except for gb18030 by the reason you said.

However, if we could have variants of mblen functions with additional
parameter: input string length, then we could remove this inconstancy.
I don't know if this is overkill or not, though.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

Re: [HACKERS] More efficient truncation of pg_stat_activity query strings

[Andres Freund]

On 2017-09-12 16:53:49 +0900, Tatsuo Ishii wrote:
> > read side.  I think that should work because all *server side* encodings
> > store character lengths in the *first* byte of a multibyte character
> 
> What do you mean? I don't see such data in a multibyte string.

Check the information the pg_*_mblen use / how the relevant encodings
work. Will be something like
int
pg_utf_mblen(const unsigned char *s)
{
int len;

if ((*s & 0x80) == 0)
len = 1;
else if ((*s & 0xe0) == 0xc0)
len = 2;
else if ((*s & 0xf0) == 0xe0)
len = 3;
else if ((*s & 0xf8) == 0xf0)
len = 4;
#ifdef NOT_USED
else if ((*s & 0xfc) == 0xf8)
len = 5;
else if ((*s & 0xfe) == 0xfc)
len = 6;
#endif
else
len = 1;
return len;
}

As you can see, only the first character (*s) is accessed to determine
the length/width of the multibyte-character.  That's afaict the case for
all server-side encodings.

Greetings,

Andres Freund


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

Re: [HACKERS] More efficient truncation of pg_stat_activity query strings

[Tatsuo Ishii]

> read side.  I think that should work because all *server side* encodings
> store character lengths in the *first* byte of a multibyte character

What do you mean? I don't see such data in a multibyte string.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

[HACKERS] More efficient truncation of pg_stat_activity query strings

[Andres Freund]

Hi,

I've recently seen a benchmark in which pg_mbcliplen() showed up
prominently. Which it will basically in any benchmark with longer query
strings, but fast queries. That's not that uncommon.

I wonder if we could avoid the cost of pg_mbcliplen() from within
pgstat_report_activity(), by moving some of the cost to the read
side. pgstat values are obviously read far less frequently in nearly all
cases that are performance relevant.

Therefore I wonder if we couldn't just store a querystring that's
essentially just a memcpy()ed prefix, and do a pg_mbcliplen() on the
read side.  I think that should work because all *server side* encodings
store character lengths in the *first* byte of a multibyte character
(at least one clientside encoding, gb18030, doesn't behave that way).

That'd necessitate an added memory copy in pg_stat_get_activity(), but
that seems fairly harmless.

Faults in my thinking?

Greetings,

Andres Freund


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Thomas Munro]

On Tue, Sep 12, 2017 at 7:21 AM, Peter Eisentraut
 wrote:
> On 9/8/17 13:24, Mark Cave-Ayland wrote:
>> My weapon of choice for LDAP deployments on POSIX-based systems is
>> Arthur De Jong's nss-pam-ldapd (https://arthurdejong.org/nss-pam-ldapd)
>> which is far more flexible than pam_ldap and fixes a large number of
>> bugs, including the tendency for pam_ldap to hang infinitely if it can't
>> contact its LDAP server.
>>
>> Take a look at nss-pam-ldapd's man page for nslcd.conf and in particular
>> pam_authz_search - this is exactly the type of filters I would end up
>> deploying onto servers. This happens a lot in large organisations
>> whereby getting group memberships updated in the main directory can take
>> days/weeks whereas someone with root access to the server itself can
>> hard-code an authentication list of users and/or groups in an LDAP
>> filter in just a few minutes.
>
> Thomas, would you consider using the placeholder syntax described at
>  under
> pam_authz_search?

Sounds good.  Here it is with $username.  It's nice not to have to
escape any characters in URLs.  I suppose more keywords could be added
in follow-up patches if someone thinks that would be useful
($hostname, $dbname, ...?).  I got sick of that buffer sizing code and
changed it to use StringInfo.  Here also are your test patches tweaked
slightly: 0002 just adds FreeBSD support as per previous fixup and
0003 changes to $username.

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


0001-Allow-custom-search-filters-to-be-configured-for-LDA.patch
Description: Binary data


0002-Add-LDAP-authentication-test-suite.patch
Description: Binary data


0003-Add-tests-for-ldapsearchfilter-functionality.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] More flexible LDAP auth search filters?

[Peter Eisentraut]

On 9/8/17 21:31, Thomas Munro wrote:
> +if ($^O eq 'darwin')
> +{
> +   $slapd = '/usr/local/opt/openldap/libexec/slapd';
> +   $ldap_schema_dir = '/usr/local/etc/openldap/schema';
> +}
> 
> I'm guessing this is the MacPorts location, and someone from that
> other tribe that uses Brew can eventually post a patch to make this
> look in more places.

Or the other way around :)

> +my $ldap_port = int(rand() * 16384) + 49152;
> 
> Hmm.  I guess ldapi (Unix domain sockets) would be less roulette-like,
> but require client side support too.

Yeah, issue similar to the SSL tests.  The above formula is what
PostgresNode uses.

> Here's a change I needed to make to run this here.  It seems that to
> use "database mdb" I'd need to add a config line to tell it the path
> to load back_mdb.so from.  I could have done, but I noticed that if I
> tell it to use raw ldif files instead it's happy.  Does this still
> work for you on the systems you tested?

Yes, that seems like a better choice.

-- 
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] More flexible LDAP auth search filters?

[Peter Eisentraut]

On 9/8/17 13:24, Mark Cave-Ayland wrote:
> My weapon of choice for LDAP deployments on POSIX-based systems is
> Arthur De Jong's nss-pam-ldapd (https://arthurdejong.org/nss-pam-ldapd)
> which is far more flexible than pam_ldap and fixes a large number of
> bugs, including the tendency for pam_ldap to hang infinitely if it can't
> contact its LDAP server.
> 
> Take a look at nss-pam-ldapd's man page for nslcd.conf and in particular
> pam_authz_search - this is exactly the type of filters I would end up
> deploying onto servers. This happens a lot in large organisations
> whereby getting group memberships updated in the main directory can take
> days/weeks whereas someone with root access to the server itself can
> hard-code an authentication list of users and/or groups in an LDAP
> filter in just a few minutes.

Thomas, would you consider using the placeholder syntax described at
 under
pam_authz_search?

-- 
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] More flexible LDAP auth search filters?

[Thomas Munro]

On Sat, Sep 9, 2017 at 3:33 AM, Peter Eisentraut
 wrote:
> A couple of comments on this patch.  I have attached a "fixup" patch on
> top of your v4 that should address them.
>
> - I think the bracketing of the LDAP URL synopsis is wrong.

+1

> - I have dropped the sentence that LDAP URL extensions are not
> supported.  That sentence was written mainly to point out that filters
> are not supported, which they are now.  There is nothing beyond filters
> and extensions left in LDAP URLs, AFAIK.

+1

> - We have previously used the ldapsearchattribute a bit strangely.  We
> use it as both the search filter and as the attribute to return from the
> search, but we don't actually care about the returned attribute (we only
> use the DN (which is not a real attribute)).  That hasn't been a real
> problem, but now if you specify a filter, as the code stands it will
> always request the "uid" attribute, which won't work if there is no such
> attribute.  I have found that there is an official way to request "no
> attribute", so I have fixed the code to do that.

Ah.  Good.

> - I was wondering whether we need a way to escape the % (%%?) but it
> doesn't seem worth bothering.
>
> I have been looking around the Internet how this functionality compares
> to other LDAP authentication systems.
>
> I didn't see the origin of the %u idea in this thread, but perhaps it
> came from Dovecot.  But there it's a general configuration file syntax,
> nothing specific to LDAP.  In nginx you use %(username), which again is
> general configuration file syntax.  I'm OK with using %u.
>
> The original LDAP URL design was adapted from Apache
> (https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
>  They combine the attribute filter and the general filter if both are
> specified, but I think they got that a bit wrong.  The attribute field
> in the URL is actually not supposed to be a filter but a return field,
> which is also the confusion mentioned above.
>
> The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
> specify a search attribute and a general filter and combines them.
>
> Neither of these allows substituting the user name into the search filter.
>
> I think there would be a case to be made to allow the searchattribute
> and the searchfilter both be specified.  One typical use would be to use
> the first one to locate the user and the second one to "filter" out
> certain things, which I think is the approach the PAM and Apache modules
> take.  This wouldn't work well, however, if you use the %u placeholder,
> because then you'd have to explicitly unset ldapsearchattribute, which
> would be annoying.  So maybe not.

I like this way, because it doesn't leave the user wondering how the
filter is constructed.  It's either the user's filter using %u
placeholders or a system-built one, but not a combination where you
have to wonder whether it's an implicit AND, OR or something else...

> Please check my patch.  I think it's ready to go then.

+1 from me to all your changes except this one:

- buffer_size += user_name_size;
+ buffer_size += user_name_size - 2;

The algorithm is: start with buffer_size = 0; add user_name_size if
you see "%u" but otherwise just add one per character; finally add one
for the terminator.  There is no reason to subtract 2, since we didn't
count the "%u" characters.  Consider a username of "x" and a search
filter of "%u": your change would underflow buffer_size.

Here's a patch with your fixup squashed (except for the above-noted thing).

Thanks for all your work on this.

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


ldap-search-filters-v5.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] More flexible LDAP auth search filters?

[Thomas Munro]

On Sat, Sep 9, 2017 at 3:36 AM, Peter Eisentraut
 wrote:
> For additional entertainment I have written a test suite for this LDAP
> authentication functionality.  It's not quite robust enough to be run by
> default, because it needs a full OpenLDAP installation, but it's been
> very helpful for reviewing this patch.  Here it is.

Very nice!

+if ($^O eq 'darwin')
+{
+   $slapd = '/usr/local/opt/openldap/libexec/slapd';
+   $ldap_schema_dir = '/usr/local/etc/openldap/schema';
+}

I'm guessing this is the MacPorts location, and someone from that
other tribe that uses Brew can eventually post a patch to make this
look in more places.

+my $ldap_port = int(rand() * 16384) + 49152;

Hmm.  I guess ldapi (Unix domain sockets) would be less roulette-like,
but require client side support too.

Here's a change I needed to make to run this here.  It seems that to
use "database mdb" I'd need to add a config line to tell it the path
to load back_mdb.so from.  I could have done, but I noticed that if I
tell it to use raw ldif files instead it's happy.  Does this still
work for you on the systems you tested?

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


0001-fixup-Add-LDAP-authentication-test-suite.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] More flexible LDAP auth search filters?

[Mark Cave-Ayland]

On 08/09/17 16:33, Peter Eisentraut wrote:

> A couple of comments on this patch.  I have attached a "fixup" patch on
> top of your v4 that should address them.
> 
> - I think the bracketing of the LDAP URL synopsis is wrong.
> 
> - I have dropped the sentence that LDAP URL extensions are not
> supported.  That sentence was written mainly to point out that filters
> are not supported, which they are now.  There is nothing beyond filters
> and extensions left in LDAP URLs, AFAIK.
> 
> - We have previously used the ldapsearchattribute a bit strangely.  We
> use it as both the search filter and as the attribute to return from the
> search, but we don't actually care about the returned attribute (we only
> use the DN (which is not a real attribute)).  That hasn't been a real
> problem, but now if you specify a filter, as the code stands it will
> always request the "uid" attribute, which won't work if there is no such
> attribute.  I have found that there is an official way to request "no
> attribute", so I have fixed the code to do that.
> 
> - I was wondering whether we need a way to escape the % (%%?) but it
> doesn't seem worth bothering.
> 
> I have been looking around the Internet how this functionality compares
> to other LDAP authentication systems.
> 
> I didn't see the origin of the %u idea in this thread, but perhaps it
> came from Dovecot.  But there it's a general configuration file syntax,
> nothing specific to LDAP.  In nginx you use %(username), which again is
> general configuration file syntax.  I'm OK with using %u.
> 
> The original LDAP URL design was adapted from Apache
> (https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
>  They combine the attribute filter and the general filter if both are
> specified, but I think they got that a bit wrong.  The attribute field
> in the URL is actually not supposed to be a filter but a return field,
> which is also the confusion mentioned above.
> 
> The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
> specify a search attribute and a general filter and combines them.
> 
> Neither of these allows substituting the user name into the search filter.

Great work! Having installed quite a few LDAP-based authentication
setups in the past, I can promise you that pam_ldap is the last tool I
would consider for the job so please don't hold to this as being the
gold standard(!).

My weapon of choice for LDAP deployments on POSIX-based systems is
Arthur De Jong's nss-pam-ldapd (https://arthurdejong.org/nss-pam-ldapd)
which is far more flexible than pam_ldap and fixes a large number of
bugs, including the tendency for pam_ldap to hang infinitely if it can't
contact its LDAP server.

Take a look at nss-pam-ldapd's man page for nslcd.conf and in particular
pam_authz_search - this is exactly the type of filters I would end up
deploying onto servers. This happens a lot in large organisations
whereby getting group memberships updated in the main directory can take
days/weeks whereas someone with root access to the server itself can
hard-code an authentication list of users and/or groups in an LDAP
filter in just a few minutes.


ATB,

Mark.


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Peter Eisentraut]

For additional entertainment I have written a test suite for this LDAP
authentication functionality.  It's not quite robust enough to be run by
default, because it needs a full OpenLDAP installation, but it's been
very helpful for reviewing this patch.  Here it is.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 9457ef272d011dbb34b1a204cac9cbac08e4d652 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 8 Sep 2017 10:33:49 -0400
Subject: [PATCH 1/3] Add LDAP authentication test suite

---
 src/test/Makefile   |   2 +-
 src/test/ldap/.gitignore|   2 +
 src/test/ldap/Makefile  |  20 +++
 src/test/ldap/README|  16 +
 src/test/ldap/data.ldif |  19 ++
 src/test/ldap/t/001_auth.pl | 139 
 6 files changed, 197 insertions(+), 1 deletion(-)
 create mode 100644 src/test/ldap/.gitignore
 create mode 100644 src/test/ldap/Makefile
 create mode 100644 src/test/ldap/README
 create mode 100644 src/test/ldap/data.ldif
 create mode 100644 src/test/ldap/t/001_auth.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index dbfa799a84..193b977bf3 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -17,7 +17,7 @@ SUBDIRS = perl regress isolation modules authentication 
recovery subscription
 # We don't build or execute examples/, locale/, or thread/ by default,
 # but we do want "make clean" etc to recurse into them.  Likewise for ssl/,
 # because the SSL test suite is not secure to run on a multi-user system.
-ALWAYS_SUBDIRS = examples locale thread ssl
+ALWAYS_SUBDIRS = examples ldap locale thread ssl
 
 # We want to recurse to all subdirs for all standard targets, except that
 # installcheck and install should not recurse into the subdirectory "modules".
diff --git a/src/test/ldap/.gitignore b/src/test/ldap/.gitignore
new file mode 100644
index 00..871e943d50
--- /dev/null
+++ b/src/test/ldap/.gitignore
@@ -0,0 +1,2 @@
+# Generated by test suite
+/tmp_check/
diff --git a/src/test/ldap/Makefile b/src/test/ldap/Makefile
new file mode 100644
index 00..9dd1bbeade
--- /dev/null
+++ b/src/test/ldap/Makefile
@@ -0,0 +1,20 @@
+#-
+#
+# Makefile for src/test/ldap
+#
+# Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
+# Portions Copyright (c) 1994, Regents of the University of California
+#
+# src/test/ldap/Makefile
+#
+#-
+
+subdir = src/test/ldap
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+check:
+   $(prove_check)
+
+clean distclean maintainer-clean:
+   rm -rf tmp_check
diff --git a/src/test/ldap/README b/src/test/ldap/README
new file mode 100644
index 00..20a7a0b5de
--- /dev/null
+++ b/src/test/ldap/README
@@ -0,0 +1,16 @@
+src/test/ldap/README
+
+Tests for LDAP functionality
+
+
+This directory contains a test suite for LDAP functionality.  This
+requires a full OpenLDAP installation, including server and client
+tools, and is therefore kept separate and not run by default.
+
+
+Running the tests
+=
+
+make check
+
+NOTE: This requires the --enable-tap-tests argument to configure.
diff --git a/src/test/ldap/data.ldif b/src/test/ldap/data.ldif
new file mode 100644
index 00..b30604e1f0
--- /dev/null
+++ b/src/test/ldap/data.ldif
@@ -0,0 +1,19 @@
+dn: dc=example,dc=net
+objectClass: top
+objectClass: dcObject
+objectClass: organization
+dc: example
+o: ExmapleCo
+
+dn: uid=test1,dc=example,dc=net
+objectClass: inetOrgPerson
+objectClass: posixAccount
+uid: test1
+sn: Lastname
+givenName: Firstname
+cn: First Test User
+displayName: First Test User
+uidNumber: 101
+gidNumber: 100
+homeDirectory: /home/test1
+mail: te...@example.net
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
new file mode 100644
index 00..af9e34d7cf
--- /dev/null
+++ b/src/test/ldap/t/001_auth.pl
@@ -0,0 +1,139 @@
+use strict;
+use warnings;
+use TestLib;
+use PostgresNode;
+use Test::More tests => 9;
+
+my ($slapd, $ldap_bin_dir, $ldap_schema_dir);
+
+$ldap_bin_dir = undef; # usually in PATH
+
+if ($^O eq 'darwin')
+{
+   $slapd = '/usr/local/opt/openldap/libexec/slapd';
+   $ldap_schema_dir = '/usr/local/etc/openldap/schema';
+}
+elsif ($^O eq 'linux')
+{
+   $slapd = '/usr/sbin/slapd';
+   $ldap_schema_dir = '/etc/ldap/schema' if -f '/etc/ldap/schema';
+   $ldap_schema_dir = '/etc/openldap/schema' if -f '/etc/openldap/schema';
+}
+
+# make your own edits here
+#$slapd = '';
+#$ldap_bin_dir = '';
+#$ldap_schema_dir = '';
+
+$ENV{PATH} = "$ldap_bin_dir:$ENV{PATH}" if $ldap_bin_dir;
+
+my $ldap_datadir = "${TestLib::tmp_check}/openldap-data";
+my $slapd_conf = "${TestLib::tmp_check}/slapd.conf";
+my $slapd_pidfile 

Re: [HACKERS] More flexible LDAP auth search filters?

[Peter Eisentraut]

A couple of comments on this patch.  I have attached a "fixup" patch on
top of your v4 that should address them.

- I think the bracketing of the LDAP URL synopsis is wrong.

- I have dropped the sentence that LDAP URL extensions are not
supported.  That sentence was written mainly to point out that filters
are not supported, which they are now.  There is nothing beyond filters
and extensions left in LDAP URLs, AFAIK.

- We have previously used the ldapsearchattribute a bit strangely.  We
use it as both the search filter and as the attribute to return from the
search, but we don't actually care about the returned attribute (we only
use the DN (which is not a real attribute)).  That hasn't been a real
problem, but now if you specify a filter, as the code stands it will
always request the "uid" attribute, which won't work if there is no such
attribute.  I have found that there is an official way to request "no
attribute", so I have fixed the code to do that.

- I was wondering whether we need a way to escape the % (%%?) but it
doesn't seem worth bothering.

I have been looking around the Internet how this functionality compares
to other LDAP authentication systems.

I didn't see the origin of the %u idea in this thread, but perhaps it
came from Dovecot.  But there it's a general configuration file syntax,
nothing specific to LDAP.  In nginx you use %(username), which again is
general configuration file syntax.  I'm OK with using %u.

The original LDAP URL design was adapted from Apache
(https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
 They combine the attribute filter and the general filter if both are
specified, but I think they got that a bit wrong.  The attribute field
in the URL is actually not supposed to be a filter but a return field,
which is also the confusion mentioned above.

The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
specify a search attribute and a general filter and combines them.

Neither of these allows substituting the user name into the search filter.

I think there would be a case to be made to allow the searchattribute
and the searchfilter both be specified.  One typical use would be to use
the first one to locate the user and the second one to "filter" out
certain things, which I think is the approach the PAM and Apache modules
take.  This wouldn't work well, however, if you use the %u placeholder,
because then you'd have to explicitly unset ldapsearchattribute, which
would be annoying.  So maybe not.

Please check my patch.  I think it's ready to go then.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From cd5536fa70eed74f8b1aa4f856edae8b84d76ef5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 8 Sep 2017 10:58:53 -0400
Subject: [PATCH] fixup! Allow custom search filters to be configured for LDAP
 auth.

---
 doc/src/sgml/client-auth.sgml |  4 +---
 src/backend/libpq/auth.c  | 18 --
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 1773ce29a9..1c3db96134 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -1525,7 +1525,7 @@ LDAP Authentication
  An RFC 4516 LDAP URL.  This is an alternative way to write some of the
  other LDAP options in a more compact and standard form.  The format is
 
-ldap://host[:port]/basedn[[[?[attribute]][?scope]][?filter]]
+ldap://host[:port]/basedn[?[attribute][?[scope][?[filter
 
  scope must be one
  of base, one, 
sub,
@@ -1537,8 +1537,6 @@ LDAP Authentication
  ldapsearchfilter.  When specifying a search filter
  as part of a URL, the special token %u standing
  for the user name must be written as %25u.
- Some other components of standard LDAP URLs such as extensions are
- not supported.
 
 
 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 6c96e87d37..c43f203eb3 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2401,8 +2401,8 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
 static char *
 FormatSearchFilter(const char *pattern, const char *user_name)
 {
-   Size user_name_size = strlen(user_name);
-   Size buffer_size = 0;
+   size_t user_name_size = strlen(user_name);
+   size_t buffer_size = 0;
const char *in;
char *out;
char *result;
@@ -2413,7 +2413,7 @@ FormatSearchFilter(const char *pattern, const char 
*user_name)
{
if (in[0] == '%' && in[1] == 'u')
{
-   buffer_size += user_name_size;
+   buffer_size += user_name_size - 2;
in += 2;
}
else
@@ -2486,7 +2486,7 @@ CheckLDAPAuth(Port *port)
char   *filter;

Re: [HACKERS] More replication race conditions

[Michael Paquier]

On Sat, Sep 2, 2017 at 12:03 AM, Alvaro Herrera  wrote:
> Michael Paquier wrote:
>> On Mon, Aug 28, 2017 at 8:25 AM, Michael Paquier
>>  wrote:
>> > Today's run has finished with the same failure:
>> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-27%2018%3A00%3A13
>> > Attached is a patch to make this code path wait that the transaction
>> > has been replayed. We could use as well synchronous_commit = apply,
>> > but I prefer the solution of this patch with a wait query.
>>
>> I have added an open item for this issue for PG10. The 2PC tests in
>> src/test/recovery are new in PG10, introduced by 3082098.
>
> Thanks, pushed.

Thanks, the error has now not happened for 5 runs. Let's see if it
ever shows up again.
-- 
Michael


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

Re: [HACKERS] More replication race conditions

[Alvaro Herrera]

Michael Paquier wrote:
> On Mon, Aug 28, 2017 at 8:25 AM, Michael Paquier
>  wrote:
> > Today's run has finished with the same failure:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-27%2018%3A00%3A13
> > Attached is a patch to make this code path wait that the transaction
> > has been replayed. We could use as well synchronous_commit = apply,
> > but I prefer the solution of this patch with a wait query.
> 
> I have added an open item for this issue for PG10. The 2PC tests in
> src/test/recovery are new in PG10, introduced by 3082098.

Thanks, pushed.

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


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

Re: [HACKERS] More replication race conditions

[Simon Riggs]

On 31 August 2017 at 12:54, Simon Riggs  wrote:

>> The above-described topic is currently a PostgreSQL 10 open item.  Simon,
>> since you committed the patch believed to have created it, you own this open
>> item.  If some other commit is more relevant or if this does not belong as a
>> v10 open item, please let us know.  Otherwise, please observe the policy on
>> open item ownership[1] and send a status update within three calendar days of
>> this message.  Include a date for your subsequent status update.  Testers may
>> discover new open items at any time, and I want to plan to get them all fixed
>> well in advance of shipping v10.  Consequently, I will appreciate your 
>> efforts
>> toward speedy resolution.  Thanks.
>>
>> [1] 
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>
> I acknowledge receipt of the reminder and will fix by end of day tomorrow.

Applied. Thanks for the patch, Petr.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] More replication race conditions

[Simon Riggs]

On 27 August 2017 at 03:32, Noah Misch  wrote:
> On Fri, Aug 25, 2017 at 12:09:00PM +0200, Petr Jelinek wrote:
>> On 24/08/17 19:54, Tom Lane wrote:
>> > sungazer just failed with
>> >
>> > pg_recvlogical exited with code '256', stdout '' and stderr 
>> > 'pg_recvlogical: could not send replication command "START_REPLICATION 
>> > SLOT "test_slot" LOGICAL 0/0 ("include-xids" '0', "skip-empty-xacts" 
>> > '1')": ERROR:  replication slot "test_slot" is active for PID 8913148
>> > pg_recvlogical: disconnected
>> > ' at 
>> > /home/nm/farm/gcc64/HEAD/pgsql.build/src/test/recovery/../../../src/test/perl/PostgresNode.pm
>> >  line 1657.
>> >
>> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer=2017-08-24%2015%3A16%3A10
>> >
>> > Looks like we're still not there on preventing replication startup
>> > race conditions.
>>
>> Hmm, that looks like "by design" behavior. Slot acquiring will throw
>> error if the slot is already used by somebody else (slots use their own
>> locking mechanism that does not wait). In this case it seems the
>> walsender which was using slot for previous previous step didn't finish
>> releasing the slot by the time we start new command. We can work around
>> this by changing the test to wait perhaps.
>
> [Action required within three days.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 10 open item.  Simon,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
>
> [1] 
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

I acknowledge receipt of the reminder and will fix by end of day tomorrow.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] More replication race conditions

[Noah Misch]

On Tue, Aug 29, 2017 at 08:44:42PM +0900, Michael Paquier wrote:
> On Mon, Aug 28, 2017 at 8:25 AM, Michael Paquier
>  wrote:
> > Today's run has finished with the same failure:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-27%2018%3A00%3A13
> > Attached is a patch to make this code path wait that the transaction
> > has been replayed. We could use as well synchronous_commit = apply,
> > but I prefer the solution of this patch with a wait query.
> 
> I have added an open item for this issue for PG10. The 2PC tests in
> src/test/recovery are new in PG10, introduced by 3082098.

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

The above-described topic is currently a PostgreSQL 10 open item.  Álvaro,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

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


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

Re: [HACKERS] More replication race conditions

[Noah Misch]

On Sun, Aug 27, 2017 at 02:32:49AM +, Noah Misch wrote:
> On Fri, Aug 25, 2017 at 12:09:00PM +0200, Petr Jelinek wrote:
> > On 24/08/17 19:54, Tom Lane wrote:
> > > sungazer just failed with
> > > 
> > > pg_recvlogical exited with code '256', stdout '' and stderr 
> > > 'pg_recvlogical: could not send replication command "START_REPLICATION 
> > > SLOT "test_slot" LOGICAL 0/0 ("include-xids" '0', "skip-empty-xacts" 
> > > '1')": ERROR:  replication slot "test_slot" is active for PID 8913148
> > > pg_recvlogical: disconnected
> > > ' at 
> > > /home/nm/farm/gcc64/HEAD/pgsql.build/src/test/recovery/../../../src/test/perl/PostgresNode.pm
> > >  line 1657.
> > > 
> > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer=2017-08-24%2015%3A16%3A10
> > > 
> > > Looks like we're still not there on preventing replication startup
> > > race conditions.
> > 
> > Hmm, that looks like "by design" behavior. Slot acquiring will throw
> > error if the slot is already used by somebody else (slots use their own
> > locking mechanism that does not wait). In this case it seems the
> > walsender which was using slot for previous previous step didn't finish
> > releasing the slot by the time we start new command. We can work around
> > this by changing the test to wait perhaps.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Simon,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
> 
> [1] 
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] More replication race conditions

[Michael Paquier]

On Mon, Aug 28, 2017 at 8:25 AM, Michael Paquier
 wrote:
> Today's run has finished with the same failure:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-27%2018%3A00%3A13
> Attached is a patch to make this code path wait that the transaction
> has been replayed. We could use as well synchronous_commit = apply,
> but I prefer the solution of this patch with a wait query.

I have added an open item for this issue for PG10. The 2PC tests in
src/test/recovery are new in PG10, introduced by 3082098.
-- 
Michael


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

Re: [HACKERS] More replication race conditions

[Petr Jelinek]

On 28/08/17 01:36, Michael Paquier wrote:
> On Sun, Aug 27, 2017 at 6:32 PM, Petr Jelinek
>  wrote:
>> Attached should fix this.
> 
> +$node_master->poll_query_until('postgres',
> +"SELECT EXISTS (SELECT 1 FROM pg_replication_slots WHERE slot_name =
> 'test_slot' AND active_pid IS NULL)"
> +)
> +  or die "slot never became inactive";
> +
>  $stdout_recv = $node_master->pg_recvlogical_upto(
> I am wondering if a similar check should actually go into
> pg_recvlogical_upto() instead. I tend to think that we've learned
> things the hard way with 3043c1dd and such.
> 

Putting it there can lead to tests that take forever to finish if
written incorrectly. This way they at least just fail.

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


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

Re: [HACKERS] More replication race conditions

[Michael Paquier]

On Mon, Aug 28, 2017 at 8:33 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> Attached is a patch to make this code path wait that the transaction
>> has been replayed. We could use as well synchronous_commit = apply,
>> but I prefer the solution of this patch with a wait query.
>
> Petr proposed a different patch to fix the same problem at
> https://www.postgresql.org/message-id/1636c52e-c144-993a-6665-9358f322deda%402ndquadrant.com
>
> Which one is better?

Petr's patch is here to fix the race condition in test 006 for logical
decoding. My patch involves the failures that dangomushi sees since
yesterday with 2PC test 009. (Note: It took me a while to put this
animal in an environment with a more stable connection, so now I hope
that we'll get again daily reports)
-- 
Michael


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

Re: [HACKERS] More replication race conditions

[Michael Paquier]

On Sun, Aug 27, 2017 at 6:32 PM, Petr Jelinek
 wrote:
> Attached should fix this.

+$node_master->poll_query_until('postgres',
+"SELECT EXISTS (SELECT 1 FROM pg_replication_slots WHERE slot_name =
'test_slot' AND active_pid IS NULL)"
+)
+  or die "slot never became inactive";
+
 $stdout_recv = $node_master->pg_recvlogical_upto(
I am wondering if a similar check should actually go into
pg_recvlogical_upto() instead. I tend to think that we've learned
things the hard way with 3043c1dd and such.
-- 
Michael


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

Re: [HACKERS] More replication race conditions

[Tom Lane]

Michael Paquier  writes:
> Attached is a patch to make this code path wait that the transaction
> has been replayed. We could use as well synchronous_commit = apply,
> but I prefer the solution of this patch with a wait query.

Petr proposed a different patch to fix the same problem at
https://www.postgresql.org/message-id/1636c52e-c144-993a-6665-9358f322deda%402ndquadrant.com

Which one is better?

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] More replication race conditions

[Michael Paquier]

On Sun, Aug 27, 2017 at 3:34 PM, Michael Paquier
 wrote:
> On Sun, Aug 27, 2017 at 12:03 PM, Tom Lane  wrote:
>> contains exactly no means of ensuring that the master's transaction has
>> been replayed on the standby before we check for its results.  It's not
>> real clear why it seems to work 99.99% of the time, because, well, there
>> isn't any frickin' interlock there ...
>
> I have noticed this one this morning, and I am planning to address it
> with a proper patch soonishly. (I am still fighting a bit to get
> dangomushi in a more stable stable, and things run slow on it, so it
> is good at catching race conditions of this kind).

Today's run has finished with the same failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-27%2018%3A00%3A13
Attached is a patch to make this code path wait that the transaction
has been replayed. We could use as well synchronous_commit = apply,
but I prefer the solution of this patch with a wait query.
-- 
Michael


tap-2pc-fix.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] More replication race conditions

[Petr Jelinek]

On 27/08/17 04:32, Noah Misch wrote:
> On Fri, Aug 25, 2017 at 12:09:00PM +0200, Petr Jelinek wrote:
>> On 24/08/17 19:54, Tom Lane wrote:
>>> sungazer just failed with
>>>
>>> pg_recvlogical exited with code '256', stdout '' and stderr 
>>> 'pg_recvlogical: could not send replication command "START_REPLICATION SLOT 
>>> "test_slot" LOGICAL 0/0 ("include-xids" '0', "skip-empty-xacts" '1')": 
>>> ERROR:  replication slot "test_slot" is active for PID 8913148
>>> pg_recvlogical: disconnected
>>> ' at 
>>> /home/nm/farm/gcc64/HEAD/pgsql.build/src/test/recovery/../../../src/test/perl/PostgresNode.pm
>>>  line 1657.
>>>
>>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer=2017-08-24%2015%3A16%3A10
>>>
>>> Looks like we're still not there on preventing replication startup
>>> race conditions.
>>
>> Hmm, that looks like "by design" behavior. Slot acquiring will throw
>> error if the slot is already used by somebody else (slots use their own
>> locking mechanism that does not wait). In this case it seems the
>> walsender which was using slot for previous previous step didn't finish
>> releasing the slot by the time we start new command. We can work around
>> this by changing the test to wait perhaps.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Simon,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
> 
> [1] 
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 

Attached should fix this.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
>From d66caa3532558828ecb89b4f153cc1f29332b918 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Sun, 27 Aug 2017 11:28:49 +0200
Subject: [PATCH] Fix race condition in logical decoding tap test

---
 src/test/recovery/t/006_logical_decoding.pl | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/test/recovery/t/006_logical_decoding.pl b/src/test/recovery/t/006_logical_decoding.pl
index 4a90e9a..8b35bc8 100644
--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
@@ -78,6 +78,11 @@ chomp($stdout_recv);
 is($stdout_recv, $expected,
 	'got same expected output from pg_recvlogical decoding session');
 
+$node_master->poll_query_until('postgres',
+"SELECT EXISTS (SELECT 1 FROM pg_replication_slots WHERE slot_name = 'test_slot' AND active_pid IS NULL)"
+)
+  or die "slot never became inactive";
+
 $stdout_recv = $node_master->pg_recvlogical_upto(
 	'postgres', 'test_slot', $endpos, 10,
 	'include-xids' => '0',
-- 
2.7.4


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

Re: [HACKERS] More replication race conditions

[Michael Paquier]

On Sun, Aug 27, 2017 at 12:03 PM, Tom Lane  wrote:
> And *another* replication test race condition just now:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-26%2019%3A37%3A08
>
> As best I can interpret this, it's pointing out that this bit in
> src/test/recovery/t/009_twophase.pl:
>
> $cur_master->psql(
> 'postgres', "
> BEGIN;
> CREATE TABLE t_009_tbl2 (id int, msg text);
> SAVEPOINT s1;
> INSERT INTO t_009_tbl2 VALUES (27, 'issued to ${cur_master_name}');
> PREPARE TRANSACTION 'xact_009_13';
> -- checkpoint will issue XLOG_STANDBY_LOCK that can conflict with lock
> -- held by 'create table' statement
> CHECKPOINT;
> COMMIT PREPARED 'xact_009_13';");
>
> $cur_standby->psql(
> 'postgres',
> "SELECT count(*) FROM t_009_tbl2",
> stdout => \$psql_out);
> is($psql_out, '1', "Replay prepared transaction with DDL");
>
> contains exactly no means of ensuring that the master's transaction has
> been replayed on the standby before we check for its results.  It's not
> real clear why it seems to work 99.99% of the time, because, well, there
> isn't any frickin' interlock there ...

I have noticed this one this morning, and I am planning to address it
with a proper patch soonishly. (I am still fighting a bit to get
dangomushi in a more stable stable, and things run slow on it, so it
is good at catching race conditions of this kind).
-- 
Michael


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

Re: [HACKERS] More replication race conditions

[Tom Lane]

And *another* replication test race condition just now:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dangomushi=2017-08-26%2019%3A37%3A08

As best I can interpret this, it's pointing out that this bit in
src/test/recovery/t/009_twophase.pl:

$cur_master->psql(
'postgres', "
BEGIN;
CREATE TABLE t_009_tbl2 (id int, msg text);
SAVEPOINT s1;
INSERT INTO t_009_tbl2 VALUES (27, 'issued to ${cur_master_name}');
PREPARE TRANSACTION 'xact_009_13';
-- checkpoint will issue XLOG_STANDBY_LOCK that can conflict with lock
-- held by 'create table' statement
CHECKPOINT;
COMMIT PREPARED 'xact_009_13';");

$cur_standby->psql(
'postgres',
"SELECT count(*) FROM t_009_tbl2",
stdout => \$psql_out);
is($psql_out, '1', "Replay prepared transaction with DDL");

contains exactly no means of ensuring that the master's transaction has
been replayed on the standby before we check for its results.  It's not
real clear why it seems to work 99.99% of the time, because, well, there
isn't any frickin' interlock there ...

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] More replication race conditions

[Noah Misch]

On Fri, Aug 25, 2017 at 12:09:00PM +0200, Petr Jelinek wrote:
> On 24/08/17 19:54, Tom Lane wrote:
> > sungazer just failed with
> > 
> > pg_recvlogical exited with code '256', stdout '' and stderr 
> > 'pg_recvlogical: could not send replication command "START_REPLICATION SLOT 
> > "test_slot" LOGICAL 0/0 ("include-xids" '0', "skip-empty-xacts" '1')": 
> > ERROR:  replication slot "test_slot" is active for PID 8913148
> > pg_recvlogical: disconnected
> > ' at 
> > /home/nm/farm/gcc64/HEAD/pgsql.build/src/test/recovery/../../../src/test/perl/PostgresNode.pm
> >  line 1657.
> > 
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer=2017-08-24%2015%3A16%3A10
> > 
> > Looks like we're still not there on preventing replication startup
> > race conditions.
> 
> Hmm, that looks like "by design" behavior. Slot acquiring will throw
> error if the slot is already used by somebody else (slots use their own
> locking mechanism that does not wait). In this case it seems the
> walsender which was using slot for previous previous step didn't finish
> releasing the slot by the time we start new command. We can work around
> this by changing the test to wait perhaps.

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

The above-described topic is currently a PostgreSQL 10 open item.  Simon,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

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


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

Re: [HACKERS] More replication race conditions

[Petr Jelinek]

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

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

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


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

[HACKERS] More replication race conditions

[Tom Lane]

sungazer just failed with

pg_recvlogical exited with code '256', stdout '' and stderr 'pg_recvlogical: 
could not send replication command "START_REPLICATION SLOT "test_slot" LOGICAL 
0/0 ("include-xids" '0', "skip-empty-xacts" '1')": ERROR:  replication slot 
"test_slot" is active for PID 8913148
pg_recvlogical: disconnected
' at 
/home/nm/farm/gcc64/HEAD/pgsql.build/src/test/recovery/../../../src/test/perl/PostgresNode.pm
 line 1657.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer=2017-08-24%2015%3A16%3A10

Looks like we're still not there on preventing replication startup
race conditions.

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] More race conditions in logical replication

[Michael Paquier]

On Sun, Aug 13, 2017 at 10:25 AM, Noah Misch  wrote:
> Committed.  These counts broke three times in the v10 release cycle.  It's too
> bad this defect doesn't cause an error when building the docs.

That's another argument for generating the table dynamically. Thanks
for the commit.
-- 
Michael


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

Re: [HACKERS] More race conditions in logical replication

[Noah Misch]

On Sat, Aug 12, 2017 at 05:38:29PM +0900, Michael Paquier wrote:
> On Thu, Aug 10, 2017 at 4:22 PM, Michael Paquier
>  wrote:
> > On Tue, Aug 8, 2017 at 8:11 PM, Alvaro Herrera  
> > wrote:
> >> Here's a patch.  It turned to be a bit larger than I initially expected.
> >
> > Álvaro, 030273b7 did not get things completely right. A couple of wait
> > events have been added in the docs for sections Client, IPC and
> > Activity but it forgot to update the counters for morerows . Attached
> > is a patch to fix all that.
> 
> As the documentation format is weird because of this commit, I am
> adding an open item dedicated to that. Look for example at
> WalSenderWaitForWAL in
> https://www.postgresql.org/docs/devel/static/monitoring-stats.html.
> While looking again, I have noticed that one line for the section of
> LWLock is weird, so attached is an updated patch.

Committed.  These counts broke three times in the v10 release cycle.  It's too
bad this defect doesn't cause an error when building the docs.


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

Re: [HACKERS] More race conditions in logical replication

[Michael Paquier]

On Thu, Aug 10, 2017 at 4:22 PM, Michael Paquier
 wrote:
> On Tue, Aug 8, 2017 at 8:11 PM, Alvaro Herrera  
> wrote:
>> Here's a patch.  It turned to be a bit larger than I initially expected.
>
> Álvaro, 030273b7 did not get things completely right. A couple of wait
> events have been added in the docs for sections Client, IPC and
> Activity but it forgot to update the counters for morerows . Attached
> is a patch to fix all that.

As the documentation format is weird because of this commit, I am
adding an open item dedicated to that. Look for example at
WalSenderWaitForWAL in
https://www.postgresql.org/docs/devel/static/monitoring-stats.html.
While looking again, I have noticed that one line for the section of
LWLock is weird, so attached is an updated patch.

Thanks,
-- 
Michael
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 12d5628266..5575c2c837 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -845,7 +845,7 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
 
   

-LWLock
+LWLock
 ShmemIndexLock
 Waiting to find or allocate space in shared memory.

@@ -1155,7 +1155,7 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
  Waiting to acquire a pin on a buffer.
 
 
- Activity
+ Activity
  ArchiverMain
  Waiting in main loop of the archiver process.
 
@@ -1212,7 +1212,7 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
  Waiting in main loop of WAL writer process.
 
 
- Client
+ Client
  ClientRead
  Waiting to read data from the client.
 
@@ -1250,7 +1250,7 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
  Waiting in an extension.
 
 
- IPC
+ IPC
  BgWorkerShutdown
  Waiting for background worker to shut down.
 

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

Re: [HACKERS] More race conditions in logical replication

[Michael Paquier]

On Tue, Aug 8, 2017 at 8:11 PM, Alvaro Herrera  wrote:
> Robert Haas wrote:
>> On Mon, Aug 7, 2017 at 8:14 PM, Alvaro Herrera  
>> wrote:
>> > BTW, I noticed that the PG_WAIT_LOCK value that we're using for wait
>> > event here (and in the replication slot case) is bogus.  We probably
>> > need something new here.
>>
>> Yeah, if you're adding a new wait point, you should add document a new
>> constant in the appropriate section, probably something under
>> PG_WAIT_IPC in this case.
>
> Here's a patch.  It turned to be a bit larger than I initially expected.

Álvaro, 030273b7 did not get things completely right. A couple of wait
events have been added in the docs for sections Client, IPC and
Activity but it forgot to update the counters for morerows . Attached
is a patch to fix all that.
-- 
Michael


docfix-wait-event-rows.patch
Description: Binary data

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

[HACKERS] More fun with container types

[Tom Lane]

While poking at the arrays-of-domains TODO item, I found yet another area
in which the existing code fails to handle existing supported cases.
Specifically, although DefineRange will happily take domains or composites
as a range subtype, and we already noticed that domains-over-array-of-
composite are supported, find_composite_type_dependencies is innocent of
the idea that the target type might be embedded in anything but an array
or composite type.  The test cases in the attached patch show cases where
HEAD either fails to notice stored data violating a proposed new domain
constraint, or hits assertion failures or core dumps.

The patch addresses this by generalizing the existing special case for
array-over-composite so that any type that's directly dependent on the
target type (which could be its array type, or a domain or range over
it) is treated as a container type and recursively scanned for.

I believe this coding will work without any further changes for the
case of arrays over domains, but I've not fully tested that yet.

I intend to apply and back-patch this shortly, unless anyone has a
better idea about how to handle such cases.

regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1b8d4b3..2afde0a 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** ATTypedTableRecursion(List **wqueue, Rel
*** 4849,4861 
  /*
   * find_composite_type_dependencies
   *
!  * Check to see if a composite type is being used as a column in some
!  * other table (possibly nested several levels deep in composite types!).
   * Eventually, we'd like to propagate the check or rewrite operation
!  * into other such tables, but for now, just error out if we find any.
   *
!  * Caller should provide either a table name or a type name (not both) to
!  * report in the error message, if any.
   *
   * We assume that functions and views depending on the type are not reasons
   * to reject the ALTER.  (How safe is this really?)
--- 4849,4866 
  /*
   * find_composite_type_dependencies
   *
!  * Check to see if the type "typeOid" is being used as a column in some table
!  * (possibly nested several levels deep in composite types, arrays, etc!).
   * Eventually, we'd like to propagate the check or rewrite operation
!  * into such tables, but for now, just error out if we find any.
   *
!  * Caller should provide either the associated relation of a rowtype,
!  * or a type name (not both) for use in the error message, if any.
!  *
!  * Note that "typeOid" is not necessarily a composite type; it could also be
!  * another container type such as an array or range, or a domain over one of
!  * these things.  The name of this function is therefore somewhat historical,
!  * but it's not worth changing.
   *
   * We assume that functions and views depending on the type are not reasons
   * to reject the ALTER.  (How safe is this really?)
*** find_composite_type_dependencies(Oid typ
*** 4868,4878 
  	ScanKeyData key[2];
  	SysScanDesc depScan;
  	HeapTuple	depTup;
! 	Oid			arrayOid;
  
  	/*
! 	 * We scan pg_depend to find those things that depend on the rowtype. (We
! 	 * assume we can ignore refobjsubid for a rowtype.)
  	 */
  	depRel = heap_open(DependRelationId, AccessShareLock);
  
--- 4873,4885 
  	ScanKeyData key[2];
  	SysScanDesc depScan;
  	HeapTuple	depTup;
! 
! 	/* since this function recurses, it could be driven to stack overflow */
! 	check_stack_depth();
  
  	/*
! 	 * We scan pg_depend to find those things that depend on the given type.
! 	 * (We assume we can ignore refobjsubid for a type.)
  	 */
  	depRel = heap_open(DependRelationId, AccessShareLock);
  
*** find_composite_type_dependencies(Oid typ
*** 4894,4901 
  		Relation	rel;
  		Form_pg_attribute att;
  
! 		/* Ignore dependees that aren't user columns of relations */
! 		/* (we assume system columns are never of rowtypes) */
  		if (pg_depend->classid != RelationRelationId ||
  			pg_depend->objsubid <= 0)
  			continue;
--- 4901,4922 
  		Relation	rel;
  		Form_pg_attribute att;
  
! 		/* Check for directly dependent types */
! 		if (pg_depend->classid == TypeRelationId)
! 		{
! 			/*
! 			 * This must be an array, domain, or range containing the given
! 			 * type, so recursively check for uses of this type.  Note that
! 			 * any error message will mention the original type not the
! 			 * container; this is intentional.
! 			 */
! 			find_composite_type_dependencies(pg_depend->objid,
! 			 origRelation, origTypeName);
! 			continue;
! 		}
! 
! 		/* Else, ignore dependees that aren't user columns of relations */
! 		/* (we assume system columns are never of interesting types) */
  		if (pg_depend->classid != RelationRelationId ||
  			pg_depend->objsubid <= 0)
  			continue;
*** find_composite_type_dependencies(Oid typ
*** 4952,4965 
  	systable_endscan(depScan);
  

Re: [HACKERS] More race conditions in logical replication

[Alvaro Herrera]

Robert Haas wrote:
> On Mon, Aug 7, 2017 at 8:14 PM, Alvaro Herrera  
> wrote:
> > BTW, I noticed that the PG_WAIT_LOCK value that we're using for wait
> > event here (and in the replication slot case) is bogus.  We probably
> > need something new here.
> 
> Yeah, if you're adding a new wait point, you should add document a new
> constant in the appropriate section, probably something under
> PG_WAIT_IPC in this case.

Here's a patch.  It turned to be a bit larger than I initially expected.

Wait events are a maintainability fail IMO.  I think we need to rethink
this stuff; using generated files from a single source containing the C
symbol, text name and doc blurb sounds better.  That can wait for pg11
though.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From e050ef66956935e6dba41fc18e11632ff9f0068f Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Tue, 8 Aug 2017 13:33:47 -0400
Subject: [PATCH] Fix various inadequacies in wait events

In commit 9915de6c1cb2, we introduced a new wait point for replication
slots and incorrectly labelled it as wait event PG_WAIT_LOCK.  That's
wrong, so invent an appropriate new wait event instead, and document it
properly.

While at it, fix numerous other problems in the vicinity:
- two different walreceiver wait events were being mixed up in a single
  wait event; split it out so that they can be distinguished, and
  document the new symbols properly.

- ParallelBitmapPopulate was documented but didn't exist.

- ParallelBitmapScan was not documented

- Logical replication wait events weren't documented

- various symbols had been added in dartboard order in various places.
  Put them back in alphabetical order, as was originally intended.
---
 doc/src/sgml/monitoring.sgml   | 32 +--
 src/backend/postmaster/pgstat.c| 36 +-
 .../libpqwalreceiver/libpqwalreceiver.c|  4 +--
 src/backend/replication/slot.c |  3 +-
 src/include/pgstat.h   | 16 +-
 5 files changed, 64 insertions(+), 27 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index be3dc672bc..eb20c9c543 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1176,6 +1176,14 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
  Waiting in main loop of checkpointer process.
 
 
+ LogicalLauncherMain
+ Waiting in main loop of logical launcher process.
+
+
+ LogicalApplyMain
+ Waiting in main loop of logical apply process.
+
+
  PgStatMain
  Waiting in main loop of the statistics collector 
process.
 
@@ -1213,6 +1221,14 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
  Waiting to write data from the client.
 
 
+ LibPQWalReceiverConnect
+ Waiting in WAL receiver to establish connection to remote 
server.
+
+
+ LibPQWalReceiverReceive
+ Waiting in WAL receiver to receive data from remote 
server.
+
+
  SSLOpenServer
  Waiting for SSL while attempting connection.
 
@@ -1251,6 +1267,14 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
  Waiting for activity from child process when executing 
Gather node.
 
 
+ LogicalSyncData
+ Waiting for logical replication remote server to send data for 
initial table synchronization.
+
+
+ LogicalSyncStateChange
+ Waiting for logical replication remote server to change 
state.
+
+
  MessageQueueInternal
  Waiting for other process to be attached in shared message 
queue.
 
@@ -1271,14 +1295,18 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   
11:34   0:00 postgres: ser
  Waiting for parallel workers to finish computing.
 
 
- ParallelBitmapPopulate
- Waiting for the leader to populate the TidBitmap.
+ ParallelBitmapScan
+ Waiting for parallel bitmap scan to become initialized.
 
 
  ProcArrayGroupUpdate
  Waiting for group leader to clear transaction id at 
transaction end.
 
 
+ ReplicationSlotDrop
+ Waiting for a replication slot to become inactive to be 
dropped.
+
+
  SafeSnapshot
  Waiting for a snapshot for a READ ONLY DEFERRABLE 
transaction.
 
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index a0b0eecbd5..3f5fb796a5 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ 

Re: [HACKERS] More race conditions in logical replication

[Robert Haas]

On Mon, Aug 7, 2017 at 8:14 PM, Alvaro Herrera  wrote:
> BTW, I noticed that the PG_WAIT_LOCK value that we're using for wait
> event here (and in the replication slot case) is bogus.  We probably
> need something new here.

Yeah, if you're adding a new wait point, you should add document a new
constant in the appropriate section, probably something under
PG_WAIT_IPC in this case.

-- 
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] More race conditions in logical replication

[Alvaro Herrera]

Alvaro Herrera wrote:
> Alvaro Herrera wrote:

> > I just noticed that Jacana failed again in the subscription test, and it
> > looks like it's related to this.  I'll take a look tomorrow if no one
> > beats me to it.
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2017-07-26%2014%3A39%3A54
> 
> Ahh, I misread the message.  This one is actually about the replication
> *origin* being still active, not the replication *slot*.  I suppose
> origins need the same treatment as we just did for slots.  Anybody wants
> to volunteer a patch?

So here's a patch.  I was able to reproduce the issue locally by adding
a couple of sleeps, just like Tom did in the case of replication slots.
With this patch it seems the problem is solved.  The mechanism is the
same as Petr used to fix replication origins -- if an origin is busy,
sleep on the CV until someone else signals us; and each time the
acquirer PID changes, broadcast a signal.  Like before, this is likely
to be a problem in older branches too, but we have no CVs to backpatch
this.

BTW, I noticed that the PG_WAIT_LOCK value that we're using for wait
event here (and in the replication slot case) is bogus.  We probably
need something new here.

I found four instances of this problem in buildfarm,
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2017-07-26%2014%3A39%3A54
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2017-07-28%2021%3A00%3A15
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2017-07-31%2007%3A03%3A20
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=nightjar=2017-08-04%2004%3A34%3A04

Jacana only stopped having it because it broke for unrelated reasons.  I
bet we'll see another failure soon ...

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/commands/subscriptioncmds.c 
b/src/backend/commands/subscriptioncmds.c
index 3593712791..ae40f7164d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -939,7 +939,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool 
isTopLevel)
snprintf(originname, sizeof(originname), "pg_%u", subid);
originid = replorigin_by_name(originname, true);
if (originid != InvalidRepOriginId)
-   replorigin_drop(originid);
+   replorigin_drop(originid, false);
 
/*
 * If there is no slot associated with the subscription, we can finish
diff --git a/src/backend/replication/logical/origin.c 
b/src/backend/replication/logical/origin.c
index 1c665312a4..4f32e7861c 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -79,15 +79,15 @@
 #include "access/xact.h"
 
 #include "catalog/indexing.h"
-
 #include "nodes/execnodes.h"
 
 #include "replication/origin.h"
 #include "replication/logical.h"
-
+#include "pgstat.h"
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "storage/lmgr.h"
+#include "storage/condition_variable.h"
 #include "storage/copydir.h"
 
 #include "utils/builtins.h"
@@ -125,6 +125,11 @@ typedef struct ReplicationState
int acquired_by;
 
/*
+* Condition variable that's signalled when acquired_by changes.
+*/
+   ConditionVariable origin_cv;
+
+   /*
 * Lock protecting remote_lsn and local_lsn.
 */
LWLock  lock;
@@ -324,9 +329,9 @@ replorigin_create(char *roname)
  * Needs to be called in a transaction.
  */
 void
-replorigin_drop(RepOriginId roident)
+replorigin_drop(RepOriginId roident, bool nowait)
 {
-   HeapTuple   tuple = NULL;
+   HeapTuple   tuple;
Relationrel;
int i;
 
@@ -334,6 +339,8 @@ replorigin_drop(RepOriginId roident)
 
rel = heap_open(ReplicationOriginRelationId, ExclusiveLock);
 
+restart:
+   tuple = NULL;
/* cleanup the slot state info */
LWLockAcquire(ReplicationOriginLock, LW_EXCLUSIVE);
 
@@ -346,11 +353,21 @@ replorigin_drop(RepOriginId roident)
{
if (state->acquired_by != 0)
{
-   ereport(ERROR,
-   (errcode(ERRCODE_OBJECT_IN_USE),
-errmsg("could not drop 
replication origin with OID %d, in use by PID %d",
-   state->roident,
-   
state->acquired_by)));
+   ConditionVariable  *cv;
+
+   if (nowait)
+   ereport(ERROR,
+   
(errcode(ERRCODE_OBJECT_IN_USE),
+errmsg("could not drop 

Re: [HACKERS] More flexible LDAP auth search filters?

[Mark Cave-Ayland]

On 01/08/17 23:17, Thomas Munro wrote:

> On Wed, Aug 2, 2017 at 5:36 AM, Peter Eisentraut
>  wrote:
>> On 7/16/17 19:09, Thomas Munro wrote:
>>> On Mon, Jul 17, 2017 at 10:26 AM, Thomas Munro
>>>  wrote:
 ldap-search-filters-v2.patch
>>>
>>> Gah, it would help if I could spell "occurrences" correctly.  Fixed in
>>> the attached.
>>
>> Please also add the corresponding support for specifying search filters
>> in LDAP URLs.  See RFC 4516 for the format and
>> https://linux.die.net/man/3/ldap_url_parse for the API.  You might just
>> need to grab LDAPURLDesc.lud_filter and use it.
> 
> Good idea.  Yes, it seems to be that simple.  Here's a version like
> that.  Here's an example of how it looks in pg_hba.conf:
> 
> host   all all  127.0.0.1/32ldap
> ldapurl="ldap://localhost/ou=people1,dc=my-domain,dc=com??sub?(cn=%25u)"
> 
> Maybe we could choose a better token than %u for user name, since it
> has to be escaped when included in a URL like that, but on the other
> hand there seems to be wide precedent for %u in other software.

Yeah, mostly I only ever see ldapurls used programatically, i.e. the
configuration allows you to set the various fields separately and then
the software generates the URL with the correct encoding itself. But if
it's documented that's not a reason to reject the patch as I definitely
see it as an improvement.

As I mentioned previously in the thread, the main barrier preventing
people from using LDAP is that the role cannot be generated from other
attributes in the directory. In a lot of real-life cases I see, that
would be enough to discount PostgreSQL's LDAP authentication completely.


ATB,

Mark.


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Thomas Munro]

On Wed, Aug 2, 2017 at 5:36 AM, Peter Eisentraut
 wrote:
> On 7/16/17 19:09, Thomas Munro wrote:
>> On Mon, Jul 17, 2017 at 10:26 AM, Thomas Munro
>>  wrote:
>>> ldap-search-filters-v2.patch
>>
>> Gah, it would help if I could spell "occurrences" correctly.  Fixed in
>> the attached.
>
> Please also add the corresponding support for specifying search filters
> in LDAP URLs.  See RFC 4516 for the format and
> https://linux.die.net/man/3/ldap_url_parse for the API.  You might just
> need to grab LDAPURLDesc.lud_filter and use it.

Good idea.  Yes, it seems to be that simple.  Here's a version like
that.  Here's an example of how it looks in pg_hba.conf:

host   all all  127.0.0.1/32ldap
ldapurl="ldap://localhost/ou=people1,dc=my-domain,dc=com??sub?(cn=%25u)"

Maybe we could choose a better token than %u for user name, since it
has to be escaped when included in a URL like that, but on the other
hand there seems to be wide precedent for %u in other software.

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


ldap-search-filters-v4.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] More flexible LDAP auth search filters?

[Peter Eisentraut]

On 7/16/17 19:09, Thomas Munro wrote:
> On Mon, Jul 17, 2017 at 10:26 AM, Thomas Munro
>  wrote:
>> ldap-search-filters-v2.patch
> 
> Gah, it would help if I could spell "occurrences" correctly.  Fixed in
> the attached.

Please also add the corresponding support for specifying search filters
in LDAP URLs.  See RFC 4516 for the format and
https://linux.die.net/man/3/ldap_url_parse for the API.  You might just
need to grab LDAPURLDesc.lud_filter and use it.

-- 
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] More race conditions in logical replication

[Alvaro Herrera]

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> 
> > Hmm, yeah, that's not good.  However, I didn't like the idea of putting
> > it inside the locked area, as it's too much code.  Instead I added just
> > before acquiring the spinlock.  We cancel the sleep unconditionally
> > later on if we didn't need to sleep after all.
> 
> I just noticed that Jacana failed again in the subscription test, and it
> looks like it's related to this.  I'll take a look tomorrow if no one
> beats me to it.
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2017-07-26%2014%3A39%3A54

Ahh, I misread the message.  This one is actually about the replication
*origin* being still active, not the replication *slot*.  I suppose
origins need the same treatment as we just did for slots.  Anybody wants
to volunteer a patch?

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


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

Re: [HACKERS] More race conditions in logical replication

[Alvaro Herrera]

Alvaro Herrera wrote:

> Hmm, yeah, that's not good.  However, I didn't like the idea of putting
> it inside the locked area, as it's too much code.  Instead I added just
> before acquiring the spinlock.  We cancel the sleep unconditionally
> later on if we didn't need to sleep after all.

I just noticed that Jacana failed again in the subscription test, and it
looks like it's related to this.  I'll take a look tomorrow if no one
beats me to it.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2017-07-26%2014%3A39%3A54

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


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

Re: [HACKERS] More race conditions in logical replication

[Robert Haas]

On Tue, Jul 25, 2017 at 5:47 AM, Petr Jelinek
 wrote:
> As a side note, the ConditionVariablePrepareToSleep()'s comment could be
> improved because currently it says the only advantage is that we skip
> double-test in the beginning of ConditionVariableSleep(). But that's not
> true, it's essential for preventing race conditions like the one above
> because it puts the current process into waiting list so we can be sure
> it will be signaled on broadcast once ConditionVariablePrepareToSleep()
> has been called.

But if you don't call ConditionVariablePrepareToSleep() before calling
ConditionVariableSleep(), then the first call to the latter will call
the former and return without doing anything else.  So I don't see how
this can ever go wrong if you're using these primitives as documented.

-- 
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] More race conditions in logical replication

[Robert Haas]

On Tue, Jul 25, 2017 at 1:42 PM, Alvaro Herrera
 wrote:
> As I see it, we need to backpatch at least parts of this patch.  I've
> received reports that in earlier branches pglogical and BDR can
> sometimes leave slots behind when removing nodes, and I have a hunch
> that it can be explained by the bugs being fixed here.  Now we cannot
> use condition variables in back-branches, so we'll need to figure out
> how to actually do it ...

If all you had to back-patch was the condition variable code itself,
that might not really be all that bad, but it depends on the
WaitEventSet stuff, which I think is far too dangerous to back-patch.
However, you might be able to create a dumber, slower version that
only uses WaitLatch.

-- 
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] More race conditions in logical replication

[Alvaro Herrera]

Petr Jelinek wrote:
> On 25/07/17 01:33, Alvaro Herrera wrote:
> > Alvaro Herrera wrote:
> >> I'm back at looking into this again, after a rather exhausting week.  I
> >> think I have a better grasp of what is going on in this code now,
> > 
> > Here's an updated patch, which I intend to push tomorrow.
> 
> I think the ConditionVariablePrepareToSleep() call in
> ReplicationSlotAcquire() needs to be done inside the mutex lock
> otherwise there is tiny race condition where the process which has slot
> acquired might release the slot between the mutex unlock and the
> ConditionVariablePrepareToSleep() call which means we'll never get
> signaled and ConditionVariableSleep() will wait forever.

Hmm, yeah, that's not good.  However, I didn't like the idea of putting
it inside the locked area, as it's too much code.  Instead I added just
before acquiring the spinlock.  We cancel the sleep unconditionally
later on if we didn't need to sleep after all.

As I see it, we need to backpatch at least parts of this patch.  I've
received reports that in earlier branches pglogical and BDR can
sometimes leave slots behind when removing nodes, and I have a hunch
that it can be explained by the bugs being fixed here.  Now we cannot
use condition variables in back-branches, so we'll need to figure out
how to actually do it ...

> As a side note, the ConditionVariablePrepareToSleep()'s comment could be
> improved because currently it says the only advantage is that we skip
> double-test in the beginning of ConditionVariableSleep(). But that's not
> true, it's essential for preventing race conditions like the one above
> because it puts the current process into waiting list so we can be sure
> it will be signaled on broadcast once ConditionVariablePrepareToSleep()
> has been called.

Hmm.

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


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

Re: [HACKERS] More race conditions in logical replication

[Petr Jelinek]

On 25/07/17 01:33, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>> I'm back at looking into this again, after a rather exhausting week.  I
>> think I have a better grasp of what is going on in this code now,
> 
> Here's an updated patch, which I intend to push tomorrow.
> 

I think the ConditionVariablePrepareToSleep() call in
ReplicationSlotAcquire() needs to be done inside the mutex lock
otherwise there is tiny race condition where the process which has slot
acquired might release the slot between the mutex unlock and the
ConditionVariablePrepareToSleep() call which means we'll never get
signaled and ConditionVariableSleep() will wait forever.

Otherwise the patch looks good to me.

As a side note, the ConditionVariablePrepareToSleep()'s comment could be
improved because currently it says the only advantage is that we skip
double-test in the beginning of ConditionVariableSleep(). But that's not
true, it's essential for preventing race conditions like the one above
because it puts the current process into waiting list so we can be sure
it will be signaled on broadcast once ConditionVariablePrepareToSleep()
has been called.

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


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

Re: [HACKERS] More race conditions in logical replication

[Alvaro Herrera]

Alvaro Herrera wrote:
> I'm back at looking into this again, after a rather exhausting week.  I
> think I have a better grasp of what is going on in this code now,

Here's an updated patch, which I intend to push tomorrow.

> and it
> appears to me that we should change the locking so that active_pid is
> protected by ReplicationSlotControlLock instead of the slot's spinlock;
> but I haven't analyzed the idea fully yet and I don't have the patch
> done yet either.

This doesn't seem to be a good idea actually.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 33f08678bf20eed3a4cb3d10960bb06543a1b3db Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 12 Jul 2017 18:38:33 -0400
Subject: [PATCH v4] Wait for slot to become free in when dropping it

---
 src/backend/replication/logical/logicalfuncs.c |   2 +-
 src/backend/replication/slot.c | 115 ++---
 src/backend/replication/slotfuncs.c|  32 ---
 src/backend/replication/walsender.c|   6 +-
 src/include/replication/slot.h |  10 ++-
 5 files changed, 110 insertions(+), 55 deletions(-)

diff --git a/src/backend/replication/logical/logicalfuncs.c 
b/src/backend/replication/logical/logicalfuncs.c
index 363ca82cb0..a3ba2b1266 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -244,7 +244,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, 
bool confirm, bool bin
else
end_of_wal = GetXLogReplayRecPtr();
 
-   ReplicationSlotAcquire(NameStr(*name));
+   ReplicationSlotAcquire(NameStr(*name), true);
 
PG_TRY();
{
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index dc7de20e11..ea9cd1f22b 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -157,6 +157,7 @@ ReplicationSlotsShmemInit(void)
/* everything else is zeroed by the memset above */
SpinLockInit(>mutex);
LWLockInitialize(>io_in_progress_lock, 
LWTRANCHE_REPLICATION_SLOT_IO_IN_PROGRESS);
+   ConditionVariableInit(>active_cv);
}
}
 }
@@ -313,25 +314,35 @@ ReplicationSlotCreate(const char *name, bool db_specific,
LWLockRelease(ReplicationSlotControlLock);
 
/*
-* Now that the slot has been marked as in_use and in_active, it's safe 
to
+* Now that the slot has been marked as in_use and active, it's safe to
 * let somebody else try to allocate a slot.
 */
LWLockRelease(ReplicationSlotAllocationLock);
+
+   /* Let everybody know we've modified this slot */
+   ConditionVariableBroadcast(>active_cv);
 }
 
 /*
  * Find a previously created slot and mark it as used by this backend.
  */
 void
-ReplicationSlotAcquire(const char *name)
+ReplicationSlotAcquire(const char *name, bool nowait)
 {
-   ReplicationSlot *slot = NULL;
+   ReplicationSlot *slot;
+   int active_pid;
int i;
-   int active_pid = 0; /* Keep compiler quiet */
 
+retry:
Assert(MyReplicationSlot == NULL);
 
-   /* Search for the named slot and mark it active if we find it. */
+   /*
+* Search for the named slot and mark it active if we find it.  If the
+* slot is already active, we exit the loop with active_pid set to the 
PID
+* of the backend that owns it.
+*/
+   active_pid = 0;
+   slot = NULL;
LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
for (i = 0; i < max_replication_slots; i++)
{
@@ -339,35 +350,59 @@ ReplicationSlotAcquire(const char *name)
 
if (s->in_use && strcmp(name, NameStr(s->data.name)) == 0)
{
+   /* Found the slot we want -- can we activate it? */
SpinLockAcquire(>mutex);
+
active_pid = s->active_pid;
if (active_pid == 0)
active_pid = s->active_pid = MyProcPid;
+
SpinLockRelease(>mutex);
slot = s;
+
break;
}
}
LWLockRelease(ReplicationSlotControlLock);
 
-   /* If we did not find the slot or it was already active, error out. */
+   /* If we did not find the slot, error out. */
if (slot == NULL)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
 errmsg("replication slot \"%s\" does not 
exist", name)));
+
+   /*
+* If we found the slot but it's already active in another backend, we
+* either error out or retry after a short wait, as caller 

Re: [HACKERS] More race conditions in logical replication

[Alvaro Herrera]

I'm back at looking into this again, after a rather exhausting week.  I
think I have a better grasp of what is going on in this code now, and it
appears to me that we should change the locking so that active_pid is
protected by ReplicationSlotControlLock instead of the slot's spinlock;
but I haven't analyzed the idea fully yet and I don't have the patch
done yet either.  I'm going to report, hopefully with a fix committed
this time, on Monday at 19:00 CLT.

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


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

Re: [HACKERS] More optimization effort?

[Greg Stark]

On 21 July 2017 at 20:00, Tom Lane  wrote:

>> I have, however, decided not to volunteer to be the one who works on
>> that project.
>
> Me either.  Any one of these things would require a *lot* of work in
> order to have a coherent feature that provided useful behavior across
> a bunch of different datatypes.

I had in the past idly thought about whether it would be possible to
link in one of the various general purpose theorem proving libraries
and use it to simplify the expressions. But I really have no idea how
much work it would be to teach one about all the properties and
constraints of our existing data types and operators or for that
matter how easy it would be to figure out what theorems we want proven
to be able to use an index.



-- 
greg


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

Re: [HACKERS] More optimization effort?

[Tom Lane]

Robert Haas  writes:
> Another very similar (but possibly easier) case is:

> select * from pgbench_accounts where aid = 1.0;

> This will use a sequential scan rather than an index scan, because the
> query optimizer doesn't know that the only integer for which =(int4,
> numeric) will return true is 1.  Therefore it has to scan the whole
> table one row at a time and check, for each one, whether the =
> operator returns true.  It can't cast the constant to an integer
> because the user might have written 1.1 rather than 1.0, in which case
> the cast would fail; but the query should return 0 rows, not ERROR.

> You can imagine fixing this by having some kind of datatype-specific
> knowledge that would replace "aid = 1.0" with "aid = 1" and "aid =
> 1.1" with "false"; it would also have to know that "aid = 99"
> should be changed to "false" because 99 isn't representable as
> int4.

Couple thoughts about that:

* Another conceivable route to a solution is to add "int = numeric"
and friends to the btree opclasses for integers.  I'm not sure if
there is any fundamental reason we've not done that (it's possible
it would fall foul of the requirements about transitivity, not sure).
However, this would only fix things to the extent of allowing an
index scan to occur; it wouldn't help in non-indexed cases, nor would
it know anything about simplifying say "aid = 1.1" to "false".

* The already-existing protransform infrastructure could be used to
address this type of problem; that is, you could imagine attaching
a transform function to numeric_eq that would look for cases like
"integer::numeric = nonintegralconstant" and simplify them accordingly.
When that feature went in, there was talk of using transforms to
simplify e.g. "variable + 0" or "variable * 1", but nobody's got round
to anything of the sort yet.

* On the other hand, protransform doesn't offer any easy way to apply
similar optimizations to a bunch of different functions/operators.
For instance, if your goal is to simplify "variable + 0", there are
a depressingly large number of "+" operators to write transform
functions for.  Maybe there's no way around duplicate coding for that,
but it looks tedious and bulky.

> I have, however, decided not to volunteer to be the one who works on
> that project.

Me either.  Any one of these things would require a *lot* of work in
order to have a coherent feature that provided useful behavior across
a bunch of different datatypes.

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] More optimization effort?

[Robert Haas]

On Fri, Jul 21, 2017 at 10:33 AM, Tom Lane  wrote:
> But the bigger picture is that doing something that helps to any
> useful extent would require a really substantial amount of new,
> datatype- and operator-specific knowledge that doesn't exist in the
> system today.  And as Craig noted, applying that knowledge would
> be expensive, even in cases where it failed to help.
>
> So, color me skeptical ...

I agree, but with a caveat.  If somebody felt like doing all of that
work, and either made it cheap enough to justify enabling it by
default or added a controlling GUC, it'd be fine with me.  We've
talked before about having knobs to adjust how hard the optimizer
tries to optimize things, and this would be a good candidate for such
a thing.  The bigger issue from my perspective is that I really doubt
that anybody wants to put the effort into doing something like this in
a principled way.

Another very similar (but possibly easier) case is:

select * from pgbench_accounts where aid = 1.0;

This will use a sequential scan rather than an index scan, because the
query optimizer doesn't know that the only integer for which =(int4,
numeric) will return true is 1.  Therefore it has to scan the whole
table one row at a time and check, for each one, whether the =
operator returns true.  It can't cast the constant to an integer
because the user might have written 1.1 rather than 1.0, in which case
the cast would fail; but the query should return 0 rows, not ERROR.

You can imagine fixing this by having some kind of datatype-specific
knowledge that would replace "aid = 1.0" with "aid = 1" and "aid =
1.1" with "false"; it would also have to know that "aid = 99"
should be changed to "false" because 99 isn't representable as
int4.

I have, however, decided not to volunteer to be the one who works on
that project.

-- 
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] More optimization effort?

[Tom Lane]

Tatsuo Ishii  writes:
> Currently following query does not use an index:
> test=# explain select * from pgbench_accounts where aid*100 < 1;
> While following one does use the index.
> test=# explain select * from pgbench_accounts where aid < 1/100;

> Is it worth to make our optimizer a little bit smarter to convert the
> the first query into the second form?

That's a rather ambitious definition of "little bit" smarter.

For starters, I'm not sure those queries are even fully equivalent
w.r.t. issues like overflow and roundoff.  While a person might
decide that the transformation is OK anyway, I'm not sure that the
optimizer should be allowed to take such liberties.

But the bigger picture is that doing something that helps to any
useful extent would require a really substantial amount of new,
datatype- and operator-specific knowledge that doesn't exist in the
system today.  And as Craig noted, applying that knowledge would
be expensive, even in cases where it failed to help.

So, color me skeptical ...

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] More optimization effort?

[Craig Ringer]

On 21 July 2017 at 07:11, Tatsuo Ishii  wrote:

> Currently following query does not use an index:
>
> t-ishii@localhost: psql -p 5433 test
> Pager usage is off.
> psql (9.6.3)
> Type "help" for help.
>
> test=# explain select * from pgbench_accounts where aid*100 < 1;
>QUERY PLAN
> 
>  Seq Scan on pgbench_accounts  (cost=0.00..3319.00 rows=3 width=97)
>Filter: ((aid * 100) < 1)
> (2 rows)
>
> While following one does use the index.
>
> test=# explain select * from pgbench_accounts where aid < 1/100;
> QUERY PLAN
> 
> --
>  Index Scan using pgbench_accounts_pkey on pgbench_accounts
> (cost=0.29..11.08 rows=102 width=97)
>Index Cond: (aid < 100)
> (2 rows)
>
> Is it worth to make our optimizer a little bit smarter to convert the
> the first query into the second form?
>

If I understand correctly, you're proposing that the optimiser should
attempt algebraic simplification to fold more constants, rather than
stopping pre-evaluation constant expressions  as soon as we see a
non-constant like we do now. Right?

I'm sure there are documented algorithms out there for algebraic
manipulations like that, taking account of precedence etc. But will they be
cheap enough to run in the optimiser? And likely to benefit many queries?

There's also the hiccup of partial index matching. If Pg simplifies and
rearranges expressions more, will we potentially fail to match partial
indexes that we would've originally matched? I'm not sure it's a blocker,
but it bears consideration, and Pg might have to do more work on partial
index matching too.

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

[HACKERS] More optimization effort?

[Tatsuo Ishii]

Currently following query does not use an index:

t-ishii@localhost: psql -p 5433 test
Pager usage is off.
psql (9.6.3)
Type "help" for help.

test=# explain select * from pgbench_accounts where aid*100 < 1;
   QUERY PLAN   

 Seq Scan on pgbench_accounts  (cost=0.00..3319.00 rows=3 width=97)
   Filter: ((aid * 100) < 1)
(2 rows)

While following one does use the index.

test=# explain select * from pgbench_accounts where aid < 1/100;
QUERY PLAN  
  
--
 Index Scan using pgbench_accounts_pkey on pgbench_accounts  (cost=0.29..11.08 
rows=102 width=97)
   Index Cond: (aid < 100)
(2 rows)

Is it worth to make our optimizer a little bit smarter to convert the
the first query into the second form?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Robert Haas]

On Sun, Jul 16, 2017 at 7:23 PM, Stephen Frost  wrote:
>> Refusing to improve LDAP for the users who have no choice seems like a very
>> unfriendly thing to do.
>
> I'm fine with improving LDAP in general, but, as I tried to point out,
> having a way to make it easier to integrate PG into an AD environment
> would be better.  It's not my intent to stop this patch but rather to
> point out the issues with LDAP auth that far too frequently are not
> properly understood.

Then it's off-topic for this thread.

-- 
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] More flexible LDAP auth search filters?

[Mark Cave-Ayland]

On 17/07/17 18:08, Magnus Hagander wrote:

> On Mon, Jul 17, 2017 at 6:47 PM, Mark Cave-Ayland
> >
> wrote: 
> Great to hear from you! It has definitely been a while...
> 
> Indeed. You should spend more time on these lists :P

Well I do get the emails, unfortunately it's trying to find the time to
read them all ;)

> > How would that actually work, though? Given the same user in ldap could
> > now potentially match multiple different users in PostgreSQL. Would you
> > then create two accounts for the user, one with the uid as name and one
> > with email as name? Wouldn't that actually cause more issues than it 
> solves?
> 
> Normally what happens for search+bind is that you execute the custom
> filter with substitutions in order to find the entry for your bind DN,
> but you also request the value of ldapsearchattribute (or equivalent) at
> the same time. Say for example you had an entry like this:
> 
> dn: uid=mha,dc=users,dc=hagander,dc=net
> uid: mha
> mail: mag...@hagander.net 
> 
> Using the filter "(|(mail=%u)(uid=%u))" would locate the same bind DN
> "uid=mha,dc=users,dc=hagander,dc=net" with either one of your uid or
> email address.
> 
> If the bind is successful then the current user identity should be set
> to the value of the ldapsearchattribute retrieved from the bind DN
> entry, which with the default of "uid" would be "mha". Hence you end up
> with the same user role "mha" regardless of whether a uid or email
> address was entered.
> 
> 
> Right. This is the part that doesn't work for PostgreSQL. Because we
> have already specified the username -- it goes in the startup packet in
> order to pick the correct row from pg_hba.conf.

I don't think that's necessarily going to be an issue here because if
you're specifying a custom LDAP filter then there's a very good chance
that you're delegating access control to information held in the
directory anyway, e.g.

  (&(memberOf=cn=pgusers,dc=groups,dc=hagander,dc=net)(uid=%u))

  ((&(uid=%u)(|(uid=mha)(uid=mark)(uid=thomas)))

In the mail example above when you're using more than one attribute, I
think it's fair enough to simply say in the documentation you must set
user to "all" in pg_hba.conf since it is impossible to know which
attribute is being used to identify the user role until after
authentication.

I should add that personally I don't recommend such setups where the
user can log in using more than one identifier, but there are clients
out there who absolutely will insist on it (think internal vs. external
users) so if the LDAP support is being updated then it's worth exploring
to see if these cases can be supported.

> I guess in theory we could treat it like Kerberos or another one of the
> systems where we get the username from an external entity. But then
> you'd still have to specify the mapping again in pg_ident.conf, and it
> would be a pretty strong break of backwards compatibility.

(goes and glances at the code)

Is there no reason why you couldn't just overwrite port->user_name based
upon ldapsearchattribute at the end of CheckLDAPAuth?

And if this were only enabled when a custom filter were specified then
it shouldn't cause any backwards compatibility issues being a new feature?

> In terms of matching multiple users, all LDAP authentication routines
> I've seen will fail if more than one DN matches the search filter, so
> this nicely handles the cases where either the custom filter is
> incorrect or more than one entry is accidentally matched in the
> directory.
> 
> So do we, in the current implementation. But it's a lot less likely to
> happen in the current implementation, since we do a single equals lookup.

Great, that's absolutely fine :)


ATB,

Mark.


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

Re: [HACKERS] More race conditions in logical replication

[Alvaro Herrera]

Alvaro Herrera wrote:
> After going over this a bunch more times, I found other problems.  For
> example, I noticed that if I create a temporary slot in one session,
> then another session would rightly go to sleep if it tries to drop that
> slot.  But the slot goes away when the first session disconnects, so I'd
> expect the sleeping session to get a signal and wake up, but that
> doesn't happen.
> 
> I'm going to continue to look at this and report back Tuesday 18th.

I got tangled up in a really tough problem this week, so I won't have
time to work on this for a couple of days.  I can next update tomorrow
19:00 CLT (probably not with a final fix yet, though).

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


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Mon, Jul 17, 2017 at 6:47 PM, Mark Cave-Ayland <
mark.cave-ayl...@ilande.co.uk> wrote:

> On 17/07/17 13:09, Magnus Hagander wrote:
>
> Hi Magnus,
>
> Great to hear from you! It has definitely been a while...
>

Indeed. You should spend more time on these lists :P



>
> > Generally you find that you will be given the option to set the
> > attribute for the default search filter of the form
> > "(attribute=username)" which defaults to uid for UNIX-type systems
> and
> > sAMAccountName for AD. However there is always the ability to
> specify a
> > custom filter where the user is substituted via e.g. %u to cover all
> the
> > other use-cases.
> >
> > Right, but that's something we do already today. It just defaults to
> > uid, but it's easy to change.
>
> Yes, I think that bit is fine as long as the default can be overridden.
> There's always a choice as to whether the defaults favour a POSIX-based
> LDAP or AD environment but that happens with all installations so it's
> not a big deal.
>

It's easy enough to change.



> > As an example, I don't know if anyone would actually do this with
> > PostgreSQL but I've been asked on multiple occasions to configure
> > software so that users should be allowed to log in with either their
> > email address or username which is easily done with a custom LDAP
> filter
> > like "(|(mail=%u)(uid=%u))".
> >
> >
> > How would that actually work, though? Given the same user in ldap could
> > now potentially match multiple different users in PostgreSQL. Would you
> > then create two accounts for the user, one with the uid as name and one
> > with email as name? Wouldn't that actually cause more issues than it
> solves?
>
> Normally what happens for search+bind is that you execute the custom
> filter with substitutions in order to find the entry for your bind DN,
> but you also request the value of ldapsearchattribute (or equivalent) at
> the same time. Say for example you had an entry like this:
>
> dn: uid=mha,dc=users,dc=hagander,dc=net
> uid: mha
> mail: mag...@hagander.net
>
> Using the filter "(|(mail=%u)(uid=%u))" would locate the same bind DN
> "uid=mha,dc=users,dc=hagander,dc=net" with either one of your uid or
> email address.
>
> If the bind is successful then the current user identity should be set
> to the value of the ldapsearchattribute retrieved from the bind DN
> entry, which with the default of "uid" would be "mha". Hence you end up
> with the same user role "mha" regardless of whether a uid or email
> address was entered.
>

Right. This is the part that doesn't work for PostgreSQL. Because we have
already specified the username -- it goes in the startup packet in order to
pick the correct row from pg_hba.conf.

I guess in theory we could treat it like Kerberos or another one of the
systems where we get the username from an external entity. But then you'd
still have to specify the mapping again in pg_ident.conf, and it would be a
pretty strong break of backwards compatibility.



> In terms of matching multiple users, all LDAP authentication routines
> I've seen will fail if more than one DN matches the search filter, so
> this nicely handles the cases where either the custom filter is
> incorrect or more than one entry is accidentally matched in the directory.
>

So do we, in the current implementation. But it's a lot less likely to
happen in the current implementation, since we do a single equals lookup.

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Mark Cave-Ayland]

On 17/07/17 13:09, Magnus Hagander wrote:

Hi Magnus,

Great to hear from you! It has definitely been a while...

> Generally you find that you will be given the option to set the
> attribute for the default search filter of the form
> "(attribute=username)" which defaults to uid for UNIX-type systems and
> sAMAccountName for AD. However there is always the ability to specify a
> custom filter where the user is substituted via e.g. %u to cover all the
> other use-cases.
> 
> Right, but that's something we do already today. It just defaults to
> uid, but it's easy to change. 

Yes, I think that bit is fine as long as the default can be overridden.
There's always a choice as to whether the defaults favour a POSIX-based
LDAP or AD environment but that happens with all installations so it's
not a big deal.

> As an example, I don't know if anyone would actually do this with
> PostgreSQL but I've been asked on multiple occasions to configure
> software so that users should be allowed to log in with either their
> email address or username which is easily done with a custom LDAP filter
> like "(|(mail=%u)(uid=%u))".
> 
> 
> How would that actually work, though? Given the same user in ldap could
> now potentially match multiple different users in PostgreSQL. Would you
> then create two accounts for the user, one with the uid as name and one
> with email as name? Wouldn't that actually cause more issues than it solves?

Normally what happens for search+bind is that you execute the custom
filter with substitutions in order to find the entry for your bind DN,
but you also request the value of ldapsearchattribute (or equivalent) at
the same time. Say for example you had an entry like this:

dn: uid=mha,dc=users,dc=hagander,dc=net
uid: mha
mail: mag...@hagander.net

Using the filter "(|(mail=%u)(uid=%u))" would locate the same bind DN
"uid=mha,dc=users,dc=hagander,dc=net" with either one of your uid or
email address.

If the bind is successful then the current user identity should be set
to the value of the ldapsearchattribute retrieved from the bind DN
entry, which with the default of "uid" would be "mha". Hence you end up
with the same user role "mha" regardless of whether a uid or email
address was entered.

In terms of matching multiple users, all LDAP authentication routines
I've seen will fail if more than one DN matches the search filter, so
this nicely handles the cases where either the custom filter is
incorrect or more than one entry is accidentally matched in the directory.


ATB,

Mark.


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Mon, Jul 17, 2017 at 1:23 AM, Stephen Frost  wrote:

>
> * Magnus Hagander (mag...@hagander.net) wrote:
> > On Sun, Jul 16, 2017 at 11:05 PM, Stephen Frost 
> wrote:
> > > I'd suggest that we try to understand why Kerberos couldn't be used in
> > > that environment.  I suspect in at least some cases what users would
> > > like is the ability to do Kerberos auth but then have LDAP checked to
> > > see if a given user (who has now auth'd through Kerberos) is allowed to
> > > connect.  We don't currently have any way to do that, but if we were
> > > looking for things to do, that's what I'd suggest working on rather
> than
> > > adding more to our LDAP auth system and implying by doing so that it's
> > > reasonable to use.
> > >
> > > I find it particularly disappointing to see recommendations for using
> > > LDAP auth, particularly in AD environments, that don't even mention
> > > Kerberos or bother to explain how using LDAP sends the user's PW to the
> > > server in cleartext.
> >
> > You do realize, I'm sure, that there are many LDAP servers out there that
> > are not AD, and that do not come with a Kerberos server attached to
> them...
>
> I am aware that some exist, I've even contributed to their development
> and packaging, but that doesn't make it a good idea to use them for
> authentication.
>

Pretty sure that doesn't include any of the ones I'm talking about, but
sure :)



> > I agree that Kerberos is usually the better choice *if it's available*.
>
> Which is the case in an AD environment..
>

Yes. But we shouldn't force everybody to use AD :P



> > It's several orders of magnitude more complicated to set up though, and
> > there are many environments that have ldap but don't have Kerberos.
>
> Frankly, I simply don't agree with this.
>

Really?

For LDAP auth I don't need to do *anything* in preparation. For Kerberos
auth I need to create an account, set encryption type, export keys, etc.
You can't possibly claim this is the same level of complexity?




> > Refusing to improve LDAP for the users who have no choice seems like a
> very
> > unfriendly thing to do.
>
> I'm fine with improving LDAP in general, but, as I tried to point out,
> having a way to make it easier to integrate PG into an AD environment
> would be better.  It's not my intent to stop this patch but rather to
> point out the issues with LDAP auth that far too frequently are not
> properly understood.
>

A documentation patch for that would certainly be a good place to start.
Perhaps with up to date instructions for how to actually set up Kerberos in
an AD environment including all steps required?



> > (And you can actually reasonably solve the case of
> > kerberos-for-auth-ldap-for-priv by syncing the groups into postgres
> roles)
>
> Yes, but sync'ing roles is a bit of a pain and it'd be nice if we could
> avoid it, or perhaps make it easier.
>

Certainly.

//Magnus

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Sun, Jul 16, 2017 at 7:58 PM, Mark Cave-Ayland <
mark.cave-ayl...@ilande.co.uk> wrote:

> On 16/07/17 00:08, Thomas Munro wrote:
>
> > On Fri, Jul 14, 2017 at 11:04 PM, Magnus Hagander 
> wrote:
> >> On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro
> >>  wrote:
> >>> A post on planet.postgresql.org today reminded me that a colleague had
> >>> asked me to post this POC patch here for discussion.  It allows custom
> >>> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
> >>> might be to take a filter pattern with "%USERNAME%" or whatever in it.
> >>> There's an existing precedent for the prefix and suffix approach, but
> >>> on the other hand a pattern approach would allow filters where the
> >>> username is inserted more than once.
> >>
> >>
> >> Do we even need prefix/suffix? If we just make it "ldapsearchpattern",
> then
> >> you could have something like:
> >>
> >> ldapsearchattribute="uid"
> >> ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA
> Team)"
> >>
> >> We could then always to substitution of the kind:
> >> (&(attr=)())
> >>
> >> which would in this case give:
> >> (&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))
> >>
> >>
> >> Basically we'd always AND together the username lookup with the
> additional
> >> filter.
> >
> > Ok, so we have 3 ideas put forward:
> >
> > 1.  Wrap username with ldapsearchprefix ldapsearchsuffix to build
> > filter (as implemented by POC patch)
> > 2.  Optionally AND ldapsearchfilter with the existing
> > ldapsearchattribute-based filter (Magnus's proposal)
> > 3.  Pattern-based ldapsearchfilter so that %USER% is replaced with
> > username (my other suggestion)
> >
> > The main argument for approach 1 is that it follows the style of the
> > bind-only mode.
> >
> > With idea 2, I wonder if there are some more general kinds of things
> > that people might want to do that that wouldn't be possible because it
> > has to include (attribute=user)... perhaps something involving a
> > substring or other transformation functions (but I'm no LDAP expert,
> > that may not make sense).
> >
> > With idea 3 it would allow "(|(foo=%USER%)(bar=%USER%))", though I
> > don't know if any such multiple-mention filters would ever make sense
> > in a sane LDAP configuration.
> >
> > Any other views from LDAP-users?
>
> I've spent quite a bit of time integrating various bits of
> non-PostgreSQL software to LDAP and in my experience option 3 tends to
> be the standard.
>
> Generally you find that you will be given the option to set the
> attribute for the default search filter of the form
> "(attribute=username)" which defaults to uid for UNIX-type systems and
> sAMAccountName for AD. However there is always the ability to specify a
> custom filter where the user is substituted via e.g. %u to cover all the
> other use-cases.


Right, but that's something we do already today. It just defaults to uid,
but it's easy to change.



>
> As an example, I don't know if anyone would actually do this with
> PostgreSQL but I've been asked on multiple occasions to configure
> software so that users should be allowed to log in with either their
> email address or username which is easily done with a custom LDAP filter
> like "(|(mail=%u)(uid=%u))".
>

How would that actually work, though? Given the same user in ldap could now
potentially match multiple different users in PostgreSQL. Would you then
create two accounts for the user, one with the uid as name and one with
email as name? Wouldn't that actually cause more issues than it solves?

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Mark Cave-Ayland]

On 17/07/17 00:14, Stephen Frost wrote:

>> If it helps, we normally recommend that clients use ldaps for both AD
>> and UNIX environments, although this can be trickier from an
>> administrative perspective in AD environments because it can require
>> changes to the Windows firewall and certificate installation.
> 
> LDAPS is better than straight LDAP, of course, but it still doesn't
> address the issue that the password is sent to the server, which both
> SCRAM and Kerberos do and is why AD environments use Kerberos for
> authentication, and why everything in an AD environment also should use
> Kerberos.
> 
> Using Kerberos should also avoid the need to hack the Windows firewall
> or deal with certificate installation.  In an AD environment, it's
> actually pretty straight-forward to add a PG server too.  Further, in my
> experience at least, there's been other changes recommended by Microsoft
> that prevent using LDAP for auth because it's insecure.

Oh sure - I'm not questioning that Kerberos is a far better choice in
pure AD environments, it's just that I spend the majority of my time in
mixed-mode environments where Windows is very much in the minority.

In my experience LDAP is often implemented badly; for example the
majority of software still uses simple binds (i.e. plain logins) rather
than using SASL binds which support a whole range of better
authentication methods (e.g. GSSAPI, and even DIGEST-MD5 has been
mandatory for v3 and is implemented on AD).

And yes, while better authentication mechanisms do exist, I find that
all too often most software packages claim LDAP support rather than
Kerberos, and even then it is often limited to LDAP simple binds without
ldaps support.


ATB,

Mark.


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Stephen Frost]

Magnus,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Sun, Jul 16, 2017 at 11:05 PM, Stephen Frost  wrote:
> > I'd suggest that we try to understand why Kerberos couldn't be used in
> > that environment.  I suspect in at least some cases what users would
> > like is the ability to do Kerberos auth but then have LDAP checked to
> > see if a given user (who has now auth'd through Kerberos) is allowed to
> > connect.  We don't currently have any way to do that, but if we were
> > looking for things to do, that's what I'd suggest working on rather than
> > adding more to our LDAP auth system and implying by doing so that it's
> > reasonable to use.
> >
> > I find it particularly disappointing to see recommendations for using
> > LDAP auth, particularly in AD environments, that don't even mention
> > Kerberos or bother to explain how using LDAP sends the user's PW to the
> > server in cleartext.
> 
> You do realize, I'm sure, that there are many LDAP servers out there that
> are not AD, and that do not come with a Kerberos server attached to them...

I am aware that some exist, I've even contributed to their development
and packaging, but that doesn't make it a good idea to use them for
authentication.

> I agree that Kerberos is usually the better choice *if it's available*.

Which is the case in an AD environment..

> It's several orders of magnitude more complicated to set up though, and
> there are many environments that have ldap but don't have Kerberos.

Frankly, I simply don't agree with this.

> Refusing to improve LDAP for the users who have no choice seems like a very
> unfriendly thing to do.

I'm fine with improving LDAP in general, but, as I tried to point out,
having a way to make it easier to integrate PG into an AD environment
would be better.  It's not my intent to stop this patch but rather to
point out the issues with LDAP auth that far too frequently are not
properly understood.

> (And you can actually reasonably solve the case of
> kerberos-for-auth-ldap-for-priv by syncing the groups into postgres roles)

Yes, but sync'ing roles is a bit of a pain and it'd be nice if we could
avoid it, or perhaps make it easier.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] More flexible LDAP auth search filters?

[Stephen Frost]

Mark,

* Mark Cave-Ayland (mark.cave-ayl...@ilande.co.uk) wrote:
> On 16/07/17 23:26, Thomas Munro wrote:
> > Thank you very much for this feedback and example, which I used in the
> > documentation in the patch.  I see similar examples in the
> > documentation for other things on the web.
> > 
> > I'll leave it up to Magnus and Stephen to duke it out over whether we
> > want to encourage LDAP usage, extend documentation to warn about
> > cleartext passwords with certain LDAP implementations or
> > configurations, etc etc.  I'll add this patch to the commitfest and
> > get some popcorn.
> 
> If it helps, we normally recommend that clients use ldaps for both AD
> and UNIX environments, although this can be trickier from an
> administrative perspective in AD environments because it can require
> changes to the Windows firewall and certificate installation.

LDAPS is better than straight LDAP, of course, but it still doesn't
address the issue that the password is sent to the server, which both
SCRAM and Kerberos do and is why AD environments use Kerberos for
authentication, and why everything in an AD environment also should use
Kerberos.

Using Kerberos should also avoid the need to hack the Windows firewall
or deal with certificate installation.  In an AD environment, it's
actually pretty straight-forward to add a PG server too.  Further, in my
experience at least, there's been other changes recommended by Microsoft
that prevent using LDAP for auth because it's insecure.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] More flexible LDAP auth search filters?

[Thomas Munro]

On Mon, Jul 17, 2017 at 10:26 AM, Thomas Munro
 wrote:
> ldap-search-filters-v2.patch

Gah, it would help if I could spell "occurrences" correctly.  Fixed in
the attached.

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


ldap-search-filters-v3.patch
Description: Binary data

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Mark Cave-Ayland]

On 16/07/17 23:26, Thomas Munro wrote:

> Thank you very much for this feedback and example, which I used in the
> documentation in the patch.  I see similar examples in the
> documentation for other things on the web.
> 
> I'll leave it up to Magnus and Stephen to duke it out over whether we
> want to encourage LDAP usage, extend documentation to warn about
> cleartext passwords with certain LDAP implementations or
> configurations, etc etc.  I'll add this patch to the commitfest and
> get some popcorn.

If it helps, we normally recommend that clients use ldaps for both AD
and UNIX environments, although this can be trickier from an
administrative perspective in AD environments because it can require
changes to the Windows firewall and certificate installation.

Whilst OpenLDAP will support ldap+starttls you can end up with some
clients with starttls either disabled or misconfigured sending plaintext
passwords over the wire regardless, so it's generally easiest to
firewall ldap port 389 at the edge of the trusted VLAN so that only
ldaps port 636 connections make it out onto the untrusted network
hosting the local AD/OpenLDAP server.


ATB,

Mark.


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Thomas Munro]

On Mon, Jul 17, 2017 at 5:58 AM, Mark Cave-Ayland
 wrote:
>> Any other views from LDAP-users?
>
> I've spent quite a bit of time integrating various bits of
> non-PostgreSQL software to LDAP and in my experience option 3 tends to
> be the standard.
>
> Generally you find that you will be given the option to set the
> attribute for the default search filter of the form
> "(attribute=username)" which defaults to uid for UNIX-type systems and
> sAMAccountName for AD. However there is always the ability to specify a
> custom filter where the user is substituted via e.g. %u to cover all the
> other use-cases.

Cool.  Here is a new version of the patch updated to do it exactly
like that.  I tested it against OpenLDAP.

> As an example, I don't know if anyone would actually do this with
> PostgreSQL but I've been asked on multiple occasions to configure
> software so that users should be allowed to log in with either their
> email address or username which is easily done with a custom LDAP filter
> like "(|(mail=%u)(uid=%u))".

Thank you very much for this feedback and example, which I used in the
documentation in the patch.  I see similar examples in the
documentation for other things on the web.

I'll leave it up to Magnus and Stephen to duke it out over whether we
want to encourage LDAP usage, extend documentation to warn about
cleartext passwords with certain LDAP implementations or
configurations, etc etc.  I'll add this patch to the commitfest and
get some popcorn.

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


ldap-search-filters-v2.patch
Description: Binary data

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Sun, Jul 16, 2017 at 11:05 PM, Stephen Frost  wrote:

> Magnus, all,
>
> * Magnus Hagander (mag...@hagander.net) wrote:
> > (FWIW, a workaround I've applied more than once to this in AD
> environments
> > (where kerberos for one reason or other can't be done, sorry Stephen) is
> to
> > set up a RADIUS server and use that one as a "middle man". But it would
> be
> > much better if we could do it natively)
>
> I'd suggest that we try to understand why Kerberos couldn't be used in
> that environment.  I suspect in at least some cases what users would
> like is the ability to do Kerberos auth but then have LDAP checked to
> see if a given user (who has now auth'd through Kerberos) is allowed to
> connect.  We don't currently have any way to do that, but if we were
> looking for things to do, that's what I'd suggest working on rather than
> adding more to our LDAP auth system and implying by doing so that it's
> reasonable to use.
>
> I find it particularly disappointing to see recommendations for using
> LDAP auth, particularly in AD environments, that don't even mention
> Kerberos or bother to explain how using LDAP sends the user's PW to the
> server in cleartext.
>

You do realize, I'm sure, that there are many LDAP servers out there that
are not AD, and that do not come with a Kerberos server attached to them...

I agree that Kerberos is usually the better choice *if it's available*.
It's several orders of magnitude more complicated to set up though, and
there are many environments that have ldap but don't have Kerberos.

Refusing to improve LDAP for the users who have no choice seems like a very
unfriendly thing to do.

(And you can actually reasonably solve the case of
kerberos-for-auth-ldap-for-priv by syncing the groups into postgres roles)

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Stephen Frost]

Magnus, all,

* Magnus Hagander (mag...@hagander.net) wrote:
> (FWIW, a workaround I've applied more than once to this in AD environments
> (where kerberos for one reason or other can't be done, sorry Stephen) is to
> set up a RADIUS server and use that one as a "middle man". But it would be
> much better if we could do it natively)

I'd suggest that we try to understand why Kerberos couldn't be used in
that environment.  I suspect in at least some cases what users would
like is the ability to do Kerberos auth but then have LDAP checked to
see if a given user (who has now auth'd through Kerberos) is allowed to
connect.  We don't currently have any way to do that, but if we were
looking for things to do, that's what I'd suggest working on rather than
adding more to our LDAP auth system and implying by doing so that it's
reasonable to use.

I find it particularly disappointing to see recommendations for using
LDAP auth, particularly in AD environments, that don't even mention
Kerberos or bother to explain how using LDAP sends the user's PW to the
server in cleartext.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] More flexible LDAP auth search filters?

[Mark Cave-Ayland]

On 16/07/17 00:08, Thomas Munro wrote:

> On Fri, Jul 14, 2017 at 11:04 PM, Magnus Hagander  wrote:
>> On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro
>>  wrote:
>>> A post on planet.postgresql.org today reminded me that a colleague had
>>> asked me to post this POC patch here for discussion.  It allows custom
>>> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
>>> might be to take a filter pattern with "%USERNAME%" or whatever in it.
>>> There's an existing precedent for the prefix and suffix approach, but
>>> on the other hand a pattern approach would allow filters where the
>>> username is inserted more than once.
>>
>>
>> Do we even need prefix/suffix? If we just make it "ldapsearchpattern", then
>> you could have something like:
>>
>> ldapsearchattribute="uid"
>> ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)"
>>
>> We could then always to substitution of the kind:
>> (&(attr=)())
>>
>> which would in this case give:
>> (&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))
>>
>>
>> Basically we'd always AND together the username lookup with the additional
>> filter.
> 
> Ok, so we have 3 ideas put forward:
> 
> 1.  Wrap username with ldapsearchprefix ldapsearchsuffix to build
> filter (as implemented by POC patch)
> 2.  Optionally AND ldapsearchfilter with the existing
> ldapsearchattribute-based filter (Magnus's proposal)
> 3.  Pattern-based ldapsearchfilter so that %USER% is replaced with
> username (my other suggestion)
> 
> The main argument for approach 1 is that it follows the style of the
> bind-only mode.
> 
> With idea 2, I wonder if there are some more general kinds of things
> that people might want to do that that wouldn't be possible because it
> has to include (attribute=user)... perhaps something involving a
> substring or other transformation functions (but I'm no LDAP expert,
> that may not make sense).
> 
> With idea 3 it would allow "(|(foo=%USER%)(bar=%USER%))", though I
> don't know if any such multiple-mention filters would ever make sense
> in a sane LDAP configuration.
> 
> Any other views from LDAP-users?

I've spent quite a bit of time integrating various bits of
non-PostgreSQL software to LDAP and in my experience option 3 tends to
be the standard.

Generally you find that you will be given the option to set the
attribute for the default search filter of the form
"(attribute=username)" which defaults to uid for UNIX-type systems and
sAMAccountName for AD. However there is always the ability to specify a
custom filter where the user is substituted via e.g. %u to cover all the
other use-cases.

As an example, I don't know if anyone would actually do this with
PostgreSQL but I've been asked on multiple occasions to configure
software so that users should be allowed to log in with either their
email address or username which is easily done with a custom LDAP filter
like "(|(mail=%u)(uid=%u))".


ATB,

Mark.


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Sun, Jul 16, 2017 at 1:08 AM, Thomas Munro  wrote:

> On Fri, Jul 14, 2017 at 11:04 PM, Magnus Hagander 
> wrote:
> > On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro
> >  wrote:
> >> A post on planet.postgresql.org today reminded me that a colleague had
> >> asked me to post this POC patch here for discussion.  It allows custom
> >> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
> >> might be to take a filter pattern with "%USERNAME%" or whatever in it.
> >> There's an existing precedent for the prefix and suffix approach, but
> >> on the other hand a pattern approach would allow filters where the
> >> username is inserted more than once.
> >
> >
> > Do we even need prefix/suffix? If we just make it "ldapsearchpattern",
> then
> > you could have something like:
> >
> > ldapsearchattribute="uid"
> > ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA
> Team)"
> >
> > We could then always to substitution of the kind:
> > (&(attr=)())
> >
> > which would in this case give:
> > (&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))
> >
> >
> > Basically we'd always AND together the username lookup with the
> additional
> > filter.
>
> Ok, so we have 3 ideas put forward:
>
> 1.  Wrap username with ldapsearchprefix ldapsearchsuffix to build
> filter (as implemented by POC patch)
> 2.  Optionally AND ldapsearchfilter with the existing
> ldapsearchattribute-based filter (Magnus's proposal)
> 3.  Pattern-based ldapsearchfilter so that %USER% is replaced with
> username (my other suggestion)
>
> The main argument for approach 1 is that it follows the style of the
> bind-only mode.
>

Agreed, but I'm not sure it's a good style to follow (and yes, I think I
may be the original author of it..). I'd rank option 3 over option 1.


>
> With idea 2, I wonder if there are some more general kinds of things
> that people might want to do that that wouldn't be possible because it
> has to include (attribute=user)... perhaps something involving a
> substring or other transformation functions (but I'm no LDAP expert,
> that may not make sense).
>

Yeah, that's exactly what I'm wondering about it :)



>
> With idea 3 it would allow "(|(foo=%USER%)(bar=%USER%))", though I
> don't know if any such multiple-mention filters would ever make sense
> in a sane LDAP configuration.
>
> Any other views from LDAP-users?
>
>

+1 for some input from people who directly use it in larger LDAP
environments. If we're going to change how it works, let's make it right
this time!

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

Re: [HACKERS] More flexible LDAP auth search filters?

[Thomas Munro]

On Fri, Jul 14, 2017 at 11:04 PM, Magnus Hagander  wrote:
> On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro
>  wrote:
>> A post on planet.postgresql.org today reminded me that a colleague had
>> asked me to post this POC patch here for discussion.  It allows custom
>> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
>> might be to take a filter pattern with "%USERNAME%" or whatever in it.
>> There's an existing precedent for the prefix and suffix approach, but
>> on the other hand a pattern approach would allow filters where the
>> username is inserted more than once.
>
>
> Do we even need prefix/suffix? If we just make it "ldapsearchpattern", then
> you could have something like:
>
> ldapsearchattribute="uid"
> ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)"
>
> We could then always to substitution of the kind:
> (&(attr=)())
>
> which would in this case give:
> (&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))
>
>
> Basically we'd always AND together the username lookup with the additional
> filter.

Ok, so we have 3 ideas put forward:

1.  Wrap username with ldapsearchprefix ldapsearchsuffix to build
filter (as implemented by POC patch)
2.  Optionally AND ldapsearchfilter with the existing
ldapsearchattribute-based filter (Magnus's proposal)
3.  Pattern-based ldapsearchfilter so that %USER% is replaced with
username (my other suggestion)

The main argument for approach 1 is that it follows the style of the
bind-only mode.

With idea 2, I wonder if there are some more general kinds of things
that people might want to do that that wouldn't be possible because it
has to include (attribute=user)... perhaps something involving a
substring or other transformation functions (but I'm no LDAP expert,
that may not make sense).

With idea 3 it would allow "(|(foo=%USER%)(bar=%USER%))", though I
don't know if any such multiple-mention filters would ever make sense
in a sane LDAP configuration.

Any other views from LDAP-users?

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


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

Re: [HACKERS] More race conditions in logical replication

[Alvaro Herrera]

After going over this a bunch more times, I found other problems.  For
example, I noticed that if I create a temporary slot in one session,
then another session would rightly go to sleep if it tries to drop that
slot.  But the slot goes away when the first session disconnects, so I'd
expect the sleeping session to get a signal and wake up, but that
doesn't happen.

I'm going to continue to look at this and report back Tuesday 18th.

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


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

Re: [HACKERS] More race conditions in logical replication

[Noah Misch]

On Wed, Jul 12, 2017 at 06:48:28PM -0400, Alvaro Herrera wrote:
> Petr Jelinek wrote:
> 
> > So best idea I could come up with is to make use of the new condition
> > variable API. That lets us wait for variable which can be set per slot.
> 
> Here's a cleaned up version of that patch, which I intend to get in the
> tree tomorrow.  I verified that this allows the subscription tests to
> pass with Tom's sleep additions.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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

Re: [HACKERS] More flexible LDAP auth search filters?

[Magnus Hagander]

On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro  wrote:

> Hi hackers,
>
> A customer asked how to use pg_hba.conf LDAP search+bind
> authentication to restrict logins to users in one of a small number of
> groups.  ldapsearchattribute only lets you make filters like
> "(foo=username)", so it couldn't be done.  Is there any reason we
> should allow a more general kind of search filter constructions?
>
> A post on planet.postgresql.org today reminded me that a colleague had
> asked me to post this POC patch here for discussion.  It allows custom
> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
> might be to take a filter pattern with "%USERNAME%" or whatever in it.
> There's an existing precedent for the prefix and suffix approach, but
> on the other hand a pattern approach would allow filters where the
> username is inserted more than once.
>

Do we even need prefix/suffix? If we just make it "ldapsearchpattern", then
you could have something like:

ldapsearchattribute="uid"
ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)"

We could then always to substitution of the kind:
(&(attr=)())

which would in this case give:
(&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))


Basically we'd always AND together the username lookup with the additional
filter.


Perhaps there are better ways to organise your LDAP servers so that
> this sort of thing isn't necessary.  I don't know.  Thoughts?
>

I think something along this way is definitely wanted. We can argue the
syntax, but being able to filter like this is definitely useful.

(FWIW, a workaround I've applied more than once to this in AD environments
(where kerberos for one reason or other can't be done, sorry Stephen) is to
set up a RADIUS server and use that one as a "middle man". But it would be
much better if we could do it natively)

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

[HACKERS] More flexible LDAP auth search filters?

[Thomas Munro]

Hi hackers,

A customer asked how to use pg_hba.conf LDAP search+bind
authentication to restrict logins to users in one of a small number of
groups.  ldapsearchattribute only lets you make filters like
"(foo=username)", so it couldn't be done.  Is there any reason we
should allow a more general kind of search filter constructions?

A post on planet.postgresql.org today reminded me that a colleague had
asked me to post this POC patch here for discussion.  It allows custom
filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
might be to take a filter pattern with "%USERNAME%" or whatever in it.
There's an existing precedent for the prefix and suffix approach, but
on the other hand a pattern approach would allow filters where the
username is inserted more than once.

Motivating example:

  ldapsearchprefix="(&(cn="
  ldapsearchsuffix = ")(|(memberof=cn=Paris DBA
Team)(memberof=cn=Tokyo DBA Team))"

Note that with this patch ldapsearchattribute=cn is equivalent to:

  ldasearchprefix="(cn="
  ldapsearchsuffix=")"

Perhaps there are better ways to organise your LDAP servers so that
this sort of thing isn't necessary.  I don't know.  Thoughts?

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


ldap-search-filters-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] More race conditions in logical replication

[Alvaro Herrera]

Petr Jelinek wrote:

> So best idea I could come up with is to make use of the new condition
> variable API. That lets us wait for variable which can be set per slot.

Here's a cleaned up version of that patch, which I intend to get in the
tree tomorrow.  I verified that this allows the subscription tests to
pass with Tom's sleep additions.

I noticed one point where we're reading the active_pid without locking;
marked it with an XXX comment.  Not yet sure whether this is a bug or
not.


I noticed something funny in CREATE_REPLICATION; apparently we first
create a slot and set it active, then we activate it by name.  With the
current coding it seems to work fine, because we're careful to make
activation idempotent, but it looks convoluted.  I don't think this is
new, though.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 55533aa3cdc2fecbf7b6b6c661649342a204e73b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Wed, 12 Jul 2017 18:38:33 -0400
Subject: [PATCH v3 1/1] Wait for slot to become free in when dropping it

---
 src/backend/replication/logical/logicalfuncs.c |  2 +-
 src/backend/replication/slot.c | 79 +++---
 src/backend/replication/slotfuncs.c|  2 +-
 src/backend/replication/walsender.c|  6 +-
 src/include/replication/slot.h |  8 ++-
 5 files changed, 69 insertions(+), 28 deletions(-)

diff --git a/src/backend/replication/logical/logicalfuncs.c 
b/src/backend/replication/logical/logicalfuncs.c
index 363ca82cb0..a3ba2b1266 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -244,7 +244,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, 
bool confirm, bool bin
else
end_of_wal = GetXLogReplayRecPtr();
 
-   ReplicationSlotAcquire(NameStr(*name));
+   ReplicationSlotAcquire(NameStr(*name), true);
 
PG_TRY();
{
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index dc7de20e11..76198a627d 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -157,6 +157,7 @@ ReplicationSlotsShmemInit(void)
/* everything else is zeroed by the memset above */
SpinLockInit(>mutex);
LWLockInitialize(>io_in_progress_lock, 
LWTRANCHE_REPLICATION_SLOT_IO_IN_PROGRESS);
+   ConditionVariableInit(>active_cv);
}
}
 }
@@ -313,24 +314,27 @@ ReplicationSlotCreate(const char *name, bool db_specific,
LWLockRelease(ReplicationSlotControlLock);
 
/*
-* Now that the slot has been marked as in_use and in_active, it's safe 
to
+* Now that the slot has been marked as in_use and active, it's safe to
 * let somebody else try to allocate a slot.
 */
LWLockRelease(ReplicationSlotAllocationLock);
+
+   ConditionVariableBroadcast(>active_cv);
 }
 
 /*
  * Find a previously created slot and mark it as used by this backend.
  */
 void
-ReplicationSlotAcquire(const char *name)
+ReplicationSlotAcquire(const char *name, bool nowait)
 {
ReplicationSlot *slot = NULL;
+   int active_pid = 0;
int i;
-   int active_pid = 0; /* Keep compiler quiet */
 
Assert(MyReplicationSlot == NULL);
 
+retry:
/* Search for the named slot and mark it active if we find it. */
LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
for (i = 0; i < max_replication_slots; i++)
@@ -339,27 +343,52 @@ ReplicationSlotAcquire(const char *name)
 
if (s->in_use && strcmp(name, NameStr(s->data.name)) == 0)
{
+   /* Found the slot we want -- can we activate it? */
SpinLockAcquire(>mutex);
+
active_pid = s->active_pid;
if (active_pid == 0)
active_pid = s->active_pid = MyProcPid;
+
SpinLockRelease(>mutex);
slot = s;
+
break;
}
}
LWLockRelease(ReplicationSlotControlLock);
 
-   /* If we did not find the slot or it was already active, error out. */
+   /* If we did not find the slot, error out. */
if (slot == NULL)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
 errmsg("replication slot \"%s\" does not 
exist", name)));
+
+   /*
+* If we found the slot but it's already active in another backend, we
+* either error out or retry after a short wait, as caller specified.
+*/
if (active_pid != MyProcPid)
-   ereport(ERROR,
- 

Re: [HACKERS] More race conditions in logical replication

[Alvaro Herrera]

Alvaro Herrera wrote:

> I'll next update this on or before Monday 10th at 19:00 CLT.

I couldn't get to this today as I wanted.  Next update on Wednesday
12th, same time.

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


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

Re: [HACKERS] More race conditions in logical replication

[Tom Lane]

Alvaro Herrera  writes:
> I'll next update this on or before Monday 10th at 19:00 CLT.

Schedule note --- we'll be wrapping beta2 on Monday, a couple hours
before that.  Although it'd be great to have this issue fixed before
beta2, jamming in a patch just a few hours before wrap is probably
not prudent.  If you can't get it done over the weekend, I'd counsel
holding off till after beta2 is tagged.

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] More race conditions in logical replication

[Alvaro Herrera]

Petr Jelinek wrote:

> So best idea I could come up with is to make use of the new condition
> variable API. That lets us wait for variable which can be set per slot.
> 
> It's not backportable however, I am not sure how big problem that is
> considering the lack of complaints until now (maybe we could backport
> using the ugly timeout version?).
> 
> The attached patch is a prototype of such solution and there are some
> race conditions (variable can get signaled before the waiting process
> starts sleeping for it). I am mainly sending it to get feedback on the
> approach.

This one looks a lot better than the original one.

I wonder if it's possible to do this using lwlocks instead of condition
variables (similar to how we do the "wait for IO" thing both for slots
and buffers).  We could backport that easily ...

I'll next update this on or before Monday 10th at 19:00 CLT.

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


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