Re: [HACKERS] logical replication: \dRp+ and "for all tables"

2017-06-09 Thread Masahiko Sawada
On Sat, Jun 10, 2017 at 7:29 AM, Jeff Janes  wrote:
> If I create a publication "for all tables", \dRp+ doesn't indicate it is for
> all tables, it just gives a list of the tables.
>
> So it doesn't distinguish between a publication specified to be for all
> tables (which will be dynamic regarding future additions), and one which
> just happens to include all the table which currently exist.
>
> That seems unfortunate.  Should the "for all tables" be included as another
> column in \dRp and \dRp+, or at least as a footnote tag in \dRp+ ?
>

+1. I was thinking the same. Attached patch adds "All Tables" column
to both \dRp and \dRp+.

Regards,

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


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

2017-06-09 Thread Vinayak Pokale
Thank you for your reply

On Jun 9, 2017 5:39 PM, "Michael Meskes"  wrote:
>
> Hi,
>
> > To develop the ECPG application more efficiently and improve
> > portability,
> > I would like to suggest one minor improvement "WHENEVER condition DO
> > CONTINUE" support in ECPG.
> > Oracle Pro*C supports WHENEVER statement with DO CONTINUE action.[1]
> >
> > EXEC SQL WHENEVER SQLERROR CONTINUE;
> > is not same as
> > EXEC SQL WHENEVER SQLERROR DO CONTINUE;
> >
> > The CONTINUE action instructs the client application to proceed to
> > the next statement whereas DO CONTINUE action instructs the client
> > application to emit a C continue statement and the flow of control
> > return to the beginning of the enclosing loop.
>
> This did actual escape me. Thanks for bringing it to our attention and
> fixing this missing functionality.
>
> > I have tried to implement it. Please check the attached patch.
> > Please give me feedback.
> > ...
>
> Could you please add a "DO CONTINUE" case to one of the test cases? Or
> add a new one? We would need a test case IMO.
>
Yes I will add test case and send updated patch.

Regards,
Vinayak Pokale


Re: [HACKERS] Notes on testing Postgres 10b1

2017-06-09 Thread Greg Stark
On 7 June 2017 at 01:01, Josh Berkus  wrote:
> P3: apparently jsonb_to_tsvector with lang parameter isn't immutable?
> This means that it can't be used for indexing:
>
> libdata=# create index bookdata_fts on bookdata using gin ((
> to_tsvector('english',bookdata)));
> ERROR:  functions in index expression must be marked IMMUTABLE

I don't have a machine handy to check on but isn't this a strange
thing to do? Isn't there a GIN opclass on jsonb itself which would be
the default if you didn't have that to_tsvector() call  -- and which
would also work properly with the jsonb operators?

-- 
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] UPDATE of partition key

2017-06-09 Thread Amit Kapila
On Fri, Jun 9, 2017 at 7:48 PM, Amit Khandekar  wrote:
> On 9 June 2017 at 19:10, Amit Kapila  wrote:
>> On Thu, Jun 8, 2017 at 10:40 PM, Robert Haas  wrote:
>>> On Thu, Jun 8, 2017 at 7:01 AM, Amit Kapila  wrote:

 I think before doing above check we can simply check if ctid.ip_blkid
 contains InvalidBlockNumber, then return an error.
>>>
>>> Hmm, OK.  That case never happens today?
>>>
>>
>> As per my understanding that case doesn't exist.  I will verify again
>> once the patch is available.  I can take a crack at it if Amit
>> Khandekar is busy with something else or is not comfortable in this
>> area.
>
> Amit, I was going to have a look at this, once I finish with the other
> part.
>

Sure, will wait for your patch to be available.  I can help by
reviewing the same.


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


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


Re: [HACKERS] logical replication NOTICE "synchronized table states"

2017-06-09 Thread Peter Eisentraut
On 6/9/17 17:06, Jeff Janes wrote:
> When creating a subscription, I get a NOTICE "synchronized table states"
> 
> What is this supposed to be telling the end user?  Is this just a
> debugging message not intended to be included in the released code?

I think we can remove this message.

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


[HACKERS] logical replication: \dRp+ and "for all tables"

2017-06-09 Thread Jeff Janes
If I create a publication "for all tables", \dRp+ doesn't indicate it is
for all tables, it just gives a list of the tables.

So it doesn't distinguish between a publication specified to be for all
tables (which will be dynamic regarding future additions), and one which
just happens to include all the table which currently exist.

That seems unfortunate.  Should the "for all tables" be included as another
column in \dRp and \dRp+, or at least as a footnote tag in \dRp+ ?

Cheers,

Jeff


[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

2017-06-09 Thread Joe Conway
On 06/09/2017 06:16 AM, Joe Conway wrote:
> On 06/08/2017 11:09 PM, Noah Misch wrote:
>> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
>>> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
>>> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
>>> > in order to demonstrate policy sorting and order of RLS/partition
>>> > constraint checking. It should be much more straight-forward now, but
>>> > let me know if there are any further recommended changes.
>>> 
>>> Thanks, will take a look towards the end of the day.
>> 
>> 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
> 
> I started reviewing the latest patch last night and will try to finish
> up this afternoon (west coast USA time).

I left the actual (2 line) code change untouched, but I tweaked the
regression test changes a bit. If there are no complaints I will push
tomorrow (Saturday).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 35ff8bb..6cd73c1 100644
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*** fireRIRrules(Query *parsetree, List *act
*** 1835,1841 
  
  		/* Only normal relations can have RLS policies */
  		if (rte->rtekind != RTE_RELATION ||
! 			rte->relkind != RELKIND_RELATION)
  			continue;
  
  		rel = heap_open(rte->relid, NoLock);
--- 1835,1842 
  
  		/* Only normal relations can have RLS policies */
  		if (rte->rtekind != RTE_RELATION ||
! 			(rte->relkind != RELKIND_RELATION &&
! 			rte->relkind != RELKIND_PARTITIONED_TABLE))
  			continue;
  
  		rel = heap_open(rte->relid, NoLock);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 7bf2936..d382a9f 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*** EXPLAIN (COSTS OFF) SELECT * FROM t1 WHE
*** 899,904 
--- 899,1332 
   Filter: f_leak(b)
  (7 rows)
  
+ --
+ -- Partitioned Tables
+ --
+ SET SESSION AUTHORIZATION regress_rls_alice;
+ CREATE TABLE part_document (
+ did int,
+ cid int,
+ dlevel  int not null,
+ dauthor name,
+ dtitle  text
+ ) PARTITION BY RANGE (cid);
+ GRANT ALL ON part_document TO public;
+ -- Create partitions for document categories
+ CREATE TABLE part_document_fiction PARTITION OF part_document FOR VALUES FROM (11) to (12);
+ CREATE TABLE part_document_satire PARTITION OF part_document FOR VALUES FROM (55) to (56);
+ CREATE TABLE part_document_nonfiction PARTITION OF part_document FOR VALUES FROM (99) to (100);
+ GRANT ALL ON part_document_fiction TO public;
+ GRANT ALL ON part_document_satire TO public;
+ GRANT ALL ON part_document_nonfiction TO public;
+ INSERT INTO part_document VALUES
+ ( 1, 11, 1, 'regress_rls_bob', 'my first novel'),
+ ( 2, 11, 2, 'regress_rls_bob', 'my second novel'),
+ ( 3, 99, 2, 'regress_rls_bob', 'my science textbook'),
+ ( 4, 55, 1, 'regress_rls_bob', 'my first satire'),
+ ( 5, 99, 2, 'regress_rls_bob', 'my history book'),
+ ( 6, 11, 1, 'regress_rls_carol', 'great science fiction'),
+ ( 7, 99, 2, 'regress_rls_carol', 'great technology book'),
+ ( 8, 55, 2, 'regress_rls_carol', 'great satire'),
+ ( 9, 11, 1, 'regress_rls_dave', 'awesome science fiction'),
+ (10, 99, 2, 'regress_rls_dave', 'awesome technology book');
+ ALTER TABLE part_document ENABLE ROW LEVEL SECURITY;
+ -- Create policy on parent
+ -- user's security level must be higher than or equal to document's
+ CREATE POLICY pp1 ON part_document AS PERMISSIVE
+ USING (dlevel <= (SELECT seclv FROM uaccount WHERE pguser = current_user));
+ -- Dave is only allowed to see cid < 55
+ CREATE POLICY pp1r ON part_document AS RESTRICTIVE TO regress_rls_dave
+ USING (cid < 55);
+ \d+ part_document
+   Table "regress_rls_schema.part_document"
+  Column  |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description 
+ -+-+---+--+-+--+--+-
+  did | integer |   |  | | plain|  | 
+  cid | integer |   |  | | plain|  | 
+  dlevel  | integer |   | not null | | plain|  | 
+  dauthor | name|   |  | | plain|  | 
+  dtitle  | text|   |  | | extended |  | 
+ Partition key: RANGE (cid)
+ Policies:
+ POLICY "pp1"
+   

Re: [HACKERS] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

2017-06-09 Thread Tom Lane
I wrote:
> Yes, we already have guards for those cases, but they return fairly opaque
> error messages to the tune of "set-valued function called in context that
> cannot accept a set", because the executor hasn't enough context to do
> better.  I'd like the messages to be more specific, like "set-valued
> function cannot appear within CASE" and so on.

Here's an expanded version of the "bottom up" patch that adjusts some
parser APIs to allow these additional messages to be thrown.  This changes
all occurrences of "set-valued function called in context that cannot
accept a set" in the core regression tests into something more specific.
There are probably some remaining cases that this doesn't cover, but the
existing execution-time checks will still catch those.

In passing, I added explicit checks that the operator doesn't return
set to transformAExprNullIf and make_distinct_op.  We used to be a bit
laissez-faire about that, expecting that the executor would throw
errors for such cases.  But now there are hard-wired assumptions that
only FuncExpr and OpExpr nodes could return sets, so we've got to make
sure that NullIfExpr and DistinctExpr nodes don't.

(In the other direction, I suspect that expression_returns_set() is now
too optimistic about believing that it needn't descend into the arguments
of nodes whose execution code used to reject SRF results.  But that's a
matter for a separate patch.)

regards, tom lane

diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index cbcd6cf..98bcfa0 100644
*** a/src/backend/catalog/information_schema.sql
--- b/src/backend/catalog/information_schema.sql
*** CREATE VIEW user_mapping_options AS
*** 2936,2947 
  SELECT authorization_identifier,
 foreign_server_catalog,
 foreign_server_name,
!CAST((pg_options_to_table(um.umoptions)).option_name AS sql_identifier) AS option_name,
 CAST(CASE WHEN (umuser <> 0 AND authorization_identifier = current_user)
 OR (umuser = 0 AND pg_has_role(srvowner, 'USAGE'))
!OR (SELECT rolsuper FROM pg_authid WHERE rolname = current_user) THEN (pg_options_to_table(um.umoptions)).option_value
   ELSE NULL END AS character_data) AS option_value
! FROM _pg_user_mappings um;
  
  GRANT SELECT ON user_mapping_options TO PUBLIC;
  
--- 2936,2949 
  SELECT authorization_identifier,
 foreign_server_catalog,
 foreign_server_name,
!CAST(opts.option_name AS sql_identifier) AS option_name,
 CAST(CASE WHEN (umuser <> 0 AND authorization_identifier = current_user)
 OR (umuser = 0 AND pg_has_role(srvowner, 'USAGE'))
!OR (SELECT rolsuper FROM pg_authid WHERE rolname = current_user)
!  THEN opts.option_value
   ELSE NULL END AS character_data) AS option_value
! FROM _pg_user_mappings um,
!  pg_options_to_table(um.umoptions) opts;
  
  GRANT SELECT ON user_mapping_options TO PUBLIC;
  
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index a35ba32..89aea2f 100644
*** a/src/backend/executor/functions.c
--- b/src/backend/executor/functions.c
*** sql_fn_post_column_ref(ParseState *pstat
*** 388,393 
--- 388,394 
  		param = ParseFuncOrColumn(pstate,
    list_make1(subfield),
    list_make1(param),
+   pstate->p_last_srf,
    NULL,
    cref->location);
  	}
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index efe1c37..5241fd2 100644
*** a/src/backend/parser/parse_agg.c
--- b/src/backend/parser/parse_agg.c
*** check_agg_arguments_walker(Node *node,
*** 705,710 
--- 705,717 
  		}
  		/* Continue and descend into subtree */
  	}
+ 	/* We can throw error on sight for a set-returning function */
+ 	if ((IsA(node, FuncExpr) &&((FuncExpr *) node)->funcretset) ||
+ 		(IsA(node, OpExpr) &&((OpExpr *) node)->opretset))
+ 		ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+  errmsg("aggregate function calls cannot contain set-returning function calls"),
+  parser_errposition(context->pstate, exprLocation(node;
  	/* We can throw error on sight for a window function */
  	if (IsA(node, WindowFunc))
  		ereport(ERROR,
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 27dd49d..3d5b208 100644
*** a/src/backend/parser/parse_clause.c
--- b/src/backend/parser/parse_clause.c
*** transformRangeFunction(ParseState *pstat
*** 572,577 
--- 572,579 
  		List	   *pair = (List *) lfirst(lc);
  		Node	   *fexpr;
  		List	   *coldeflist;
+ 		Node	   *newfexpr;
+ 		Node	   *last_srf;
  
  		/* Disassemble the function-call/column-def-list pairs */
  		Assert(list_length(pair) == 2);
*** 

Re: [HACKERS] PG10 transition tables, wCTEs and multiple operations on the same table

2017-06-09 Thread Peter Geoghegan
On Thu, Jun 8, 2017 at 3:13 PM, Robert Haas  wrote:
> On Tue, Jun 6, 2017 at 8:19 PM, Peter Geoghegan  wrote:
>> On Tue, Jun 6, 2017 at 5:01 PM, Peter Geoghegan  wrote:
>>> Also, ISTM that the code within ENRMetadataGetTupDesc() probably
>>> requires more explanation, resource management wise.
>
> Why so?  I'm not saying you're wrong, but I don't see what's confusing
> about the status quo.  I may be missing something important.

Perhaps nothing. I just thought it looked a bit odd to rely on the
target relation's TupleDesc like that. It's not quite the same as the
foreign table tuplestore stuff, because that has its own relkind, and
has a relation RTE in the parser.


-- 
Peter Geoghegan


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


Re: [HACKERS] Logical replication in the same cluster

2017-06-09 Thread Peter Eisentraut
On 4/27/17 15:06, Peter Eisentraut wrote:
> On 4/27/17 04:08, Petr Jelinek wrote:
>> Note that the workaround for all of this is not all that complex, you do
>> same thing (create slot manually) you'd do for physical replication with
>> slots.
> 
> Maybe we should just document this issue for now.

done

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


[HACKERS] logical replication NOTICE "synchronized table states"

2017-06-09 Thread Jeff Janes
When creating a subscription, I get a NOTICE "synchronized table states"

What is this supposed to be telling the end user?  Is this just a debugging
message not intended to be included in the released code?

When I first saw this NOTICE, I thought, based on the wording, that it was
telling me the data had finished copying over (i.e. the initial state of
the table data had been synced).  But at the time this notice is issued,
the copy has not even started.  In fact, if I create a subscription for a
non-existent publication, I still get this message.

Cheers,

Jeff


Re: [HACKERS] PROVE_FLAGS

2017-06-09 Thread Peter Eisentraut
On 6/5/17 15:06, Andrew Dunstan wrote:
>> In that case we're going to need to invent a way to do this similarly
>> in vcregress.pl. I'm not simply going to revert to the situation where
>> it and the makefiles are completely out of sync on this. The previous
>> patch was made more or less by ignoring the existence of vcregress.pl.
>>
>> I will look at this again probably after pgcon. I don't think it's
>> terribly urgent.
>>
> 
> 
> Here's a patch that should do that.

I'm not able to test this, but it looks like a reasonable approach.

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


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


Re: [HACKERS] logical replication busy-waiting on a lock

2017-06-09 Thread Andres Freund
On 2017-06-09 22:28:00 +0200, Petr Jelinek wrote:
> And here it is, seems better (the 0002 is same as before).

Cool, looks good on a quick scan.


>  /* Define pathname of exported-snapshot files */
>  #define SNAPSHOT_EXPORT_DIR "pg_snapshots"
> -#define XactExportFilePath(path, xid, num, suffix) \
> - snprintf(path, sizeof(path), SNAPSHOT_EXPORT_DIR "/%08X-%d%s", \
> -  xid, num, suffix)
>  
> -/* Current xact's exported snapshots (a list of Snapshot structs) */
> +/* Structure holding info about exported snapshot. */
> +typedef struct ExportedSnapshot
> +{
> + char *snapfile;
> + Snapshot snapshot;
> +} ExportedSnapshot;
> +
> +/* Current xact's exported snapshots (a list of ExportedSnapshot structs) */
>  static List *exportedSnapshots = NIL;

trival quibble: *pointers to


Will take care of it over the weekend.

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] logical replication busy-waiting on a lock

2017-06-09 Thread Petr Jelinek
On 09/06/17 03:20, Petr Jelinek wrote:
> On 09/06/17 01:06, Andres Freund wrote:
>> Hi,
>>
>> On 2017-06-03 04:50:00 +0200, Petr Jelinek wrote:
>>> One thing I don't like is the GetLastLocalTransactionId() that I had to
>>> add because we clear the proc->lxid before we get to AtEOXact_Snapshot()
>>> but I don't have better solution there.
>>
>> I dislike that quite a bit - how about we instead just change the
>> information we keep in exportedSnapshots?  I.e. store a pointer to an
>> struct ExportedSnapshot
>> {
>> char *exportedAs;
>> Snapshot snapshot;
>> }
>>
>> Then we can get rid of that relatively ugly list_length() business too.
>>
> 
> That sounds reasonable, I'll do that.
> 

And here it is, seems better (the 0002 is same as before).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From f6df47e8257cbce4a78ceeeac504c9fe86527b05 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Sat, 3 Jun 2017 02:07:49 +0200
Subject: [PATCH 2/2] Don't assign xid to logical decoding snapshots

---
 src/backend/replication/logical/snapbuild.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 8848f5b..e06aa09 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -262,7 +262,7 @@ static bool ExportInProgress = false;
 static void SnapBuildPurgeCommittedTxn(SnapBuild *builder);
 
 /* snapshot building/manipulation/distribution functions */
-static Snapshot SnapBuildBuildSnapshot(SnapBuild *builder, TransactionId xid);
+static Snapshot SnapBuildBuildSnapshot(SnapBuild *builder);
 
 static void SnapBuildFreeSnapshot(Snapshot snap);
 
@@ -463,7 +463,7 @@ SnapBuildSnapDecRefcount(Snapshot snap)
  * and ->subxip/subxcnt values.
  */
 static Snapshot
-SnapBuildBuildSnapshot(SnapBuild *builder, TransactionId xid)
+SnapBuildBuildSnapshot(SnapBuild *builder)
 {
 	Snapshot	snapshot;
 	Size		ssize;
@@ -562,7 +562,7 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	if (TransactionIdIsValid(MyPgXact->xmin))
 		elog(ERROR, "cannot build an initial slot snapshot when MyPgXact->xmin already is valid");
 
-	snap = SnapBuildBuildSnapshot(builder, GetTopTransactionId());
+	snap = SnapBuildBuildSnapshot(builder);
 
 	/*
 	 * We know that snap->xmin is alive, enforced by the logical xmin
@@ -679,7 +679,7 @@ SnapBuildGetOrBuildSnapshot(SnapBuild *builder, TransactionId xid)
 	/* only build a new snapshot if we don't have a prebuilt one */
 	if (builder->snapshot == NULL)
 	{
-		builder->snapshot = SnapBuildBuildSnapshot(builder, xid);
+		builder->snapshot = SnapBuildBuildSnapshot(builder);
 		/* increase refcount for the snapshot builder */
 		SnapBuildSnapIncRefcount(builder->snapshot);
 	}
@@ -743,7 +743,7 @@ SnapBuildProcessChange(SnapBuild *builder, TransactionId xid, XLogRecPtr lsn)
 		/* only build a new snapshot if we don't have a prebuilt one */
 		if (builder->snapshot == NULL)
 		{
-			builder->snapshot = SnapBuildBuildSnapshot(builder, xid);
+			builder->snapshot = SnapBuildBuildSnapshot(builder);
 			/* increase refcount for the snapshot builder */
 			SnapBuildSnapIncRefcount(builder->snapshot);
 		}
@@ -1061,7 +1061,7 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
 		if (builder->snapshot)
 			SnapBuildSnapDecRefcount(builder->snapshot);
 
-		builder->snapshot = SnapBuildBuildSnapshot(builder, xid);
+		builder->snapshot = SnapBuildBuildSnapshot(builder);
 
 		/* we might need to execute invalidations, add snapshot */
 		if (!ReorderBufferXidHasBaseSnapshot(builder->reorder, xid))
@@ -1831,7 +1831,7 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	{
 		SnapBuildSnapDecRefcount(builder->snapshot);
 	}
-	builder->snapshot = SnapBuildBuildSnapshot(builder, InvalidTransactionId);
+	builder->snapshot = SnapBuildBuildSnapshot(builder);
 	SnapBuildSnapIncRefcount(builder->snapshot);
 
 	ReorderBufferSetRestartPoint(builder->reorder, lsn);
-- 
2.7.4

From aaf6a332bd5010a6f4f6fd113c703a26533d5737 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Sat, 3 Jun 2017 02:06:22 +0200
Subject: [PATCH 1/2] Use virtual transaction instead of normal ones in
 exported snapshots

---
 doc/src/sgml/ref/set_transaction.sgml |   8 +--
 src/backend/storage/ipc/procarray.c   |  11 ++--
 src/backend/storage/lmgr/predicate.c  |  26 
 src/backend/utils/time/snapmgr.c  | 121 ++
 src/include/storage/predicate.h   |   4 +-
 src/include/storage/procarray.h   |   2 +-
 6 files changed, 109 insertions(+), 63 deletions(-)

diff --git a/doc/src/sgml/ref/set_transaction.sgml b/doc/src/sgml/ref/set_transaction.sgml
index ca55a5b..116315f 100644
--- a/doc/src/sgml/ref/set_transaction.sgml
+++ b/doc/src/sgml/ref/set_transaction.sgml
@@ -221,9 +221,9 @@ SET SESSION 

Re: [HACKERS] walsender & parallelism

2017-06-09 Thread Petr Jelinek
On 09/06/17 20:56, Andres Freund wrote:
> On 2017-06-08 23:04:31 -0700, Noah Misch wrote:
>> On Wed, Jun 07, 2017 at 10:54:57PM -0700, Noah Misch wrote:
>>> On Fri, Jun 02, 2017 at 11:06:29PM -0700, Andres Freund wrote:
 On 2017-06-02 22:12:46 -0700, Noah Misch wrote:
> On Fri, Jun 02, 2017 at 11:27:55PM -0400, Peter Eisentraut wrote:
>> On 5/31/17 23:54, Peter Eisentraut wrote:
>>> On 5/29/17 22:01, Noah Misch wrote:
 On Tue, May 23, 2017 at 01:45:59PM -0400, Andres Freund wrote:
> On May 23, 2017 1:42:41 PM EDT, Petr Jelinek 
>  wrote:
>> Hi,
>>
>> so this didn't really move anywhere AFAICS, do we think the approach
>> I've chosen is good or do we want to do something else here?
>
> Can you add it to the open items list?

 [Action required within three days.  This is a generic notification.]
>>>
>>> I have posted an update.  The next update will be Friday.
>>
>> Andres is now working on this.  Let's give him a bit of time. ;-)
>
> If you intended this as your soon-due status update, it is missing a 
> mandatory
> bit.

 I'm now owning this item.  I've posted patches, await review.  If none
 were to be forthcoming, I'll do another pass Monday, and commit.
>>>
>>> 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
>>
>> IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
>> for your status update.  Please reacquaint yourself with the policy on open
>> item ownership[1] and then reply immediately.  If I do not hear from you by
>> 2017-06-10 07:00 UTC, I will transfer this item to release management team
>> ownership without further notice.
> 
> The issue starting this thread is resolved, including several issues
> found in its wake, with 47fd420fb4d3e77dde960312f8672c82b14ecbad
> 6e1dd2773eb60a6ab87b27b8d9391b756e904ac3 and 
> c1abe6c786d8f00643de8519140d77644b474163
> 
> There's a remaining testing patch, and some related things left though.
> Basically patches 0001 and 0003 from
> http://archives.postgresql.org/message-id/81eaca60-3f7b-6a21-5531-fd51ffbf3f63%402ndquadrant.com
> I'd personally rather see a separate open-items entry for those, since
> this isn't related to paralellism, signal handler or such.
> 
> Petr, Peter (henceforth P^2), do you agree?

Makes sense, one could argue that 0003 could be even 2 open items with 2
patches - one with the fixes for parser and one for the other type of
message support (I am not even sure if we want to add other message
support into PG10).

-- 
  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] PG10 transition tables, wCTEs and multiple operations on the same table

2017-06-09 Thread Robert Haas
On Fri, Jun 9, 2017 at 12:00 AM, Thomas Munro
 wrote:
> On Wed, Jun 7, 2017 at 5:36 PM, Thomas Munro
>  wrote:
>> I spent a couple of hours drafting a proof-of-concept to see if my
>> hunch was right.  It seems to work correctly so far and isn't huge
>> (but certainly needs more testing and work):
>>
>>  6 files changed, 156 insertions(+), 109 deletions(-)
>
> [Adding Andrew Gierth]
>
> Here is a new version of the patch to fix transition tables with
> wCTEs, rebased on top of
> transition-tuples-from-child-tables-v10.patch[1] which must be applied
> first.
>
> This is patch 2 of a stack of 3 patches addressing currently known
> problems with transition tables.

I don't see a reason why MakeTransitionCaptureState needs to force the
tuplestores into TopTransactionContext or make them owned by
TopTransactionResourceOwner.  I mean, it was like that before, but
afterTriggers is a global variable and, potentially, there could still
be a pointer accessible through it to a tuplestore linked from it even
after the corresponding subtransaction aborted, possibly causing some
cleanup code to trip and fall over.  But that can't be used to justify
this case, because the TransitionCaptureState is only reached through
the PlanState tree; if that goes away, how is anybody going to
accidentally follow a pointer to the now-absent tuplestore?

The structure of AfterTriggerSaveEvent with this patch applied looks
pretty weird.  IIUC, whenever we enter the block guarded by if
(row_trigger), then either (a) transition_capture != NULL or (b) each
of the four "if" statements inside that block will separately decide
to do nothing.  I think now that you're requiring a transition_capture
for all instances where transition tuplestores are used, you could
simplify this code.  Maybe just change the outer "if" statement to if
(row_trigger) and then Assert(transition_capture != NULL)?

Not this patch's problem directly, but while scrutinizing this it
crossed my mind that we would need to prohibit constraint triggers
with transition tables.  It turns out that we do, in the parser:

create constraint trigger table2_trig
after insert on table2 referencing new table as new_table
for each statement execute procedure dump_insert();
ERROR:  syntax error at or near "referencing"

Possibly it would be better to allow the syntax and error out in
CreateTrigger(), so that we can give a better error message.  It's
certainly not obvious from the syntax summary produced by \h CREATE
TRIGGER why this doesn't work. This might be more future-proof, too,
if somebody someday adds support for REFERENCING { OLD | NEW } ROW to
constraint triggers and fails to realize that there's not a check
anywhere outside the parser for the table case.

I don't see anything else wrong with this, and it seems like it solves
the reported problem.

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


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


Re: [HACKERS] List of hostaddrs not supported

2017-06-09 Thread Jeff Janes
On Fri, Jun 9, 2017 at 11:52 AM, Heikki Linnakangas  wrote:

> On 06/09/2017 05:47 PM, Jeff Janes wrote:
>
>> Your commit to fix this part, 76b11e8a43eca4612d, is giving me compiler
>> warnings:
>>
>> fe-connect.c: In function 'connectDBStart':
>> fe-connect.c:1625: warning: 'ret' may be used uninitialized in this
>> function
>>
>> gcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)
>>
>
> Oh. Apparently that version of gcc doesn't take it for granted that the
> switch-statement covers all the possible cases. I've added a dummy
> initialization, to silence it. Thanks, and let me know if it didn't help.


It worked.  Thanks.

Jeff


Re: [HACKERS] walsender & parallelism

2017-06-09 Thread Andres Freund
On 2017-06-08 23:04:31 -0700, Noah Misch wrote:
> On Wed, Jun 07, 2017 at 10:54:57PM -0700, Noah Misch wrote:
> > On Fri, Jun 02, 2017 at 11:06:29PM -0700, Andres Freund wrote:
> > > On 2017-06-02 22:12:46 -0700, Noah Misch wrote:
> > > > On Fri, Jun 02, 2017 at 11:27:55PM -0400, Peter Eisentraut wrote:
> > > > > On 5/31/17 23:54, Peter Eisentraut wrote:
> > > > > > On 5/29/17 22:01, Noah Misch wrote:
> > > > > >> On Tue, May 23, 2017 at 01:45:59PM -0400, Andres Freund wrote:
> > > > > >>> On May 23, 2017 1:42:41 PM EDT, Petr Jelinek 
> > > > > >>>  wrote:
> > > > >  Hi,
> > > > > 
> > > > >  so this didn't really move anywhere AFAICS, do we think the 
> > > > >  approach
> > > > >  I've chosen is good or do we want to do something else here?
> > > > > >>>
> > > > > >>> Can you add it to the open items list?
> > > > > >>
> > > > > >> [Action required within three days.  This is a generic 
> > > > > >> notification.]
> > > > > > 
> > > > > > I have posted an update.  The next update will be Friday.
> > > > > 
> > > > > Andres is now working on this.  Let's give him a bit of time. ;-)
> > > > 
> > > > If you intended this as your soon-due status update, it is missing a 
> > > > mandatory
> > > > bit.
> > > 
> > > I'm now owning this item.  I've posted patches, await review.  If none
> > > were to be forthcoming, I'll do another pass Monday, and commit.
> > 
> > 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
> 
> IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
> for your status update.  Please reacquaint yourself with the policy on open
> item ownership[1] and then reply immediately.  If I do not hear from you by
> 2017-06-10 07:00 UTC, I will transfer this item to release management team
> ownership without further notice.

The issue starting this thread is resolved, including several issues
found in its wake, with 47fd420fb4d3e77dde960312f8672c82b14ecbad
6e1dd2773eb60a6ab87b27b8d9391b756e904ac3 and 
c1abe6c786d8f00643de8519140d77644b474163

There's a remaining testing patch, and some related things left though.
Basically patches 0001 and 0003 from
http://archives.postgresql.org/message-id/81eaca60-3f7b-6a21-5531-fd51ffbf3f63%402ndquadrant.com
I'd personally rather see a separate open-items entry for those, since
this isn't related to paralellism, signal handler or such.

Petr, Peter (henceforth P^2), do you agree?

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] List of hostaddrs not supported

2017-06-09 Thread Heikki Linnakangas

On 06/09/2017 05:47 PM, Jeff Janes wrote:

Your commit to fix this part, 76b11e8a43eca4612d, is giving me compiler
warnings:

fe-connect.c: In function 'connectDBStart':
fe-connect.c:1625: warning: 'ret' may be used uninitialized in this function

gcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)


Oh. Apparently that version of gcc doesn't take it for granted that the 
switch-statement covers all the possible cases. I've added a dummy 
initialization, to silence it. Thanks, and let me know if it didn't help.


- Heikki



--
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] List of hostaddrs not supported

2017-06-09 Thread Tom Lane
Jeff Janes  writes:
> Your commit to fix this part, 76b11e8a43eca4612d, is giving me compiler
> warnings:
> fe-connect.c: In function 'connectDBStart':
> fe-connect.c:1625: warning: 'ret' may be used uninitialized in this function

Me too ...

> gcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)

... which is not surprising since we're using the same compiler.

regards, tom lane


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


[HACKERS] HACKERS[PATCH] split ProcArrayLock into multiple parts

2017-06-09 Thread Jim Van Fleet
I left out the retry in LWLockAcquire.





ProcArrayLock_part.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] Tightening isspace() tests where behavior should match SQL parser

2017-06-09 Thread Peter Eisentraut
On 5/24/17 15:34, Tom Lane wrote:
> I wrote:
>> Heikki Linnakangas  writes:
>>> +1 for back-patching. If I understand correctly, it would change the 
>>> behavior when you pass a string with non-ASCII leading or trailing 
>>> whitespace characters to those functions. I suppose that can happen, but 
>>> it's only pure luck if the current code happens to work in that case. 
> 
>> Well, it'd work properly for e.g. no-break space in LATINn.
> 
> Actually, it's dubious that treating no-break space as whitespace is
> correct at all in these use-cases.  The core scanner would think it's
> an identifier character, so it's not impossible that somebody would
> consider it cute to write  as part of a SQL identifier.  If
> the core scanner accepts that, so must these functions.

The SQL standard might permit non-breaking spaces or similar things as
token delimiters, so it could be legitimate to look into changing that
sometime.  But in any case it should be consistent, so it's correct to
make this change now.

-- 
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] transition table behavior with inheritance appears broken

2017-06-09 Thread Andrew Gierth
> "Robert" == Robert Haas  writes:

 Robert> So, Andrew, are you running with this, or should I keep looking
 Robert> into it?

I have it; I will post a status update before 23:59 BST on 11 Jun.

-- 
Andrew (irc:RhodiumToad)


-- 
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] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Robert Haas
On Fri, Jun 9, 2017 at 1:45 PM, Peter Eisentraut
 wrote:
> On 6/9/17 12:17, Robert Haas wrote:
>> IOW, suppose there
>> were a collation API call distill() which had the property that
>> strcmp(distill(X), distill(Y)) == 0 iff X and Y are considered equal
>> under that collation.  Then, you could define your hash function as
>> hash_any(distill(X)).  Alternatively, if the collation library
>> provided its own hashing function, that would be fine too, and
>> probably faster.
>
> Isn't that what strxfrm() is?

Yeah, just with bugs.  If ICU has a non-buggy equivalent, then we can
make this work.

-- 
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] transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

2017-06-09 Thread Robert Haas
On Fri, Jun 9, 2017 at 12:19 PM, Robert Haas  wrote:
> On Thu, Jun 8, 2017 at 11:56 PM, Thomas Munro
>  wrote:
>> [Adding Andrew Gierth]
>>
>> Here is a rebased version of the patch to fix transition tables with
>> inheritance.  Fixes a typo in an error message ("not support on
>> partitions" -> "... supported ..."), and changes regression test
>> triggers to be single-event (only one of INSERT, UPDATE or DELETE),
>> because a later patch will not allow multi-event triggers with TTs.
>>
>> This is patch 1 of a stack of 3 patches addressing currently known
>> problems with transition tables.
>
> So, Andrew, are you running with this, or should I keep looking into it?

I have spent some time now studying this patch.  I might be missing
something, but to me this appears to be in great shape.  A few minor
nitpicks:

-if ((event == TRIGGER_EVENT_DELETE &&
!trigdesc->trig_delete_after_row) ||
-(event == TRIGGER_EVENT_INSERT && !trigdesc->trig_insert_after_row) ||
- (event == TRIGGER_EVENT_UPDATE && !trigdesc->trig_update_after_row))
+if (trigdesc == NULL ||
+(event == TRIGGER_EVENT_DELETE &&
!trigdesc->trig_delete_after_row) ||
+(event == TRIGGER_EVENT_INSERT &&
!trigdesc->trig_insert_after_row) ||
+(event == TRIGGER_EVENT_UPDATE &&
!trigdesc->trig_update_after_row))

I suspect the whitespace changes will get reverted by pgindent, making
them pointless.  But that's a trivial issue.

+if (mtstate->mt_transition_capture != NULL)
+{
+if (resultRelInfo->ri_TrigDesc &&
+(resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
+ resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
+{
+/*
+ * If there are any BEFORE or INSTEAD triggers on the
+ * partition, we'll have to be ready to convert their result
+ * back to tuplestore format.
+ */
+
mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL;
+mtstate->mt_transition_capture->tcs_map =
+mtstate->mt_transition_tupconv_maps[leaf_part_index];
+}
+else
+{
+/*
+ * Otherwise, just remember the original unconverted tuple, to
+ * avoid a needless round trip conversion.
+ */
+
mtstate->mt_transition_capture->tcs_original_insert_tuple = tuple;
+mtstate->mt_transition_capture->tcs_map = NULL;
+}
+}

This chunk of code gets inserted between a comment and the code to
which that comment refers.  Maybe that's not the best idea.

Some other things I thought about:

* Does has_superclass have concurrency issues?  After some
consideration, I decided that it's probably fine as long as you hold a
lock on the target relation sufficient to prevent it from concurrently
becoming an inheritance child - i.e. any lock at all.  The caller is
CREATE TRIGGER, which certainly does.

* In AfterTriggerSaveEvent, is it a problem that the large new hunk of
code ignores trigdesc if transition_capture != NULL?  If I understand
correctly, the trigdesc will be coming from the leaf relation actually
being updated, while the transition_capture will be coming from the
relation named in the query.  Is the transition_capture object
guaranteed to have all the flags set, or do we also need to include
the ones from the trigdesc?  This also seems to be fine, because of
the restriction that row-level triggers with tuplestores can't
participate in inheritance hierarchies.  We can only need to capture
the tuples for the relation named in the query, not the leaf
partitions.

* The regression tests for this function are fairly lengthy.  Given
the complexity of the behavior being tested, though, it seems like a
really good idea to have these.  Otherwise, it's easy to imagine some
future patch breaking this again.

I also like the documentation update.

So, in short, +1 from me.

Regards,

-- 
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] Typo in xlogfuncs.c [WAS Re: Incorrect mention of pg_xlog_switch() in xlogfuncs.c]

2017-06-09 Thread Peter Eisentraut
On 5/31/17 19:59, Neha Khatri wrote:
> Simplifying  $subject. There are typos in xlogfuncs.c. So Either
> 
> s/pg_xlog_switch/pg_switch_wal

fixed

-- 
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] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Peter Geoghegan
On Fri, Jun 9, 2017 at 10:45 AM, Robert Haas  wrote:
>> But they are getting the sort order they need. They just don't get the
>> equality semantics they expect.
>
> You're right.

If we happened to ever guarantee the user a stable sort, then I'd be
wrong. We don't, though.


-- 
Peter Geoghegan


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


Re: [HACKERS] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Peter Eisentraut
On 6/9/17 12:17, Robert Haas wrote:
> IOW, suppose there
> were a collation API call distill() which had the property that
> strcmp(distill(X), distill(Y)) == 0 iff X and Y are considered equal
> under that collation.  Then, you could define your hash function as
> hash_any(distill(X)).  Alternatively, if the collation library
> provided its own hashing function, that would be fine too, and
> probably faster.

Isn't that what strxfrm() is?

-- 
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] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Robert Haas
On Fri, Jun 9, 2017 at 12:18 PM, Peter Geoghegan  wrote:
> On Fri, Jun 9, 2017 at 9:17 AM, Robert Haas  wrote:
>> I'm not exactly sure what is possible or
>> desirable, but I would not be too surprised to hear complaints about
>> the observed behavior different from the "pure" ICU behavior because
>> of the tiebreak, and at least some users might even find it worth
>> giving up hashing in order to get the exact sort order they need.
>
> But they are getting the sort order they need. They just don't get the
> equality semantics they expect.

You're right.

-- 
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] partial aggregation with internal state type

2017-06-09 Thread Tom Lane
Jeff Janes  writes:
> On Fri, Jun 9, 2017 at 9:06 AM, Tom Lane  wrote:
>> The issue is how to initialize the state value to begin with.

> Why does it need to be initialized?  initializing a NULL state upon first
> use is already the job of sfunc.

Right, but for partial aggregation the combinefunc has the same kind of
role as the sfunc.

> Can't it just be left NULL if both inputs
> are NULL?  (and use serialize/deserialize to change the memory context of
> the not-NULL argument if one is NULL and one is not NULL)

Huh?  That would assume that the deserialize function is identical to what
the combinefunc would do for the first input.  I'm not sure it's even the
right abstract type signature for that.  I think you're also assuming too
much about what the combine and deserialize functions will do in terms
of memory context usage.

In the end it's not very clear to me what you hope to gain here.  It's
not like there's any chance of omitting the combinefunc altogether.
Requiring it to do something sane with a NULL initial combined state
doesn't seem like much of a burden.

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] How to refer to resource files from UDFs written in C

2017-06-09 Thread Supun Nakandala
On Fri, Jun 9, 2017 at 3:05 AM, Heikki Linnakangas  wrote:

> On 06/09/2017 08:56 AM, Supun Nakandala wrote:
>
>> Hi hackers,
>>
>> I am trying to extend PostgreSQL by adding UDT and UDF for a custom use
>> case and I am using C language extensions to do that.
>>
>> However, I have a requirement of reading a text file from one of the C
>> functions. The compiled *.so files are placed in the "pg_config
>> --pkglibdir" directory and tried copying my text files there but it didn't
>> work. I found that, when these shared libs are loaded they are run from a
>> different working directory. In this case, what is the best way to refer
>> to
>> my text files from the C code other than giving the absolute path which
>> can
>> change from system to system.
>>
>
> All backend processes run with the data directory as the current
> directory. So you can put the files into the data directory, probably best
> to have a subdirectory there to avoid confusing them with PostgreSQL's own
> files. Or you could have a config option, to set an absolute path.
>
> - Heikki
>
>
Thank you everyone for your quick replies. I ended up copying the files to
the data directory and referring using relative path.


Re: [HACKERS] partial aggregation with internal state type

2017-06-09 Thread Jeff Janes
On Fri, Jun 9, 2017 at 9:06 AM, Tom Lane  wrote:

> Jeff Janes  writes:
> > The docs for creating aggregates for 9.6 and beyond say:
> > "For aggregate functions whose state_data_type is internal, the
> combinefunc
> > must not be strict. In this case the combinefunc must ensure that null
> > states are handled correctly and that the state being returned is
> properly
> > stored in the aggregate memory context."
>
> > Since combinefunc with an internal type is only useful when serialfunc
> and
> > deserialfunc are also defined, why can't the built-in machinery just do
> the
> > right thing when faced with a strict combinefunc?
>
> The issue is how to initialize the state value to begin with.
>

Why does it need to be initialized?  initializing a NULL state upon first
use is already the job of sfunc.  Can't it just be left NULL if both inputs
are NULL?  (and use serialize/deserialize to change the memory context of
the not-NULL argument if one is NULL and one is not NULL)


Cheers,

Jeff


Re: [HACKERS] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Peter Geoghegan
On Fri, Jun 9, 2017 at 9:17 AM, Robert Haas  wrote:
> I'm not exactly sure what is possible or
> desirable, but I would not be too surprised to hear complaints about
> the observed behavior different from the "pure" ICU behavior because
> of the tiebreak, and at least some users might even find it worth
> giving up hashing in order to get the exact sort order they need.

But they are getting the sort order they need. They just don't get the
equality semantics they expect.


-- 
Peter Geoghegan


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


Re: [HACKERS] transition table behavior with inheritance appears broken (was: Declarative partitioning - another take)

2017-06-09 Thread Robert Haas
On Thu, Jun 8, 2017 at 11:56 PM, Thomas Munro
 wrote:
> [Adding Andrew Gierth]
>
> Here is a rebased version of the patch to fix transition tables with
> inheritance.  Fixes a typo in an error message ("not support on
> partitions" -> "... supported ..."), and changes regression test
> triggers to be single-event (only one of INSERT, UPDATE or DELETE),
> because a later patch will not allow multi-event triggers with TTs.
>
> This is patch 1 of a stack of 3 patches addressing currently known
> problems with transition tables.

So, Andrew, are you running with this, or should I keep looking into 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] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Robert Haas
On Fri, Jun 9, 2017 at 11:46 AM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 6/9/17 11:12, Tom Lane wrote:
>>> https://www.postgresql.org/message-id/27064.1134753...@sss.pgh.pa.us
>
>> Good to know.  That just says that if we were to go with the strcoll()
>> result only, things would work correctly.
>
> There's still the hashing problem.

Tom, that mailing list discussions is very illuminating.  Thanks for
digging it up.

Regarding the question of hashing, one way to support that would be if
we had some sort of canonicalization function.  IOW, suppose there
were a collation API call distill() which had the property that
strcmp(distill(X), distill(Y)) == 0 iff X and Y are considered equal
under that collation.  Then, you could define your hash function as
hash_any(distill(X)).  Alternatively, if the collation library
provided its own hashing function, that would be fine too, and
probably faster.

On the other hand, is there any rule that says we have to support
hashing?  Certainly, if we defined a new datatype collated_text, it
could have a btree opfamily and no hash opfamily.  It's trickier with
only one datatype, but possibly we could come up with a way for an
opfamily to be consulted about whether it is available for a given
choice of collation.  I'm not exactly sure what is possible or
desirable, but I would not be too surprised to hear complaints about
the observed behavior different from the "pure" ICU behavior because
of the tiebreak, and at least some users might even find it worth
giving up hashing in order to get the exact sort order they need.

-- 
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] partial aggregation with internal state type

2017-06-09 Thread Tom Lane
Jeff Janes  writes:
> The docs for creating aggregates for 9.6 and beyond say:
> "For aggregate functions whose state_data_type is internal, the combinefunc
> must not be strict. In this case the combinefunc must ensure that null
> states are handled correctly and that the state being returned is properly
> stored in the aggregate memory context."

> Since combinefunc with an internal type is only useful when serialfunc and
> deserialfunc are also defined, why can't the built-in machinery just do the
> right thing when faced with a strict combinefunc?

The issue is how to initialize the state value to begin with.

I suppose you're imagining that we could fill pg_aggregate.agginitval
with something that the serialfunc could read.  But that would place
serious constraints on the serialization format, like that it be
machine-independent and valid as text.

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] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Tom Lane
Peter Eisentraut  writes:
> On 6/9/17 11:12, Tom Lane wrote:
>> https://www.postgresql.org/message-id/27064.1134753...@sss.pgh.pa.us

> Good to know.  That just says that if we were to go with the strcoll()
> result only, things would work correctly.

There's still the hashing problem.

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] Something is rotten in publication drop

2017-06-09 Thread Tom Lane
Peter Eisentraut  writes:
> On 6/8/17 23:53, Tom Lane wrote:
>> ! ERROR:  publication "addr_pub" does not exist

> The \d+ command attempts to print out any publications that the relation
> is part of.  To find the publications it is part of, it runs this query:

> "SELECT pub.pubname\n"
> " FROM pg_catalog.pg_publication pub,\n"
> "  pg_catalog.pg_get_publication_tables(pub.pubname)\n"
> "WHERE relid = '%s'\n"
> "ORDER BY 1;",

> pg_get_publication_tables() calls GetPublicationByName(), which throws
> this error.

> So I suppose that if a publication is dropped between the time
> pg_publication is read and the function is called, you could get this error.

Yeah, I'd suspected as much but not tracked it down yet.

> How could we improve that?

What we've done in many comparable situations is to allow a
catalog-probing function to return NULL instead of failing
when handed an OID or other identifier that it can't locate.
Here it seems like pg_get_publication_tables() needs to use
missing_ok = TRUE and then return zero rows for a null result.
Arguably, a nonexistent publication publishes no tables, so
it's not clear that's not the Right Thing anyway.

BTW, isn't the above command a hugely inefficient way of finding
the publications for the target rel?  Unless you've got a rather
small number of rather restricted publications, seems like it's
going to take a long time.  Maybe we don't care too much about
manual invocations of \d+, but I bet somebody will carp if there's
not a better way to find this out.  Maybe a better answer is to
define a more suitable function pg_publications_for_table(relid)
and let it have the no-error-for-bad-OID behavior.

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] Fix comment in shm_mq.c

2017-06-09 Thread Masahiko Sawada
On Sat, Jun 10, 2017 at 12:40 AM, Peter Eisentraut
 wrote:
> On 6/8/17 21:58, Masahiko Sawada wrote:
>> Attached patch for $subject.
>
> committed
>

Thank you!

Regards,

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


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


Re: [HACKERS] Fix comment in shm_mq.c

2017-06-09 Thread Peter Eisentraut
On 6/8/17 21:58, Masahiko Sawada wrote:
> Attached patch for $subject.

committed

-- 
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] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Peter Eisentraut
On 6/9/17 11:12, Tom Lane wrote:
> Robert Haas  writes:
>> I have to admit that I'm still a little confused about what's actually
>> going on here.  Commit says that it "fixes inconsistent behavior under
>> glibc's hu_HU locale", but it doesn't say what sort of inconsistent
>> behavior it fixes.
> 
> Unfortunately we were not good back then about linking commits to
> list discussions, but a bit of excavation in the archives found this:
> 
> https://www.postgresql.org/message-id/27064.1134753...@sss.pgh.pa.us

Good to know.  That just says that if we were to go with the strcoll()
result only, things would work correctly.

Again, some other details to work out.

-- 
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] Something is rotten in publication drop

2017-06-09 Thread Peter Eisentraut
On 6/8/17 23:53, Tom Lane wrote:
> --- 155,161 
>   
>   SET search_path = mvtest_mvschema, public;
>   \d+ mvtest_tvm
> ! ERROR:  publication "addr_pub" does not exist
>   -- modify the underlying table data
>   INSERT INTO mvtest_t VALUES (6, 'z', 13);
>   -- confirm pre- and post-refresh contents of fairly simple materialized 
> views
> 
> This appears to have something to do with the concurrently-running
> object_address test script, which creates and then drops a publication
> named "addr_pub".  However, there is no visible connection between
> mvtest_tvm (or any of the objects it depends on) and addr_pub or any
> of the objects it is told to publish.  So what happened here, and
> isn't this a bug?

The \d+ command attempts to print out any publications that the relation
is part of.  To find the publications it is part of, it runs this query:

"SELECT pub.pubname\n"
" FROM pg_catalog.pg_publication pub,\n"
"  pg_catalog.pg_get_publication_tables(pub.pubname)\n"
"WHERE relid = '%s'\n"
"ORDER BY 1;",

pg_get_publication_tables() calls GetPublicationByName(), which throws
this error.

So I suppose that if a publication is dropped between the time
pg_publication is read and the function is called, you could get this error.

How could we improve that?

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


[HACKERS] partial aggregation with internal state type

2017-06-09 Thread Jeff Janes
The docs for creating aggregates for 9.6 and beyond say:

"For aggregate functions whose state_data_type is internal, the combinefunc
must not be strict. In this case the combinefunc must ensure that null
states are handled correctly and that the state being returned is properly
stored in the aggregate memory context."

Since combinefunc with an internal type is only useful when serialfunc and
deserialfunc are also defined, why can't the built-in machinery just do the
right thing when faced with a strict combinefunc?

Cheers,

Jeff


Re: [HACKERS] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Tom Lane
Robert Haas  writes:
> I have to admit that I'm still a little confused about what's actually
> going on here.  Commit says that it "fixes inconsistent behavior under
> glibc's hu_HU locale", but it doesn't say what sort of inconsistent
> behavior it fixes.

Unfortunately we were not good back then about linking commits to
list discussions, but a bit of excavation in the archives found this:

https://www.postgresql.org/message-id/27064.1134753...@sss.pgh.pa.us

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] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Peter Eisentraut
On 6/9/17 10:31, Robert Haas wrote:
> + * In some locales strcoll() can claim that nonidentical strings are
> + * equal.  Believing that would be bad news for a number of reasons,
> + * so we follow Perl's lead and sort "equal" strings according to
> + * strcmp().
> 
> Again, however, the reasons why believing it would be bad news are not
> enumerated.  It is merely asserted that there is more than one such
> reason.

I suspect that there were just issues that haven't been thought through
yet, including hashing.

More generally, the code's receptiveness to internationalization issues
is ever expanding.  Early code probably also thought that using
multibyte characters or non-C locales was bad news.  Over time, we have
worked those issues out.  This might be just be one more.

> So, what's special about text that it can never report two
> non-byte-for-byte values as equal?  And could we consider changing
> that, so that users can select an ICU collator and get exactly the
> behavior ICU delivers, without the extra tiebreak?

I don't think there is anything special.  We just need to work through
the details.

-- 
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] List of hostaddrs not supported

2017-06-09 Thread Jeff Janes
On Thu, Jun 8, 2017 at 1:36 AM, Heikki Linnakangas  wrote:

> While testing libpq and GSS the other day, I was surprised by the behavior
> of the host and hostaddr libpq options, if you specify a list of hostnames.
>
> I did this this, and it took me quite a while to figure out what was going
> on:
>
> $ psql "dbname=postgres hostaddr=::1  host=localhost,localhost
>> user=krbtestuser" -c "SELECT 'hello'"
>> psql: GSSAPI continuation error: Unspecified GSS failure.  Minor code may
>> provide more information
>> GSSAPI continuation error: Server postgres/localhost,localhost@PG.EXAMPLE
>> not found in Kerberos database
>>
>
> That was a pilot error; I specified a list of hostnames, but only one
> hostaddr. But I would've expected to get a more helpful error, pointing
> that out.
>
> Some thoughts on this:
>
> 1. You cannot actually specify a list of hostaddrs. Trying to do so gives
> error:
>
> psql: could not translate host name "::1,::1" to address: Name or service
>> not known
>>
>
> That error message is a bit inaccurate, in that it wasn't really a "host
> name" that it tried to translate, but a raw address in string format.
> That's even more confusing if you make the mistake that you specify
> "hostaddr=localhost":
>


Your commit to fix this part, 76b11e8a43eca4612d, is giving me compiler
warnings:

fe-connect.c: In function 'connectDBStart':
fe-connect.c:1625: warning: 'ret' may be used uninitialized in this function

gcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)


 Cheers,

Jeff


Re: [HACKERS] Proposal : For Auto-Prewarm.

2017-06-09 Thread Amit Kapila
On Fri, Jun 9, 2017 at 10:01 AM, Mithun Cy  wrote:
> I have merged Rafia's patch for cosmetic changes. I have also fixed
> some of the changes you have recommended over that. But kept few as it
> is since Rafia's opinion was needed on that.
>

Few comments on the latest patch:
1.
+ /* Check whether blocknum is valid and within fork file size. */
+ if (blk->blocknum >= nblocks)
+ {
+ /* Move to next forknum. */
+ ++pos;
+ old_blk = blk;
+ continue;
+ }
+
+ /* Prewarm buffer. */
+ buf = ReadBufferExtended(rel, blk->forknum, blk->blocknum, RBM_NORMAL,
+ NULL);
+ if (BufferIsValid(buf))
+ ReleaseBuffer(buf);
+
+ old_blk = blk;
+ ++pos;

You are incrementing position at different places in this loop. I
think you can do it once at the start of the loop.

2.
+dump_now(bool is_bgworker)
{
..
+ fd = OpenTransientFile(transient_dump_file_path,
+   O_CREAT | O_WRONLY | O_TRUNC, 0666);

+prewarm_buffer_pool(void)
{
..
+ file = AllocateFile(AUTOPREWARM_FILE, PG_BINARY_R);

During prewarm, you seem to be using binary mode to open a file
whereas during dump binary flag is not passed.  Is there a reason
for such a difference?

3.
+ ereport(LOG,
+ (errmsg("saved metadata info of %d blocks", num_blocks)));

It doesn't seem like a good idea to log this info at each dump
interval.  How about making this as a DEBUG1 message?

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


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


Re: [HACKERS] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Robert Haas
On Fri, Jun 2, 2017 at 2:22 PM, Peter Geoghegan  wrote:
> On Fri, Jun 2, 2017 at 10:34 AM, Amit Khandekar  
> wrote:
>> Ok. I was thinking we are doing the tie-breaker because specifically
>> strcoll_l() was unexpectedly returning 0 for some cases. Now I get it,
>> that we do that to be compatible with texteq().
>
> Both of these explanations are correct, in a way. See commit 656beff.

I have to admit that I'm still a little confused about what's actually
going on here.  Commit says that it "fixes inconsistent behavior under
glibc's hu_HU locale", but it doesn't say what sort of inconsistent
behavior it fixes.  It added a comment - which remains to this day -
saying this:

+ * In some locales strcoll() can claim that nonidentical strings are
+ * equal.  Believing that would be bad news for a number of reasons,
+ * so we follow Perl's lead and sort "equal" strings according to
+ * strcmp().

Again, however, the reasons why believing it would be bad news are not
enumerated.  It is merely asserted that there is more than one such
reason.

Now, it is obviously not true in general that a comparison operator
can never deem two values which are not byte-for-byte identical as
equal, because citext does exactly that (indeed, that's the point).  I
thought maybe citext could get away with it because it lacked indexing
support but, nope, it has indexing support.  Also, the in-core numeric
data type has the same property ('1.0'::numeric = '1'::numeric, but
scale() reveals that they are not byte-for-byte identical).

So, what's special about text that it can never report two
non-byte-for-byte values as equal?  And could we consider changing
that, so that users can select an ICU collator and get exactly the
behavior ICU delivers, without the extra tiebreak?

-- 
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] Fix tab-completion of ALTER SUBSCRIPTION SET PUBLICATION

2017-06-09 Thread Peter Eisentraut
On 6/6/17 23:52, Masahiko Sawada wrote:
> On Wed, Jun 7, 2017 at 12:41 PM, Peter Eisentraut
>  wrote:
>> On 6/6/17 04:17, Masahiko Sawada wrote:
>>> With this patch, ALTER SUBSCRIPTION  SET PUBLICATION  [TAB]
>>> completes with "REFRESH" and "SKIP REFRESH".
>>> Specifying either REFRESH or SKIP REFRESH is mandatory after ALTER
>>> SUBSCRIPTION SET PUBLICATION, so i think it's good to add this.
>>
>> That syntax does not exist anymore.
>>
>> You could add support for the new WITH () syntax.
>>
> 
> Sorry, I missed it.
> Attached updated version patch adds WITH() syntax.

Committed, with the addition of the copy_data option.

-- 
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] UPDATE of partition key

2017-06-09 Thread Amit Khandekar
On 9 June 2017 at 19:10, Amit Kapila  wrote:
> On Thu, Jun 8, 2017 at 10:40 PM, Robert Haas  wrote:
>> On Thu, Jun 8, 2017 at 7:01 AM, Amit Kapila  wrote:
>>> On Thu, Jun 8, 2017 at 1:33 AM, Robert Haas  wrote:
 On Wed, Jun 7, 2017 at 5:46 AM, Amit Kapila  
 wrote:
> As far as I understand, it is to ensure that for deleted rows, nothing
> more needs to be done.  For example, see the below check in
> ExecUpdate/ExecDelete.
> if (!ItemPointerEquals(tupleid, ))
> {
> ..
> }
> ..
>
> Also a similar check in ExecLockRows.  Now for deleted rows, if the
> t_ctid wouldn't point to itself, then in the mentioned functions, we
> were not in a position to conclude that the row is deleted.

 Right, so we would have to find all such checks and change them to use
 some other method to conclude that the row is deleted.  What method
 would we use?
>>>
>>> I think before doing above check we can simply check if ctid.ip_blkid
>>> contains InvalidBlockNumber, then return an error.
>>
>> Hmm, OK.  That case never happens today?
>>
>
> As per my understanding that case doesn't exist.  I will verify again
> once the patch is available.  I can take a crack at it if Amit
> Khandekar is busy with something else or is not comfortable in this
> area.

Amit, I was going to have a look at this, once I finish with the other
part. I was busy on getting that done first. But your comments/help
are always welcome.

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



-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database 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] Notes on testing Postgres 10b1

2017-06-09 Thread Peter Eisentraut
On 6/8/17 13:15, Josh Berkus wrote:
> Well, we *could* provide a system view, as we now do for archiving, and
> for the same reasons.

Which view are you referring to here?

-- 
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] Re: Alter subscription..SET - NOTICE message is coming for table which is already removed

2017-06-09 Thread Peter Eisentraut
On 6/9/17 02:07, Noah Misch wrote:
> On Tue, Jun 06, 2017 at 03:21:34PM -0400, Peter Eisentraut wrote:
>> On 6/3/17 01:04, Noah Misch wrote:
>>> On Wed, May 31, 2017 at 10:15:50PM -0400, Peter Eisentraut wrote:
 On 5/30/17 13:25, Masahiko Sawada wrote:
> I think this cause is that the relation status entry could be deleted
> by ALTER SUBSCRIPTION REFRESH before corresponding table sync worker
> starting. Attached patch fixes issues reported on this thread so far.

 This looks like a reasonable change, but it conflicts with the change
 discussed on "logical replication - still unstable after all these
 months".  I think I'll deal with that one first.
>>>
>>> The above-described topic is currently a PostgreSQL 10 open item.  You own
>>> this open item.  Please observe the policy on open item ownership and send a
>>> status update within three calendar days of this message.  Include a date 
>>> for
>>> your subsequent status update.
>>
>> I'm working on this now and will report back tomorrow.
> 
> IMMEDIATE ATTENTION REQUIRED.

Work is ongoing and progress is being made.  I will check back in on Monday.


-- 
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] Re: Alter subscription..SET - NOTICE message is coming for table which is already removed

2017-06-09 Thread Peter Eisentraut
On 5/30/17 13:25, Masahiko Sawada wrote:
> However there is one more problem here; if the relation status entry
> is deleted while corresponding table sync worker is waiting to be
> changed its status, the table sync worker can be orphaned in waiting
> status. In this case, should table sync worker check the relation
> status and exits if the relation status record gets removed? Or should
> ALTER SUBSCRIPTION update status of table sync worker to UNKNOWN?

I think this would be fixed with the attached patch.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From f34ed996af298d731551695da562d17be5d6b248 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 9 Jun 2017 09:47:52 -0400
Subject: [PATCH] Stop table sync workers when subscription relation entry is
 removed

When a table sync worker is in waiting state and the subscription table
entry is removed because of a concurrent subscription refresh, the
worker could be left orphaned.  To avoid that, explicitly stop the
worker when the pg_subscription_rel entry is removed.

Reported-by: Masahiko Sawada 
---
 src/backend/commands/subscriptioncmds.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/backend/commands/subscriptioncmds.c 
b/src/backend/commands/subscriptioncmds.c
index 49737a9042..8ec8742480 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -600,6 +600,8 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 
RemoveSubscriptionRel(sub->oid, relid);
 
+   logicalrep_worker_stop(sub->oid, relid);
+
namespace = 
get_namespace_name(get_rel_namespace(relid));
ereport(NOTICE,
(errmsg("removed subscription for table 
%s.%s",
-- 
2.13.1


-- 
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] How to refer to resource files from UDFs written in C

2017-06-09 Thread Tom Lane
Supun Nakandala  writes:
> However, I have a requirement of reading a text file from one of the C
> functions. The compiled *.so files are placed in the "pg_config
> --pkglibdir" directory and tried copying my text files there but it didn't
> work. I found that, when these shared libs are loaded they are run from a
> different working directory. In this case, what is the best way to refer to
> my text files from the C code other than giving the absolute path which can
> change from system to system.

You probably want to use get_share_path, or one of its sibling functions
in src/port/path.c.  Then your files go with the PG installation tree,
whereever it is.  There are different functions that are appropriate for
different types of files.

regards, tom lane


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


Re: [HACKERS] UPDATE of partition key

2017-06-09 Thread Amit Kapila
On Thu, Jun 8, 2017 at 10:40 PM, Robert Haas  wrote:
> On Thu, Jun 8, 2017 at 7:01 AM, Amit Kapila  wrote:
>> On Thu, Jun 8, 2017 at 1:33 AM, Robert Haas  wrote:
>>> On Wed, Jun 7, 2017 at 5:46 AM, Amit Kapila  wrote:
 As far as I understand, it is to ensure that for deleted rows, nothing
 more needs to be done.  For example, see the below check in
 ExecUpdate/ExecDelete.
 if (!ItemPointerEquals(tupleid, ))
 {
 ..
 }
 ..

 Also a similar check in ExecLockRows.  Now for deleted rows, if the
 t_ctid wouldn't point to itself, then in the mentioned functions, we
 were not in a position to conclude that the row is deleted.
>>>
>>> Right, so we would have to find all such checks and change them to use
>>> some other method to conclude that the row is deleted.  What method
>>> would we use?
>>
>> I think before doing above check we can simply check if ctid.ip_blkid
>> contains InvalidBlockNumber, then return an error.
>
> Hmm, OK.  That case never happens today?
>

As per my understanding that case doesn't exist.  I will verify again
once the patch is available.  I can take a crack at it if Amit
Khandekar is busy with something else or is not comfortable in this
area.

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


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


Re: [HACKERS] Re: Alter subscription..SET - NOTICE message is coming for table which is already removed

2017-06-09 Thread Peter Eisentraut
On 6/8/17 03:54, Masahiko Sawada wrote:
> The reproduction step is provided by tushar but I could reproduced it
> with following step.
> 
> X cluster ->
> =# select 'create table t' || generate_series(1,100) || '(c
> int);';\gexec -- create 100 tables
> =# create table t (c int);  -- create one more table
> =# create publication all_pub for all tables;
> =# create publication one_pub for table t;
> 
> Y Cluster ->
> (create the same 101 tables as well)
> =# create subscription hoge_sub connection 'host=localhost  port=5432'
> publication one_pub;
> =# alter subscription hoge_sub set publication all_pub; select
> pg_sleep(1); alter subscription hoge_sub set publication one_pub;
> *Error occurs here*

Thanks for that explanation.  I have committed the rest of your changes.
 I didn't add the log message, just exit silently, since it doesn't seem
necessary.

-- 
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] List of hostaddrs not supported

2017-06-09 Thread Robert Haas
On Fri, Jun 9, 2017 at 6:36 AM, Heikki Linnakangas  wrote:
> Right. I think it's a usability fail as it is; it certainly fooled me. We
> could make the error messages and documentation more clear. But even better
> to allow multiple host addresses, so that it works as you'd expect.

Sure, I don't have a problem with that.  I guess part of the point of
beta releases is to correct things that don't turn out to be as smart
as we thought they were, and this seems to be an example of that.

> I understand the slippery-slope argument that you might also want to have
> different usernames etc. for different hosts, but it's confusing that
> specifying a hostaddr changes the way the host-argument is interpreted. In
> the worst case, if we let that stand, someone might actually start to depend
> on that behavior. The other options don't have that issue. And hostaddr is
> much more closely tied to specifying the target to connect to, like host and
> port are.

Yeah, I'm not objecting to your changes, just telling you what my
chain of reasoning was.

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


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


[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

2017-06-09 Thread Joe Conway
On 06/08/2017 11:09 PM, Noah Misch wrote:
> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
>> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
>> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
>> > in order to demonstrate policy sorting and order of RLS/partition
>> > constraint checking. It should be much more straight-forward now, but
>> > let me know if there are any further recommended changes.
>> 
>> Thanks, will take a look towards the end of the day.
> 
> 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

I started reviewing the latest patch last night and will try to finish
up this afternoon (west coast USA time).

Joe

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



signature.asc
Description: OpenPGP digital signature


Re: [HACKERS] walsender termination error messages worse in v10

2017-06-09 Thread Peter Eisentraut
On 6/8/17 01:57, Noah Misch wrote:
> On Fri, Jun 02, 2017 at 11:51:26AM -0700, Andres Freund wrote:
>> commit 7c4f52409a8c7d85ed169bbbc1f6092274d03920
>> Author: Peter Eisentraut 
>> Date:   2017-03-23 08:36:36 -0400
>>
>> Logical replication support for initial data copy
>>
>> made walreceiver emit worse messages in v10 than before when the master
>> gets shut down.  Before 10 you'll get something like:
>>
>> 2017-06-02 11:46:07.173 PDT [16143][] LOG:  replication terminated by 
>> primary server
>> 2017-06-02 11:46:07.173 PDT [16143][] DETAIL:  End of WAL reached on 
>> timeline 1 at 0/1B7FB8C8.
>> 2017-06-02 11:46:07.173 PDT [16143][] FATAL:  could not send 
>> end-of-streaming message to primary: no COPY in progress
>>
>> the last bit is a bit superflous, but it still kinda makes sense.  Now
>> you get:
>> 2017-06-02 11:47:09.362 PDT [17061][] FATAL:  unexpected result after 
>> CommandComplete: server closed the connection unexpectedly
>> This probably means the server terminated abnormally
>> before or while processing the request.
> 
> [Action required within three days.  This is a generic notification.]

This was fixed by Andres.

-- 
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] A bug in mapping attributes in ATExecAttachPartition()

2017-06-09 Thread Ashutosh Bapat
May be we should pass a flag to predicate_implied_by() to handle NULL
behaviour for CHECK constraints. Partitioning has shown that it needs
to use predicate_implied_by() for comparing constraints and there may
be other cases that can come up in future. Instead of handling it
outside predicate_implied_by() we may want to change it under a flag.

On Fri, Jun 9, 2017 at 11:43 AM, Amit Langote
 wrote:
> On 2017/06/08 18:43, Amit Langote wrote:
>> On 2017/06/08 1:44, Robert Haas wrote:
>>> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>>>  wrote:
 In ATExecAttachPartition() there's following code

 13715 partnatts = get_partition_natts(key);
 13716 for (i = 0; i < partnatts; i++)
 13717 {
 13718 AttrNumber  partattno;
 13719
 13720 partattno = get_partition_col_attnum(key, i);
 13721
 13722 /* If partition key is an expression, must not skip
 validation */
 13723 if (!partition_accepts_null &&
 13724 (partattno == 0 ||
 13725  !bms_is_member(partattno, not_null_attrs)))
 13726 skip_validate = false;
 13727 }

 partattno obtained from the partition key is the attribute number of
 the partitioned table but not_null_attrs contains the attribute
 numbers of attributes of the table being attached which have NOT NULL
 constraint on them. But the attribute numbers of partitioned table and
 the table being attached may not agree i.e. partition key attribute in
 partitioned table may have a different position in the table being
 attached. So, this code looks buggy. Probably we don't have a test
 which tests this code with different attribute order between
 partitioned table and the table being attached. I didn't get time to
 actually construct a testcase and test it.
>>
>> There seem to be couple of bugs here:
>>
>> 1. When partition's key attributes differ in ordering from the parent,
>>predicate_implied_by() will give up due to structural inequality of
>>Vars in the expressions.  By fixing this, we can get it to return
>>'true' when it's really so.
>>
>> 2. As you said, we store partition's attribute numbers in the
>>not_null_attrs bitmap, but then check partattno (which is the parent's
>>attribute number which might differ) against the bitmap, which seems
>>like it might produce incorrect result.  If, for example,
>>predicate_implied_by() set skip_validate to true, and the above code
>>failed to set skip_validate to false where it should have, then we
>>would wrongly end up skipping the scan.  That is, rows in the partition
>>will contain null values whereas the partition constraint does not
>>allow it.  It's hard to reproduce this currently, because with
>>different ordering of attributes, predicate_refute_by() never returns
>>true (as mentioned in 1 above), so skip_validate does not need to be
>>set to false again.
>>
>> Consider this example:
>>
>> create table p (a int, b char) partition by list (a);
>>
>> create table p1 (b char not null, a int check (a in (1)));
>> insert into p1 values ('b', null);
>>
>> Note that not_null_attrs for p1 will contain 1 corresponding to column b,
>> which matches key attribute of the parent, that is 1, corresponding to
>> column a.  Hence we end up wrongly concluding that p1's partition key
>> column does not allow nulls.
>
> [ ... ]
>
>> I am working on a patch to fix the above mentioned issues and will post
>> the same no later than Friday.
>
> And here is the patch.
>
> Thanks,
> Amit



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] A bug in mapping attributes in ATExecAttachPartition()

2017-06-09 Thread Ashutosh Bapat
On Fri, Jun 9, 2017 at 10:31 AM, Amit Langote
 wrote:
> On 2017/06/08 19:25, Ashutosh Bapat wrote:
>> On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote
 I think this code could be removed entirely in light of commit
 3ec76ff1f2cf52e9b900349957b42d28128b7bc7.
>>>
>>> I am assuming you think that because now we emit IS NOT NULL constraint
>>> internally for any partition keys that do not allow null values (including
>>> all the keys of range partitions as of commit
>>> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7).  But those IS NOT NULL
>>> constraint expressions are inconclusive as far as the application of
>>> predicate_implied_by() to determine if we can skip the scan is concerned.
>>> So even if predicate_implied_by() returned 'true', we cannot conclude,
>>> just based on that result, that there are not any null values in the
>>> partition keys.
>>
>> I am not able to understand this. Are you saying that
>> predicate_implied_by() returns true even when it's not implied when
>> NOT NULL constraints are involved? That sounds like a bug in
>> predicate_implied_by(), which should be fixed instead of adding code
>> to pepper over it?
>
> No, it's not a bug of predicate_implied_by().  I meant to say
> predicate_implied_by() isn't exactly designed for ATExecAttachPartition's
> purpose, especially its treatment of IS NOT NULL constraints is not
> suitable for this application.  To prove that the table cannot contain
> NULLs when it shouldn't because of the partition constraint, we must look
> for explicit NOT NULL constraints on the partition key columns, instead of
> relying on the predicate_implied_by()'s proof.  See the following
> explanation for why that is so (or at least I think is so):
>
> There is this text in the header comment of
> predicate_implied_by_simple_clause(), which is where the individual pairs
> of expressions are compared and/or passed to operator_predicate_proof(),
> which mentions that the treatment of IS NOT NULL predicate is based on the
> assumption that 'restrictions' that are passed to predicate_implied_by()
> are a query's WHERE clause restrictions, *not* CHECK constraints that are
> checked when inserting data into a table.
>
>  * When the predicate is of the form "foo IS NOT NULL", we can conclude that
>  * the predicate is implied if the clause is a strict operator or function
>  * that has "foo" as an input.  In this case the clause must yield NULL when
>  * "foo" is NULL, which we can take as equivalent to FALSE because we know
>  * we are within an AND/OR subtree of a WHERE clause.  (Again, "foo" is
>  * already known immutable, so the clause will certainly always fail.)
>  * Also, if the clause is just "foo" (meaning it's a boolean variable),
>  * the predicate is implied since the clause can't be true if "foo" is NULL.
>
> As mentioned above, note the following part: which we can take as
> equivalent to FALSE because we know we are within an AND/OR subtree of a
> WHERE clause.
>
> Which is not what we are passing to predicate_implied_by() in
> ATExecAttachPartition().  We are passing it the table's CHECK constraint
> clauses which behave differently for the NULL result on NULL input - they
> *allow* the row to be inserted.  Which means that there will be rows with
> NULLs in the partition key, even if predicate_refuted_by() said that there
> cannot be.  We will end up *wrongly* skipping the validation scan if we
> relied on just the predicate_refuted_by()'s result.  That's why there is
> code to check for explicit NOT NULL constraints on the partition key
> columns.  If there are, it's OK then to assume that all the partition
> constraints are satisfied by existing constraints.  One more thing: if any
> partition key happens to be an expression, which there cannot be NOT NULL
> constraints for, we just give up on skipping the scan, because we don't
> have any declared knowledge about whether those keys are also non-null,
> which we want for partitions that do not accept null values.
>
> Does that make sense?

Thanks for the long explanation. I guess, this should be written in
comments somewhere in the code there. I see following comment in
ATExecAttachPartition()
--
 *
 * There is a case in which we cannot rely on just the result of the
 * proof.
 */

--

I guess, this comment should be expanded to explain what you wrote above.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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


[HACKERS] Something is rotten in publication drop

2017-06-09 Thread Tom Lane
I'm looking at this recent failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mandrill=2017-06-08%2023%3A54%3A12
which is

*** /home/nm/farm/xlc32/HEAD/pgsql.build/src/test/regress/expected/matview.out  
Thu Jun  8 23:55:50 2017
--- /home/nm/farm/xlc32/HEAD/pgsql.build/src/test/regress/results/matview.out   
Fri Jun  9 00:18:12 2017
***
*** 155,171 
  
  SET search_path = mvtest_mvschema, public;
  \d+ mvtest_tvm
!   Materialized view "mvtest_mvschema.mvtest_tvm"
!  Column |  Type   | Collation | Nullable | Default | Storage  | Stats target 
| Description 
! 
+-+---+--+-+--+--+-
!  type   | text|   |  | | extended |  
| 
!  totamt | numeric |   |  | | main |  
| 
! View definition:
!  SELECT mvtest_tv.type,
! mvtest_tv.totamt
!FROM mvtest_tv
!   ORDER BY mvtest_tv.type;
! 
  -- modify the underlying table data
  INSERT INTO mvtest_t VALUES (6, 'z', 13);
  -- confirm pre- and post-refresh contents of fairly simple materialized views
--- 155,161 
  
  SET search_path = mvtest_mvschema, public;
  \d+ mvtest_tvm
! ERROR:  publication "addr_pub" does not exist
  -- modify the underlying table data
  INSERT INTO mvtest_t VALUES (6, 'z', 13);
  -- confirm pre- and post-refresh contents of fairly simple materialized views

This appears to have something to do with the concurrently-running
object_address test script, which creates and then drops a publication
named "addr_pub".  However, there is no visible connection between
mvtest_tvm (or any of the objects it depends on) and addr_pub or any
of the objects it is told to publish.  So what happened here, and
isn't this a bug?

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] List of hostaddrs not supported

2017-06-09 Thread Heikki Linnakangas

On 06/08/2017 06:18 PM, Robert Haas wrote:

On Thu, Jun 8, 2017 at 10:50 AM, Tom Lane  wrote:

Robert Haas  writes:

It doesn't seem like a problem to me if somebody else wants to extend
it to hostaddr, though.  Whether that change belongs in v10 or v11 is
debatable.  I would object to adding this as an open item with me as
the owner because doesn't seem to me to be a must-fix issue, but I
don't mind someone else doing the work.


If you want to define multiple-hostaddrs as a future feature, that
seems fine, but I think Heikki is describing actual bugs.  The minimum
that I think needs to be done for v10 is to make libpq reject a hostaddr
string with the wrong number of entries (either different from the
host list, or different from 1).


Whatever you put in the hostaddr field - or any field other than host
and port - is one entry.  There is no notion of a list of entries in
any other field, and no attempt to split any other field on a comma or
any other symbol.  The fact that ::1,::1 looks to you like two entries
rather than a single malformed entry is just a misunderstanding on
your part, just like you'd be wrong if you thought that
password=foo,bar is a list of passwords rather than a password
containing a comma.

I think the argument is about whether I made the right decision when I
scoped the feature, not about whether there's a defect in the
implementation.


Right. I think it's a usability fail as it is; it certainly fooled me. 
We could make the error messages and documentation more clear. But even 
better to allow multiple host addresses, so that it works as you'd expect.


I understand the slippery-slope argument that you might also want to 
have different usernames etc. for different hosts, but it's confusing 
that specifying a hostaddr changes the way the host-argument is 
interpreted. In the worst case, if we let that stand, someone might 
actually start to depend on that behavior. The other options don't have 
that issue. And hostaddr is much more closely tied to specifying the 
target to connect to, like host and port are.


I propose the attached patch, to allow a comma-separated list of 
hostaddr's. It includes some refactoring of the code to parse 
comma-separated lists, as it was getting a bit repetitive. It also fixes 
a couple of minor issues in error messages, see commit message.


The patch doesn't include docs changes yet. I think we should add a new 
sub-section, at the same level as the "33.1.1.1. Keyword/Value 
Connection Strings" and "33.1.1.2. Connection URIs" sub-sections, to 
explain some of the details we've discussed here. Something like:


---
33.1.1.3 Specifying Multiple Hosts

It is possible to specify multiple hosts to connect to, so that they are 
tried in the order given. In the Keyword/Value syntax, the host, 
hostaddr, and port options accept a comma-separated list of values. The 
same number of elements should be given in each option, such that e.g. 
the first hostaddr corresponds to the first host name, the second 
hostaddr corresponds to the second host name, and so forth. As an 
exception, if only one port is specified, it applies to all the hosts.


In the connection URI syntax, you can list multiple host:port pairs 
separated by commas, in the host component of the URI.


A single hostname can also translate to multiple network addresses. A 
common example of this is that a host can have both an IPv4 and an IPv6 
address.


When multiple hosts are specified, or when a single hostname is 
translated to multiple addresses,  all the hosts and addresses will be 
tried in order, until one succeeds. If none of the hosts can be reached, 
the connection fails. If a connection is established successfully, but 
authentication fails, the remaining hosts in the list are not tried.


If a password file is used, you can have different passwords for 
different hosts. All the other connection options are the same for every 
host, it is not possible to e.g. specify a different username for 
different hosts.

---

- Heikki



0001-Allow-multiple-hostaddrs-to-go-with-multiple-hostnam.patch
Description: invalid/octet-stream

-- 
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] List of hostaddrs not supported

2017-06-09 Thread Mithun Cy
On Fri, Jun 9, 2017 at 3:22 PM, Heikki Linnakangas  wrote:
> Hmm, there is one problem with our current use of comma as a separator:
you
> cannot use a Unix-domain socket directory that has a comma in the name,
> because it's interpreted as multiple hostnames. E.g. this doesn't work:
>
> psql "host=/tmp/dir,with,commas"
>
> For hostnames, ports, and network addresses (hostaddr), a comma is not a
> problem, as it's not a valid character in any of those.
>
> I don't know if that was considered when this patch was developed. I
> couldn't find a mention of this in the archives. But in any case, that's
> quite orthogonal to the rest of this thread.

I think this was found earlier [1]. But It appeared difficult to fix
without breaking other API's behavior [2]

[1] UDS with comma in name

[2] Fix for comma in UDS path



Re: [HACKERS] List of hostaddrs not supported

2017-06-09 Thread Heikki Linnakangas

On 06/08/2017 06:39 PM, David G. Johnston wrote:

These are already failing so I'd agree that explicit rejection isn't
necessary - the question seems restricted to usability.  Though I suppose
we need to consider whether there is any problem with the current setup if
indeed our intended separator is also an allowable character - i.e., do we
want to future-proof the syntax by requiring quotes now?


Hmm, there is one problem with our current use of comma as a separator: 
you cannot use a Unix-domain socket directory that has a comma in the 
name, because it's interpreted as multiple hostnames. E.g. this doesn't 
work:


psql "host=/tmp/dir,with,commas"

For hostnames, ports, and network addresses (hostaddr), a comma is not a 
problem, as it's not a valid character in any of those.


I don't know if that was considered when this patch was developed. I 
couldn't find a mention of this in the archives. But in any case, that's 
quite orthogonal to the rest of this thread.


- Heikki



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


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

2017-06-09 Thread Michael Meskes
Hi,

> To develop the ECPG application more efficiently and improve
> portability,
> I would like to suggest one minor improvement "WHENEVER condition DO
> CONTINUE" support in ECPG.
> Oracle Pro*C supports WHENEVER statement with DO CONTINUE action.[1]
> 
> EXEC SQL WHENEVER SQLERROR CONTINUE;
> is not same as
> EXEC SQL WHENEVER SQLERROR DO CONTINUE;
> 
> The CONTINUE action instructs the client application to proceed to
> the next statement whereas DO CONTINUE action instructs the client
> application to emit a C continue statement and the flow of control
> return to the beginning of the enclosing loop.

This did actual escape me. Thanks for bringing it to our attention and
fixing this missing functionality.

> I have tried to implement it. Please check the attached patch.
> Please give me feedback.
> ...

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

Thanks

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


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


Re: [HACKERS] Surjective functional indexes

2017-06-09 Thread Konstantin Knizhnik

Attached please find rebased version of the patch.
Now "projection" attribute is used instead of surjective/injective.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 83ee7d3..b221c18 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -294,8 +294,33 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] 
 The optional WITH clause specifies storage
 parameters for the index.  Each index method has its own set of allowed
-storage parameters.  The B-tree, hash, GiST and SP-GiST index methods all
-accept this parameter:
+storage parameters. All indexes accept the following parameter:
+   
+
+   
+   
+projection
+
+ 
+   Functional index is based on on projection function: function which extract subset of its argument. 
+   In mathematic such functions are called non-injective. For injective function if any attribute used in the indexed 
+   expression is changed, then value of index expression is also changed. So to check that index is affected by the 
+   update, it is enough to check the set of changed fields. By default this parameters is assigned true value and function is considered 
+   as non-injective.
+   In this case change of any of indexed key doesn't mean that value of the function is changed. For example, for 
+   the expression expression(bookinfo-'isbn') defined
+   for column of JSON type is changed only when ISBN is changed, which rarely happen. The same is true for most
+   functional indexes. For non-injective functions, Postgres compares values of indexed expression for old and updated tuple and updates
+   index only when function results are different. It allows to eliminate index update and use HOT update.
+   But there are extra evaluations of the functions. So if function is expensive or probability that change of indexed column will not effect 
+   the function value is small, then marking index as projection may increase update speed.
+
+
+   
+   
+
+   
+ The B-tree, hash, GiST and SP-GiST index methods all accept this parameter:

 

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 6d1f22f..509c647 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -130,6 +130,15 @@ static relopt_bool boolRelOpts[] =
 	},
 	{
 		{
+			"projection",
+			"Evaluate functional index expression on update to check if its values is changed",
+			RELOPT_KIND_INDEX,
+			AccessExclusiveLock
+		},
+		true
+	},
+	{
+		{
 			"security_barrier",
 			"View acts as a row security barrier",
 			RELOPT_KIND_VIEW,
@@ -1301,7 +1310,7 @@ fillRelOptions(void *rdopts, Size basesize,
 break;
 			}
 		}
-		if (validate && !found)
+		if (validate && !found && options[i].gen->kinds != RELOPT_KIND_INDEX)
 			elog(ERROR, "reloption \"%s\" not found in parse table",
  options[i].gen->name);
 	}
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index e890e08..2be99ab 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -56,6 +56,7 @@
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
 #include "catalog/namespace.h"
+#include "catalog/index.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "storage/bufmgr.h"
@@ -73,7 +74,9 @@
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/tqual.h"
-
+#include "utils/memutils.h"
+#include "nodes/execnodes.h"
+#include "executor/executor.h"
 
 /* GUC variable */
 bool		synchronize_seqscans = true;
@@ -124,6 +127,7 @@ static bool ConditionalMultiXactIdWait(MultiXactId multi, MultiXactStatus status
 static XLogRecPtr log_heap_new_cid(Relation relation, HeapTuple tup);
 static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_modified,
 	   bool *copy);
+static bool ProjectionIsNotChanged(Relation relation, HeapTuple oldtup, HeapTuple newtup);
 
 
 /*
@@ -3533,8 +3537,6 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
 	key_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_KEY);
 	id_attrs = RelationGetIndexAttrBitmap(relation,
 		  INDEX_ATTR_BITMAP_IDENTITY_KEY);
-
-
 	block = ItemPointerGetBlockNumber(otid);
 	buffer = ReadBuffer(relation, block);
 	page = BufferGetPage(buffer);
@@ -4161,8 +4163,12 @@ l2:
 		 * changed. If the page was already full, we may have skipped checking
 		 * for index columns. If so, HOT update is possible.
 		 */
-		if (hot_attrs_checked && !bms_overlap(modified_attrs, hot_attrs))
+		if (hot_attrs_checked 
+			&& !bms_overlap(modified_attrs, hot_attrs) 
+			&& (!relation->rd_projection || ProjectionIsNotChanged(relation, , newtup)))
+		{
 			use_hot_update = true;
+		}
 	}
 	else
 	{
@@ -4199,6 

[HACKERS] ECPG: WHENEVER statement with DO CONTINUE action

2017-06-09 Thread vinayak

Hello,

To develop the ECPG application more efficiently and improve portability,
I would like to suggest one minor improvement "WHENEVER condition *DO 
CONTINUE*" support in ECPG.

Oracle Pro*C supports WHENEVER statement with DO CONTINUE action.[1]

EXEC SQL WHENEVER SQLERROR CONTINUE;
is not same as
EXEC SQL WHENEVER SQLERROR DO CONTINUE;

The CONTINUE action instructs the client application to proceed to the 
next statement whereas DO CONTINUE action instructs the client
application to emit a C continue statement and the flow of control 
return to the beginning of the enclosing loop.


I have tried to implement it. Please check the attached patch.

Please give me feedback.


[1]https://docs.oracle.com/cd/B28359_01/appdev.111/b28427/pc_09err.htm#i12340


Regards,

Vinayak Pokale
NTT Open Source Software Center



whenever-do-continue.patch
Description: binary/octet-stream

-- 
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] snapbuild woes

2017-06-09 Thread Antonin Houska
Andres Freund  wrote:

> Looking at 0001:
> - GetOldestSafeDecodingTransactionId() only guarantees to return an xid
>   safe for decoding (note how procArray->replication_slot_catalog_xmin
>   is checked), not one for the initial snapshot - so afaics this whole
>   exercise doesn't guarantee much so far.

I happen to use CreateInitDecodingContext() in an extension, so I had to think
what the new argumen exactly means (as for the incompatibility between PG
9.6.2 and 9.6.3, I suppose preprocessor directives can handle it).

One thing I'm failing to understand is: if TRUE is passed for
need_full_snapshot, then slot->effective_xmin receives the result of

GetOldestSafeDecodingTransactionId(need_full_snapshot)

but this does include "catalog xmin".

If the value returned is determined by an existing slot which has valid
effective_catalog_xmin and invalid effective_xmin (i.e. that slot only
protects catalog tables from VACUUM but not the regular ones), then the new
slot will think it (i.e. the new slot) protects even non-catalog tables, but
that's no true.

Shouldn't the xmin_horizon be computed by this call instead?

GetOldestSafeDecodingTransactionId(!need_full_snapshot);

(If so, I think "considerCatalog" argument name would be clearer than
"catalogOnly".)

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at


-- 
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] How to refer to resource files from UDFs written in C

2017-06-09 Thread Heikki Linnakangas

On 06/09/2017 08:56 AM, Supun Nakandala wrote:

Hi hackers,

I am trying to extend PostgreSQL by adding UDT and UDF for a custom use
case and I am using C language extensions to do that.

However, I have a requirement of reading a text file from one of the C
functions. The compiled *.so files are placed in the "pg_config
--pkglibdir" directory and tried copying my text files there but it didn't
work. I found that, when these shared libs are loaded they are run from a
different working directory. In this case, what is the best way to refer to
my text files from the C code other than giving the absolute path which can
change from system to system.


All backend processes run with the data directory as the current 
directory. So you can put the files into the data directory, probably 
best to have a subdirectory there to avoid confusing them with 
PostgreSQL's own files. Or you could have a config option, to set an 
absolute path.


- Heikki



--
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] strcmp() tie-breaker for identical ICU-collated strings

2017-06-09 Thread Amit Khandekar
On 2 June 2017 at 23:52, Peter Geoghegan  wrote:
> On Fri, Jun 2, 2017 at 10:34 AM, Amit Khandekar  
> wrote:
>> Ok. I was thinking we are doing the tie-breaker because specifically
>> strcoll_l() was unexpectedly returning 0 for some cases. Now I get it,
>> that we do that to be compatible with texteq().
>
> Both of these explanations are correct, in a way. See commit 656beff.
>
>> Secondly, I was also considering if ICU especially has a way to
>> customize an ICU locale by setting some attributes which dictate
>> comparison or sorting rules for a set of characters. I mean, if there
>> is such customized ICU locale defined in the system, and we use that
>> to create PG collation, I thought we might have to strictly follow
>> those rules without a tie-breaker, so as to be 100% conformant to ICU.
>> I can't come up with an example, or may there isn't one, but , say ,
>> there is a locale which is supposed to sort only by lowest comparison
>> strength (de@strength=1 ?? ). In that case, there might be many
>> characters considered equal, but PG < operator or > operator would
>> still return true for those chars.
>
> In the terminology of the Unicode collation algorithm, PostgreSQL
> "forces deterministic comparisons" [1]. There is a lot of information
> on the details of that within the UCA spec.
>
> If we ever wanted to offer a case insensitive collation feature, then
> we wouldn't necessarily have to do the equivalent of a full strxfrm()
> when hashing, at least with collations controlled by ICU. Perhaps we
> could instead use a collator whose UCOL_STRENGTH is only UCOL_PRIMARY
> to build binary sort keys, and leave the rest to a ucol_equal() call
> (within texteq()) that has the usual UCOL_STRENGTH for the underlying
> PostgreSQL collation.
>
> I don't think it would be possible to implement case insensitive
> collations by using some pre-existing ICU collation that is case
> insensitive. Instead, an implementation might directly vary collation
> strength of any given collation to achieve case insensitivity.
> PostgreSQL would know that this collation was case insensitive, so
> regular collations wouldn't need to change their
> behavior/implementation (to use ucol_equal() within texteq(), and so
> on).

Ah ok. Understood, thanks.


Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


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


[HACKERS] Re: BUG #14680: startup process on standby encounter a deadlock of TwoPhaseStateLock when redo 2PC xlog

2017-06-09 Thread Michael Paquier
On Fri, Jun 9, 2017 at 3:17 PM, Noah Misch  wrote:
> 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

I have sent an updated patch simplifying the locking here:
https://www.postgresql.org/message-id/cab7npqqthlp9gvd2242ephkobkdmd+03tsufvk3cvzsgarq...@mail.gmail.com
-- 
Michael


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


[HACKERS] Re: BUG #14680: startup process on standby encounter a deadlock of TwoPhaseStateLock when redo 2PC xlog

2017-06-09 Thread Noah Misch
On Sun, Jun 04, 2017 at 10:24:30PM +, Noah Misch wrote:
> On Thu, Jun 01, 2017 at 01:07:53AM -0700, Michael Paquier wrote:
> > On Wed, May 31, 2017 at 12:30 PM, Michael Paquier
> >  wrote:
> > > On Wed, May 31, 2017 at 6:57 AM, Tom Lane  wrote:
> > >> wangchuant...@huawei.com writes:
> > >>> startup process on standby encounter a deadlock of TwoPhaseStateLock 
> > >>> when
> > >>> redo 2PC xlog.
> > >>
> > >> Please provide an example of how to get into this state.
> > >
> > > That would help. Are you seeing in the logs something like "removing
> > > future two-phase state from memory for XXX" or "removing stale
> > > two-phase state from shared memory for XXX"?
> > >
> > > Even with that, the light-weight lock sequence taken in those code
> > > paths look definitely wrong to me, we should not take twice
> > > TwoPhaseStateLock in the same code path. I think that we should remove
> > > the lock acquisitions in RemoveGXact() and PrepareRedoRemove, and then
> > > upgrade the locks of PrescanPreparedTransactions() and
> > > StandbyRecoverPreparedTransactions() to be exclusive. We still need to
> > > keep a lock as CheckPointTwoPhase() may still be triggered by the
> > > checkpoint. Tom, what do you think?
> > 
> > Attached is what I was thinking about for reference. I just came back
> > from a long flight and I am pretty tired, so my brain may have missed
> > something. I'll take again a look at this issue on Monday, an open
> > item has been added for now.
> 
> [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] A bug in mapping attributes in ATExecAttachPartition()

2017-06-09 Thread Amit Langote
On 2017/06/08 18:43, Amit Langote wrote:
> On 2017/06/08 1:44, Robert Haas wrote:
>> On Wed, Jun 7, 2017 at 7:47 AM, Ashutosh Bapat
>>  wrote:
>>> In ATExecAttachPartition() there's following code
>>>
>>> 13715 partnatts = get_partition_natts(key);
>>> 13716 for (i = 0; i < partnatts; i++)
>>> 13717 {
>>> 13718 AttrNumber  partattno;
>>> 13719
>>> 13720 partattno = get_partition_col_attnum(key, i);
>>> 13721
>>> 13722 /* If partition key is an expression, must not skip
>>> validation */
>>> 13723 if (!partition_accepts_null &&
>>> 13724 (partattno == 0 ||
>>> 13725  !bms_is_member(partattno, not_null_attrs)))
>>> 13726 skip_validate = false;
>>> 13727 }
>>>
>>> partattno obtained from the partition key is the attribute number of
>>> the partitioned table but not_null_attrs contains the attribute
>>> numbers of attributes of the table being attached which have NOT NULL
>>> constraint on them. But the attribute numbers of partitioned table and
>>> the table being attached may not agree i.e. partition key attribute in
>>> partitioned table may have a different position in the table being
>>> attached. So, this code looks buggy. Probably we don't have a test
>>> which tests this code with different attribute order between
>>> partitioned table and the table being attached. I didn't get time to
>>> actually construct a testcase and test it.
> 
> There seem to be couple of bugs here:
> 
> 1. When partition's key attributes differ in ordering from the parent,
>predicate_implied_by() will give up due to structural inequality of
>Vars in the expressions.  By fixing this, we can get it to return
>'true' when it's really so.
> 
> 2. As you said, we store partition's attribute numbers in the
>not_null_attrs bitmap, but then check partattno (which is the parent's
>attribute number which might differ) against the bitmap, which seems
>like it might produce incorrect result.  If, for example,
>predicate_implied_by() set skip_validate to true, and the above code
>failed to set skip_validate to false where it should have, then we
>would wrongly end up skipping the scan.  That is, rows in the partition
>will contain null values whereas the partition constraint does not
>allow it.  It's hard to reproduce this currently, because with
>different ordering of attributes, predicate_refute_by() never returns
>true (as mentioned in 1 above), so skip_validate does not need to be
>set to false again.
> 
> Consider this example:
> 
> create table p (a int, b char) partition by list (a);
> 
> create table p1 (b char not null, a int check (a in (1)));
> insert into p1 values ('b', null);
> 
> Note that not_null_attrs for p1 will contain 1 corresponding to column b,
> which matches key attribute of the parent, that is 1, corresponding to
> column a.  Hence we end up wrongly concluding that p1's partition key
> column does not allow nulls.

[ ... ]

> I am working on a patch to fix the above mentioned issues and will post
> the same no later than Friday.

And here is the patch.

Thanks,
Amit
From fef05d64bfad370f79e9fc305eee3d430a8f5263 Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 8 Jun 2017 14:01:34 +0900
Subject: [PATCH] Fixes around partition constraint handling when attaching

Failure to map attribute numbers in the partition key expressions to
to partition's would cause predicate_implied_by() to unnecessarily
return 'false', in turn causing the failure to skip the validation
scan.

Conversely, failure to map partition's NOT NULL column's attribute
numbers to parent's might cause incorrect conclusion about which
columns of the partition being attached must have NOT NULL constraint
defined on them.

Rearrange code and comments a little around this area to make things
clearer.
---
 src/backend/commands/tablecmds.c  | 109 ++
 src/test/regress/expected/alter_table.out |  32 +
 src/test/regress/sql/alter_table.sql  |  31 +
 3 files changed, 113 insertions(+), 59 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b61fda9909..481bc97155 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13409,6 +13409,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
List   *childrels;
TupleConstr *attachRel_constr;
List   *partConstraint,
+  *partConstraintOrig,
   *existConstraint;
SysScanDesc scan;
ScanKeyData skey;
@@ -13574,14 +13575,24 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
 
/*
+* Adjust the generated constraint to match this 

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

2017-06-09 Thread Noah Misch
On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
> > in order to demonstrate policy sorting and order of RLS/partition
> > constraint checking. It should be much more straight-forward now, but
> > let me know if there are any further recommended changes.
> 
> Thanks, will take a look towards the end of the day.

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] Coverage improvements of src/bin/pg_basebackup and pg_receivewal --endpos

2017-06-09 Thread Michael Paquier
On Wed, Jun 7, 2017 at 9:20 AM, Michael Paquier
 wrote:
> While it is possible to tackle some of those issues independently,
> like pg_basebackup stuff, it seems to me that it would be helpful to
> make the tests more deterministic by having an --endpos option for
> pg_receivewal, similarly to what exists already in pg_recvlogical.
>
> Thoughts?

I have just played with that, and attached is a patch to implement the
so-said option with a basic set of tests, increasing code coverage of
pg_receivewal.c from 15% to 60%. I'll park that in the next CF of
September.
-- 
Michael


0001-Implement-pg_receivewal-endpos.patch
Description: Binary data

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


Re: [HACKERS] Re: Alter subscription..SET - NOTICE message is coming for table which is already removed

2017-06-09 Thread Noah Misch
On Tue, Jun 06, 2017 at 03:21:34PM -0400, Peter Eisentraut wrote:
> On 6/3/17 01:04, Noah Misch wrote:
> > On Wed, May 31, 2017 at 10:15:50PM -0400, Peter Eisentraut wrote:
> >> On 5/30/17 13:25, Masahiko Sawada wrote:
> >>> I think this cause is that the relation status entry could be deleted
> >>> by ALTER SUBSCRIPTION REFRESH before corresponding table sync worker
> >>> starting. Attached patch fixes issues reported on this thread so far.
> >>
> >> This looks like a reasonable change, but it conflicts with the change
> >> discussed on "logical replication - still unstable after all these
> >> months".  I think I'll deal with that one first.
> > 
> > The above-described topic is currently a PostgreSQL 10 open item.  You own
> > this open item.  Please observe the policy on open item ownership and send a
> > status update within three calendar days of this message.  Include a date 
> > for
> > your subsequent status update.
> 
> I'm working on this now and will report back tomorrow.

IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is past due for
your status update.  Please reacquaint yourself with the policy on open item
ownership[1] and then reply immediately.  If I do not hear from you by
2017-06-10 07:00 UTC, I will transfer this item to release management team
ownership without further notice.

[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] walsender & parallelism

2017-06-09 Thread Noah Misch
On Wed, Jun 07, 2017 at 10:54:57PM -0700, Noah Misch wrote:
> On Fri, Jun 02, 2017 at 11:06:29PM -0700, Andres Freund wrote:
> > On 2017-06-02 22:12:46 -0700, Noah Misch wrote:
> > > On Fri, Jun 02, 2017 at 11:27:55PM -0400, Peter Eisentraut wrote:
> > > > On 5/31/17 23:54, Peter Eisentraut wrote:
> > > > > On 5/29/17 22:01, Noah Misch wrote:
> > > > >> On Tue, May 23, 2017 at 01:45:59PM -0400, Andres Freund wrote:
> > > > >>> On May 23, 2017 1:42:41 PM EDT, Petr Jelinek 
> > > > >>>  wrote:
> > > >  Hi,
> > > > 
> > > >  so this didn't really move anywhere AFAICS, do we think the 
> > > >  approach
> > > >  I've chosen is good or do we want to do something else here?
> > > > >>>
> > > > >>> Can you add it to the open items list?
> > > > >>
> > > > >> [Action required within three days.  This is a generic notification.]
> > > > > 
> > > > > I have posted an update.  The next update will be Friday.
> > > > 
> > > > Andres is now working on this.  Let's give him a bit of time. ;-)
> > > 
> > > If you intended this as your soon-due status update, it is missing a 
> > > mandatory
> > > bit.
> > 
> > I'm now owning this item.  I've posted patches, await review.  If none
> > were to be forthcoming, I'll do another pass Monday, and commit.
> 
> 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

IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
for your status update.  Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately.  If I do not hear from you by
2017-06-10 07:00 UTC, I will transfer this item to release management team
ownership without further notice.

[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