Re: [PATCH] Log CSV by default

[Gilles Darold]

Le 04/12/2018 à 03:37, Michael Paquier a écrit :
> On Mon, Dec 03, 2018 at 09:20:01PM +0100, Gilles Darold wrote:
>> pgBadger is able to parse jsonlog
>> (https://github.com/michaelpq/pg_plugins/tree/master/jsonlog) output in
>> multi-process mode because it do not use an external library to parse
>> the json. In this minimalist implementation I assume that each log line
>> is a full json object. Honestly I do not have enough feedback on this
>> format to be sure that my basic jsonlog parser is enough, maybe it is
>> not. What is really annoying for a log parser is the preservation of
>> newline character in queries, if the format outputs each atomic log
>> messages on a single line each, this is ideal.
> jsonlog generates one JSON object for each elog call, and per line.
> Newlines are treated as they would for a JSON object by being replaced
> with \n within the query strings, the same way te backend handles JSON
> strings as text.

This was what I remember and this is why it is easy for pgbadger to
process these log files in multiprocess mode.


-- 
Gilles Darold

Re: Use durable_unlink for .ready and .done files for WAL segment removal

[Michael Paquier]

On Thu, Nov 29, 2018 at 03:00:42PM +, Bossart, Nathan wrote:
> +1

Okay, here is an updated patch for this stuff, which does the following:
- Check for a WAL segment if it has a ".ready" status file, an orphaned
status file is removed only on ENOENT.
- If durable_unlink fails, retry 3 times.  If there are too many
failures, the archiver gives up on the orphan status file removal.  If
the removal works correctly, the archiver moves on to the next file.

(The variable names could be better.)
--
Michael
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 844b9d1b0e..0a78003172 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -60,6 +61,7 @@
 	 * failed archiver; in seconds. */
 
 #define NUM_ARCHIVE_RETRIES 3
+#define NUM_STATUS_CLEANUP_RETRIES 3
 
 
 /* --
@@ -424,9 +426,13 @@ pgarch_ArchiverCopyLoop(void)
 	while (pgarch_readyXlog(xlog))
 	{
 		int			failures = 0;
+		int			failures_unlink = 0;
 
 		for (;;)
 		{
+			struct stat	stat_buf;
+			char		pathname[MAXPGPATH];
+
 			/*
 			 * Do not initiate any more archive commands after receiving
 			 * SIGTERM, nor after the postmaster has died unexpectedly. The
@@ -456,6 +462,44 @@ pgarch_ArchiverCopyLoop(void)
 return;
 			}
 
+			/*
+			 * In the event of a system crash, archive status files may be
+			 * left behind as their removals are not durable, cleaning up
+			 * orphan entries here is the cheapest method.  So check that
+			 * the segment trying to be archived still exists.  If it does
+			 * not, its orphan status file is removed.
+			 */
+			snprintf(pathname, MAXPGPATH, XLOGDIR "/%s", xlog);
+			if (stat(pathname, &stat_buf) != 0 && errno == ENOENT)
+			{
+char		xlogready[MAXPGPATH];
+
+StatusFilePath(xlogready, xlog, ".ready");
+if (durable_unlink(xlogready, WARNING) == 0)
+{
+	ereport(WARNING,
+			(errmsg("removed orphan archive status file %s",
+	xlogready)));
+
+	/* leave loop and move to the next status file */
+	break;
+}
+
+if (++failures_unlink >= NUM_STATUS_CLEANUP_RETRIES)
+{
+	ereport(WARNING,
+			(errmsg("failed removal of \"%s\" too many times, will try again later",
+	xlogready)));
+
+	/* give up cleanup of orphan status files */
+	return;
+}
+
+/* wait a bit before retrying */
+pg_usleep(100L);
+continue;
+			}
+
 			if (pgarch_archiveXlog(xlog))
 			{
 /* successful */


signature.asc
Description: PGP signature

RE: [Todo item] Add entry creation timestamp column to pg_stat_replication

[myungkyu.lim]

>> I have been looking at this patch more in-depth, and you missed one
>> critical thing: hot standby feedback messages also include the
>> timestamp the client used when sending the message, so if we want to
>> track the latest time when a message has been sent we should track it
>> as much as the timestamp from status update messages.
>>
>> Fixing that and updating a couple of comments and variables, I am
>> finishing with the attached.  Thoughts?

Thanks! I missed it..:(

>Another thing which is crossing my mind is if it would make sense to report
>the timestamp of the last HS feedback message and the timestamp of the last
>status update message into two separate columns.  As the point of this
>field is to help with the debugging of mostly idle systems it seems to me
>that merging both is fine, but I'd like to hear extra opinions about that.

I think purpose of this field, 
that react interval check and debugging on idle system.
So, merging both is better.
(Is 'Reply' and 'HSFeedback' worth measuring separately?)

Re: doc - improve description of default privileges

[Fabien COELHO]

I looked at the psql manpage and the HTML rendering of section 5.6 and
it all looks good to me.



Indeed, this looks great, a precise and full description of privileges
just in one place.


Pushed (with a little bit more tweaking).


Thanks for the rewrite, extensions, improvements and final push.

--
Fabien.

slow queries over information schema.tables

[Pavel Stehule]

Hi

one customer reported slow queries over information_schema.tables. There is
newer used a index over relname probably due casting to
information_schema.sql_identifier.

Slow query

select * from information_schema.tables where table_name = 'pg_class';

Usually, there is hard to fix it on application level due corporate
environment.

Regards

Pavel

Re: [Todo item] Add entry creation timestamp column to pg_stat_replication

[Michael Paquier]

On Tue, Dec 04, 2018 at 12:56:25PM +0900, Michael Paquier wrote:
> I have been looking at this patch more in-depth, and you missed one
> critical thing: hot standby feedback messages also include the timestamp
> the client used when sending the message, so if we want to track the
> latest time when a message has been sent we should track it as much as
> the timestamp from status update messages.
> 
> Fixing that and updating a couple of comments and variables, I am
> finishing with the attached.  Thoughts?
> 
> (The catversion bump is a self-reminder, don't worry about it.)

Another thing which is crossing my mind is if it would make sense to
report the timestamp of the last HS feedback message and the timestamp
of the last status update message into two separate columns.  As the
point of this field is to help with the debugging of mostly idle systems
it seems to me that merging both is fine, but I'd like to hear extra
opinions about that.
--
Michael


signature.asc
Description: PGP signature

Re: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

[Tatsuro Yamada]

At Wed, 28 Nov 2018 14:41:40 +0900, Tatsuro Yamada  wrote in 
<54bd214b-d0d3-8654-

* tab_completion_alter_index_set_statistics.patch
===
There are two problems. You can use these DDL before testing.
#create table hoge (a integer, b integer);
#create index ind_hoge on hoge (a, (a + b), (a * b));

1) Can't get column names

  # alter index ind_hoge alter column ... but can't
  # complete.

Currently the only continueable rule to the rule is SET
STATISTICS so we usually expect the number of an expression
column there. Even though we actually name every expression
column in an index, users hardly see the names. The names are in
the index column number order in your example, but what if the
name of the first column were 'foo'?
=# alter index ind_hoge2 alter column
expr   expr1  foo
We could still *guess* what is expr or exrp1 but I don't think it
helps much. (Note: foo is not usable in this context as it's a
non-expression column.)


Thanks for your comment.
We can get column name by using "\d index_name" like this:

# \d ind_hoge
Index "public.ind_hoge"
  Column |  Type   | Key? | Definition
+-+--+
  a  | integer | yes  | a
  expr   | integer | yes  | (a + b)
  expr1  | integer | yes  | (a * b)
btree, for table "public.hoge"

So, I suppose that it's easy to understand what column is an
expression column.


Yeah, actually we can find them there, but I don't think it's
popular. I suppose that most of people are unconcious of the
names since they didn't named them.  Moreover what makes me
uneasy with this is that it doesn't suggest attribute numbers,
which are firstly expected there. But anyway it is impossible
with readline since suggestions are sorted as strings. ("1",
"11", "12", "2" order, I don't think indexes often have that many
columns, though.)


Okay, I understand.

 - For now, there is no column_number column on "\d index_name" result.
   So, if tab-completion suggested column_numbers, user can't match these 
easily.

 - Suggested column_name and column_number are sorted as a string by readline.
   These are an unnatural. But this is another topic on this thread.

   Example:
 # alter index ind_hoge alter column 
 c   exprexpr1   expr10  expr11  expr2   expr3   expr4   expr5   
expr6   expr7   expr8   expr9



Of course, user will get syntax error if user chose "a" column like a
"foo" which is
non-expression column as you mentioned.
Probably, I will be able to fix the patch to get only expression
columns from the index.
Should I do that?


Nope. That's just too-much. We are already showing unusable
suggestions in other places, for example, exiting tables for
CREATE TABLE. (It is useful for me as it suggests what should be
placed there.)


I see.



I prefer to use column name instead column number because
there is no column number on \d index_name and \d+ index_name.


Some people prefer names even if they are implicitly
given. Others (including myself) prefer numbers.


So, we should better to vote to decide implementation of the tab-completion.

Which is better to use either column_names or column_numbers?
I'd like to hear opinions from others. :)






2) I expected column names for column numbers after "SET STATISTICS",
but
   tab-completion gave schema names

  # alter index ind_hoge alter column expr SET STATISTICS 
  information_schema.  pg_catalog.  pg_temp_1.  pg_toast.
  pg_toast_temp_1.  public.

This is the result of STATISTICS  completion. SET
STATISTICS always doesn't take statistics name so this is safe.


:)


So I think it is reasonable.


Thanks.


Regards,
Tatsuro Yamada

Re: Making all nbtree entries unique by having heap TIDs participate in comparisons

[Peter Geoghegan]

On Mon, Dec 3, 2018 at 7:10 PM Peter Geoghegan  wrote:
> Attached is v9, which does things that way. There are no interesting
> changes, though I have set things up so that a later patch in the
> series can add "dynamic prefix truncation" -- I do not include any
> such patch in v9, though. I'm going to start a new thread on that
> topic, and include the patch there, since it's largely unrelated to
> this work, and in any case still isn't in scope for Postgres 12 (the
> patch is still experimental, for reasons that are of general
> interest).

The dynamic prefix truncation thread that I started:

https://postgr.es/m/cah2-wzn_nayk4pr0hrwo0stwhmxjp5qyu+x8vppt030xpqr...@mail.gmail.com
-- 
Peter Geoghegan

RE: idle-in-transaction timeout error does not give a hint

[Ideriha, Takeshi]

>From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
>Subject: Re: idle-in-transaction timeout error does not give a hint
>
>>Alternative HINT message would be something like:
>>
>>HINT: In a moment you should be able to reconnect to the
>>  database and restart your transaction.
>>
>>This could make the meaning of the error (transaction aborted)
>>cleaner and might give a better suggestion to the user.
>
> Agreed. Changing "command" to "transaction" seems more accurate.
> People might think only the command they hit is not sent but
> transaction is still alive though it's of course unnatural that
> transaction is alive after
>>>connection is terminated.
>>
>>
>>>Please find attached patch which addresses the point above.
>> Thank you for the update. It looks good to me on this point.
>> Are you planning to change other similar messages?
>
>No, because this is the only message related to an explicit transaction.  Other
>messages are not related to explicit transactions, so current messages look ok 
>to me.

Sure. I didn't have a strong opinion about it, so it's ok.

Regards,
Takeshi Ideriha

Re: [Todo item] Add entry creation timestamp column to pg_stat_replication

[Michael Paquier]

On Fri, Nov 30, 2018 at 05:54:15PM +0900, Michael Paquier wrote:
> Looks pretty to me at quick glance, unfortunately I have not spent much
> time on it, particularly testing it.
> 
> +
> + reply_time
> + timestamp with time zone
> + Send time of last message received from WAL
> receiver
> +
> This is a bit confusing as this counts for the last *standby* message
> ('r'), and not the last feedback message ('h').
> 
> +   /*
> +* timestamp of the latest message, reported by standby server
> +   */
> +   TimestampTz replyTime;
> 
> A small indentation problem, not a big deal.

I have been looking at this patch more in-depth, and you missed one
critical thing: hot standby feedback messages also include the timestamp
the client used when sending the message, so if we want to track the
latest time when a message has been sent we should track it as much as
the timestamp from status update messages.

Fixing that and updating a couple of comments and variables, I am
finishing with the attached.  Thoughts?

(The catversion bump is a self-reminder, don't worry about it.)
--
Michael
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 7aada14417..22e93019a7 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1920,6 +1920,11 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i

  
 
+
+ reply_time
+ timestamp with time zone
+ Send time of last message received from standby server
+


   
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 715995dd88..8630542bb3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -734,7 +734,8 @@ CREATE VIEW pg_stat_replication AS
 W.flush_lag,
 W.replay_lag,
 W.sync_priority,
-W.sync_state
+W.sync_state,
+W.reply_time
 FROM pg_stat_get_activity(NULL) AS S
 JOIN pg_stat_get_wal_senders() AS W ON (S.pid = W.pid)
 LEFT JOIN pg_authid AS U ON (S.usesysid = U.oid);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 46edb525e8..11bdc741f0 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1763,6 +1763,8 @@ ProcessStandbyReplyMessage(void)
 applyLag;
 	bool		clearLagTimes;
 	TimestampTz now;
+	TimestampTz	replyTime;
+	char	   *replyTimeStr;
 
 	static bool fullyAppliedLastTime = false;
 
@@ -1770,14 +1772,20 @@ ProcessStandbyReplyMessage(void)
 	writePtr = pq_getmsgint64(&reply_message);
 	flushPtr = pq_getmsgint64(&reply_message);
 	applyPtr = pq_getmsgint64(&reply_message);
-	(void) pq_getmsgint64(&reply_message);	/* sendTime; not used ATM */
+	replyTime = pq_getmsgint64(&reply_message);
 	replyRequested = pq_getmsgbyte(&reply_message);
 
-	elog(DEBUG2, "write %X/%X flush %X/%X apply %X/%X%s",
+	/* Copy because timestamptz_to_str returns a static buffer */
+	replyTimeStr = pstrdup(timestamptz_to_str(replyTime));
+
+	elog(DEBUG2, "write %X/%X flush %X/%X apply %X/%X%s reply_time %s",
 		 (uint32) (writePtr >> 32), (uint32) writePtr,
 		 (uint32) (flushPtr >> 32), (uint32) flushPtr,
 		 (uint32) (applyPtr >> 32), (uint32) applyPtr,
-		 replyRequested ? " (reply requested)" : "");
+		 replyRequested ? " (reply requested)" : "",
+		 replyTimeStr);
+
+	pfree(replyTimeStr);
 
 	/* See if we can compute the round-trip lag for these positions. */
 	now = GetCurrentTimestamp();
@@ -1824,6 +1832,7 @@ ProcessStandbyReplyMessage(void)
 			walsnd->flushLag = flushLag;
 		if (applyLag != -1 || clearLagTimes)
 			walsnd->applyLag = applyLag;
+		walsnd->replyTime = replyTime;
 		SpinLockRelease(&walsnd->mutex);
 	}
 
@@ -1927,23 +1936,43 @@ ProcessStandbyHSFeedbackMessage(void)
 	uint32		feedbackEpoch;
 	TransactionId feedbackCatalogXmin;
 	uint32		feedbackCatalogEpoch;
+	TimestampTz	replyTime;
+	char	   *replyTimeStr;
 
 	/*
 	 * Decipher the reply message. The caller already consumed the msgtype
 	 * byte. See XLogWalRcvSendHSFeedback() in walreceiver.c for the creation
 	 * of this message.
 	 */
-	(void) pq_getmsgint64(&reply_message);	/* sendTime; not used ATM */
+	replyTime = pq_getmsgint64(&reply_message);
 	feedbackXmin = pq_getmsgint(&reply_message, 4);
 	feedbackEpoch = pq_getmsgint(&reply_message, 4);
 	feedbackCatalogXmin = pq_getmsgint(&reply_message, 4);
 	feedbackCatalogEpoch = pq_getmsgint(&reply_message, 4);
 
-	elog(DEBUG2, "hot standby feedback xmin %u epoch %u, catalog_xmin %u epoch %u",
+	/* Copy because timestamptz_to_str returns a static buffer */
+	replyTimeStr = pstrdup(timestamptz_to_str(replyTime));
+
+	elog(DEBUG2, "hot standby feedback xmin %u epoch %u, catalog_xmin %u epoch %u reply_time %s",
 		 feedbackXmin,
 		 feedbackEpoch,
 		 feedbackCatalogXmin,
-		 feedbackCatalogEpoch);
+		 feedbackCatalogEpoch,
+		 replyTimeStr);
+
+	pfree(replyTimeStr);
+
+	/*
+	 * Up

Re: Index Skip Scan

[Peter Geoghegan]

On Wed, Nov 21, 2018 at 12:55 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
> Well, no, it's callled with ScanDirectionIsForward(dir). But as far as I
> remember from the previous discussions the entire topic of backward scan is
> questionable for this patch, so I'll try to invest some time in it.

Another thing that I think is related to skip scans that you should be
aware of is dynamic prefix truncation, which I started a thread on
just now [1]. While I see one big problem with the POC patch I came up
with, I think that that optimization is going to be something that
ends up happening at some point. Repeatedly descending a B-Tree when
the leading column is very low cardinality can be made quite a lot
less expensive by dynamic prefix truncation. Actually, it's almost a
perfect case for it.

I'm not asking anybody to do anything with that information. "Big
picture" thinking seems particularly valuable when working on the
B-Tree code; I don't want anybody to miss a possible future
opportunity.

[1] 
https://postgr.es/m/cah2-wzn_nayk4pr0hrwo0stwhmxjp5qyu+x8vppt030xpqr...@mail.gmail.com
-- 
Peter Geoghegan

Dynamic prefix truncation for nbtree, Lanin & Shasha design issue

[Peter Geoghegan]

Attached is a prototype patch for dynamic prefix truncation that
applies on top of v9 of the nbtree patch series [1] I've been working
on. This results in considerable performance improvements in some
cases, since it's (almost!) safe to skip attributes that we know must
be redundant/equal based on our descent of the B-Tree so far -- we can
reason about that without any extra comparison work. There are
problems, though, which hint at an underlying issue with the broader
nbtree design, which is what I really want to talk about. This thread
may not go anywhere, but I feel an obligation to put my thoughts on
how we do the Lanin & Shasha deletion stuff on the record, available
from the archives. I think that there may be some wisdom in the L&S
design that was overlooked.

First, I will say a bit more about the merits of the dynamic prefix
truncation patch, to establish the practical relevance of what I'm
about to go into. I'll show an improvement in bulk insertion
performance for the UK land registry data [2]. There is one entry in
the table for every property sale in the UK going back 2 or 3 decades
-- that's about 21.5 million rows. I have an index defined against a
3GB table, where most cycles are spent during insertions:

pg@land:5432 [25978]=# \d composite
  Unlogged index "public.composite"
  Column  │ Type │ Key? │ Definition
──┼──┼──┼
 county   │ text │ yes  │ county
 city │ text │ yes  │ city
 locality │ text │ yes  │ locality
btree, for table "public.land2"

The total index size is 1101 MB after a CREATE INDEX. As you'd
imagine, this is a medium to low cardinality index, because there are
naturally quite a lot of individual property sales in each locality,
especially those in and around London. The query "INSERT INTO land2
SELECT * FROM land_registry_price_paid_uk" does a bulk insertion into
the unlogged table land2. Here is the impact on throughput/overall
duration:

Duration on master branch (best of 3): 01:17.165

Duration with just v9 of my patch (best of 3): 01:12.736

Duration with v9 + dynamic prefix truncation (best of 3): 01:04.038

Clearly dynamic prefix truncation adds quite a lot here -- a serial
REINDEX takes about 45 - 50 seconds on the same machine. Note that I'm
not cherry-picking my test-case; I've seen similar improvements while
bulk loading a TPC-E database in a similar manner. Clearly this is a
pretty good optimization, that complements the suffix truncation stuff
well. Also, the skip scan patch could make good use of this, since
that repeatedly descends a tree with a low cardinality leading
attribute. The optimization would be particularly helpful there.

Problem
===

I don't think this latest experimental patch can go anywhere for the
foreseeable future, even though the idea is workable. There is an
_incredibly_ subtle race condition. I want to talk about why this is,
since it seems like it's down to a design decision, which likely has
broad implications:

"""
(Note: Lanin and Shasha prefer to make the key space move left, but their
argument for doing so hinges on not having left-links, which we have
anyway.  So we simplify the algorithm by moving key space right.)
"""

(I'm quoting the nbtree README.)

IOW, nbtree page deletion is not the precise opposite of a page split,
unlike an implementation that sticks closely to what Lanin & Shasha
describe: a concurrent would-be insertion into a deleted page ends up
inserting into the deletion target's right sibling, rather than the
deletion target's left sibling. This nbtree README sentence seems
misleading to me. While the README is technically correct to say L&S
don't have left links, they do have something called "outlinks", which
seem like a special case of left link to me [3].

Comparing their "procedure move-right()" pseudo-code on page 16 to our
own _bt_moveright() routine is informative: you see that they
distinguish between an ignorable/half-dead page and a page that just
had a concurrent page split in a way that we clearly don't. We only
ever 1) stay put, or 2) move right on concurrent split or concurrent
deletion. They either 1) stay put, 2) move right when the high key is
less than the value being searched for, or 3) follow the "outlink"
when the page was marked ignorable (half-dead or dead) by a concurrent
deletion.

I think that the nbtree README means that nbtree does what you see in
L&S's "Figure 3. (b)", despite the fact that Lanin & Shasha find it
"inconvenient". L&S also say of Fig 3. (b):

"""
Let us consider various implementations of merging a node n with its
right neighbor n' in the B-link structure. If we move the data in n'
to n (Fig. 3a), there is no path that processes can follow from n' to
the data moved out. This may, for example, lead a process to conclude
that c is not in the structure.' If we move the data in n to n' (Fig.
3b), we will need to access the left neighbor of n to remove n from
the linked list. Finding the left neighbor requires either much time
or a doubly 

Re: [HACKERS] CLUSTER command progress monitor

[Michael Paquier]

On Mon, Dec 03, 2018 at 02:17:25PM -0300, Alvaro Herrera wrote:
> I think we should mark it as Returned with Feedback then.

+1.
--
Michael


signature.asc
Description: PGP signature

Re: error message when subscription target is a partitioned table

[Amit Langote]

On 2018/12/04 11:23, Michael Paquier wrote:
> On Tue, Dec 04, 2018 at 10:51:40AM +0900, Amit Langote wrote:
>> Okay, here is a patch.  I didn't find any tests in subscription.sql
>> related to unsupported relkinds, so didn't bother adding one for this case
>> either.
> 
> Should we care about other relkinds as well?

Maybe, foreign tables?  I'd think that code wouldn't care about explictly
handling indexes, view, sequences, toast tables, etc.

Thanks,
Amit

Re: [PATCH] Log CSV by default

[Michael Paquier]

On Mon, Dec 03, 2018 at 09:20:01PM +0100, Gilles Darold wrote:
> pgBadger is able to parse jsonlog
> (https://github.com/michaelpq/pg_plugins/tree/master/jsonlog) output in
> multi-process mode because it do not use an external library to parse
> the json. In this minimalist implementation I assume that each log line
> is a full json object. Honestly I do not have enough feedback on this
> format to be sure that my basic jsonlog parser is enough, maybe it is
> not. What is really annoying for a log parser is the preservation of
> newline character in queries, if the format outputs each atomic log
> messages on a single line each, this is ideal.

jsonlog generates one JSON object for each elog call, and per line.
Newlines are treated as they would for a JSON object by being replaced
with \n within the query strings, the same way te backend handles JSON
strings as text.
--
Michael


signature.asc
Description: PGP signature

Re: error message when subscription target is a partitioned table

[Michael Paquier]

On Tue, Dec 04, 2018 at 10:51:40AM +0900, Amit Langote wrote:
> Okay, here is a patch.  I didn't find any tests in subscription.sql
> related to unsupported relkinds, so didn't bother adding one for this case
> either.

Should we care about other relkinds as well?
--
Michael


signature.asc
Description: PGP signature

Re: error message when subscription target is a partitioned table

[Amit Langote]

On 2018/12/03 17:51, Magnus Hagander wrote:
> On Mon, Dec 3, 2018 at 7:39 AM Tatsuo Ishii  wrote:
>>> Could we improve the error message that's output when the subscription
>>> target relation is a partitioned table?  Currently, we get:
>>>
>>> ERROR:  logical replication target relation "public.foo" is not a table
>>>
>>> I think it'd be more helpful to get:
>>>
>>> ERROR: "public.foo" is a partitioned table
>>> DETAIL: Partitioned tables are not supported as logical replication
>> targets
>>>
>>> Thoughts?
>>
>> +1
> 
> +1 as well. That is definitely confusing -- to most people, that is still a
> table...

Okay, here is a patch.  I didn't find any tests in subscription.sql
related to unsupported relkinds, so didn't bother adding one for this case
either.

Thanks,
Amit
diff --git a/src/backend/executor/execReplication.c 
b/src/backend/executor/execReplication.c
index 5bd3bbc35e..095f3be54d 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -608,6 +608,14 @@ void
 CheckSubscriptionRelkind(char relkind, const char *nspname,
 const char *relname)
 {
+   /* Give more specific error for partitioned tables */
+   if (relkind == RELKIND_PARTITIONED_TABLE)
+   ereport(ERROR,
+   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s.%s\" is a partitioned table",
+   nspname, relname),
+errdetail("Partitioned tables are not 
supported as logical replication targets")));
+
/*
 * We currently only support writing to regular tables.
 */

GIN predicate locking slows down valgrind isolationtests tremendously

[Andres Freund]

Hi,

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-03-31%2014%3A40%3A02
isolation-check (02:19:36)

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-03-28%2010%3A40%3A02
isolation-check (00:18:44)

As far as I can tell that increase comes laregely from the new GIN
tests.  Could one of you please look at keeping the test time increase
to something more reasonable?

Greetings,

Andres Freund

don't mark indexes invalid unnecessarily

[Alvaro Herrera]

While working on FKs pointing to partitioned tables, I noticed that in
PG11 we fail to produce a working dump in the case of a partitioned
table that doesn't have partitions.

The attached patch fixes that.  In doing so, it breaks a test ... and
analyzing that, it turns out that the test was broken, it wasn't testing
what it was supposed to be testing.  I patched it so that it continues
to work (and now it tests the correct thing).  To be precise, the test
was doing this:

 create table parted_conflict (a int, b text) partition by range (a);
 create table parted_conflict_1 partition of parted_conflict for values from 
(0) to (1000) partition by range (a);
 create unique index on only parted_conflict_1 (a);
 create unique index on only parted_conflict (a);
 alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
 create table parted_conflict_1_1 partition of parted_conflict_1 for values 
from (0) to (500);

with the expectation that the partition would not have a proper index on
which to run an INSERT INTO parted_conflict_1 ON CONFLICT (a), expecting
that partition parted_conflict_1_1 would not have the index.  But in
reality parted_conflict_1_1 does have the index (and it only fails
because the index in its parent is marked invalid).  So the patch moves
the CREATE TABLE parted_conflict_1_1 to before the indexes creation, so
that the partition really does not have the index, and then it gets the
expected error.

If you were to run the pg_upgrade test with my fks-to-partitioned-tables
patch, it will fail because of a PK on an partitionless partitioned
table being marked invalid after restore; but if you run it after
applying this patch, it should work (it works for me).

-- 
Álvaro Herrera39°50'S 73°21'W
commit 444e92a363bafc65dde710d3c21a5f0b25b308e1
Author: Alvaro Herrera 
AuthorDate: Mon Dec 3 19:30:37 2018 -0300
CommitDate: Mon Dec 3 19:31:04 2018 -0300

Don't mark partitioned indexes invalid unnecessarily

If the partitioned table doesn't have partitions, then an index created
with CREATE INDEX/ON ONLY doesn't have to be marked as invalid ... and
doing so breaks pg_dump for those tables, so don't.

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3975f62c00..965b9f0d23 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -834,8 +834,18 @@ DefineIndex(Oid relationId,
 		flags |= INDEX_CREATE_PARTITIONED;
 	if (stmt->primary)
 		flags |= INDEX_CREATE_IS_PRIMARY;
+
+	/*
+	 * If the table is partitioned, and recursion was declined but partitions
+	 * exist, mark the index as invalid.
+	 */
 	if (partitioned && stmt->relation && !stmt->relation->inh)
-		flags |= INDEX_CREATE_INVALID;
+	{
+		PartitionDesc	pd = RelationGetPartitionDesc(rel);
+
+		if (pd->nparts != 0)
+			flags |= INDEX_CREATE_INVALID;
+	}
 
 	if (stmt->deferrable)
 		constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out
index 27cf5a01b3..a28611745c 100644
--- a/src/test/regress/expected/insert_conflict.out
+++ b/src/test/regress/expected/insert_conflict.out
@@ -876,10 +876,10 @@ drop table parted_conflict;
 -- partition
 create table parted_conflict (a int, b text) partition by range (a);
 create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
+create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
 create unique index on only parted_conflict_1 (a);
 create unique index on only parted_conflict (a);
 alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
-create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
 insert into parted_conflict values (40, 'forty');
 insert into parted_conflict_1 values (40, 'cuarenta')
   on conflict (a) do update set b = excluded.b;
diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql
index c677d70fb7..c68013e179 100644
--- a/src/test/regress/sql/insert_conflict.sql
+++ b/src/test/regress/sql/insert_conflict.sql
@@ -551,10 +551,10 @@ drop table parted_conflict;
 -- partition
 create table parted_conflict (a int, b text) partition by range (a);
 create table parted_conflict_1 partition of parted_conflict for values from (0) to (1000) partition by range (a);
+create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
 create unique index on only parted_conflict_1 (a);
 create unique index on only parted_conflict (a);
 alter index parted_conflict_a_idx attach partition parted_conflict_1_a_idx;
-create table parted_conflict_1_1 partition of parted_conflict_1 for values from (0) to (500);
 insert into parted_conflict values (40, 'forty');
 insert into parted_conflict_1 values (40, 'cuarenta')
   on conflict (a) do update set b = excluded.b;

Re: [PATCH v19] GSSAPI encryption support

[Thomas Munro]

On Tue, Dec 4, 2018 at 10:20 AM Stephen Frost  wrote:
> (and I have to wonder- if we want nearly all callers of
> WaitLatch/WaitLatchOrSocket to use WL_EXIT_ON_PM_DEATH, maybe we should
> make that the default and allow it to be overridden..? ...

That is what I proposed.  It was Heikki who talked me into the opt-in
solution, but using an assertion to make sure you handle it one way or
the other:

https://www.postgresql.org/message-id/6417314e-93d5-ed2d-9012-8d6e9ed21778%40iki.fi

Perhaps I should have sought more opinions.  Please feel free to start
a new thread on that if you don't like the way it was done.

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

Re: [PATCH v19] GSSAPI encryption support

[Stephen Frost]

Greetings Robbie,

* Dmitry Dolgov (9erthali...@gmail.com) wrote:
> > On Tue, Oct 2, 2018 at 11:12 PM Robbie Harwood  wrote:
> >
> > Michael Paquier  writes:
> >
> > > On Mon, Aug 06, 2018 at 05:23:28PM -0400, Robbie Harwood wrote:
> > >> If you're in a position where you're using Kerberos (or most other
> > >> things from the GSSAPI) for authentication, the encryption comes at
> > >> little to no additional setup cost.  And then you get all the security
> > >> benefits outlined in the docs changes.
> > >
> > > Please note that the last patch set does not apply anymore, so I have
> > > moved it to CF 2018-11 and marked it as waiting on author.
> >
> > Indeed.  Please find v19 attached.  It's just a rebase; no architecture
> > changes.
> 
> Unfortunately, patch needs another rebase, could you do this? In the meantime
> I'll move it to the next CF.

This patch needs a few minor changes to get it back to working, but I'm
happy to say that with those changes, it seems to be working rather well
for me.

I'm working on a more comprehensive review but I wanted to go ahead and
get these first minor items out of the way:

Needs the same treatment done in ab69ea9, for all the WaitLatchOrSocket
calls in be-secure-gssapi.c:

-   WaitLatchOrSocket(MyLatch, WL_SOCKET_WRITEABLE, port->sock, 0,
+   WaitLatchOrSocket(MyLatch, WL_SOCKET_WRITEABLE | 
WL_EXIT_ON_PM_DEATH, port->sock, 0,
  WAIT_EVENT_GSS_OPEN_SERVER);

(and I have to wonder- if we want nearly all callers of
WaitLatch/WaitLatchOrSocket to use WL_EXIT_ON_PM_DEATH, maybe we should
make that the default and allow it to be overridden..?  Also, does
ModifyWaitEvent() need to also do some checking here..?  Should
be_tls_read()'s waitfor settings also include WL_EXIT_ON_PM_DEATH?)

Needed a minor adjustment in src/interfaces/libpq/fe-connect.c due to
conflict with 6e5f8d4.

Otherwise, it's building and running with all flavors of client and
server-side GSS-related options (require/disable/prefer and
host/hostgss/hostnogss).

Not surprisingly, trying to connect from a newer psql w/
PGGSSMODE=require (explicitly requesting encryption from the server)
with an older server blows up, but there's not much we can do for that.
Using prefer works, with an extra roundtrip to discover the server
doesn't understand GSSAPI encryption and then falling back.

Also, when connecting from an older psql to a newer server, if
pg_hba.conf has 'host' or 'hostnogss', everything works great (though
without GSSAPI encryption, of course), while an entry of 'hostgss'
returns the typical 'no pg_hba.conf entry found', as you'd expect.

Just in general, it seems like the code could use a lot more
comments. :)  Also, it needs some pgindent run over it.

That's about all I have time to cover for today, but maybe you could try
to spruce up the comments (I'm at least a fan of function-level
comments, in particular, explaining how they're to be used, et al..),
see if you can get pgindent run over the changes and make the
above-mentioned fixes and then perhaps we can get cfbot to do its thing,
ask the other folks on the thread who were having issues before to retry
with this patch, if possible, and I'll start doing a more thorough code
review later this week.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [PATCH] Log CSV by default

[Gilles Darold]

Le 03/12/2018 à 19:25, David Fetter a écrit :
> On Mon, Dec 03, 2018 at 07:18:31PM +0100, Gilles Darold wrote:
>> Le 30/11/2018 à 19:53, David Fetter a écrit :
>>> This makes it much simpler for computers to use the logs while not
>>> making it excessively difficult for humans to use them.
>> I'm also not very comfortable with this change. For a pgBadger point
>> of view I don't know an existing library to parse csv log in
>> multi-process mode. The only way I know is to split the file or load
>> chunks in memory which is not really recommended with huge log
>> files. I am not saying that this is not possible to have a Perl CSV
>> library allowing multi-process but this require some additional days
>> of work.
> How about other structured log formats? Would one for JSON work
> better?


pgBadger is able to parse jsonlog
(https://github.com/michaelpq/pg_plugins/tree/master/jsonlog) output in
multi-process mode because it do not use an external library to parse
the json. In this minimalist implementation I assume that each log line
is a full json object. Honestly I do not have enough feedback on this
format to be sure that my basic jsonlog parser is enough, maybe it is
not. What is really annoying for a log parser is the preservation of
newline character in queries, if the format outputs each atomic log
messages on a single line each, this is ideal.


>
>> Also at this time I have not received any feature request for that,
>> maybe the use of csvlog format is not so common.
> Part of the reason it's uncommon is that it's not the default we ship
> with, and defaults influence this very much.

I agree. I also think that syslog or stderr are more human readable than
csvlog or jsonlog, which have probably contributed to their large
adoption too.



-- 
Gilles Darold
http://www.darold.net/

citelny zajimavy clanek venovany fyzicke replikaci

[Pavel Stehule]

Ahoj

viz
https://www.percona.com/blog/2018/11/30/postgresql-streaming-physical-replication-with-slots/

Pavel

Re: Bug fix for glibc broke freebsd build in REL_11_STABLE

[Tom Lane]

Andres Freund  writes:
> On 2018-12-01 17:28:54 -0500, Tom Lane wrote:
>> I guess I wasn't precise enough: I meant add -msse2 if on x86 and
>> compiler doesn't take -fexcess-precision=standard.

> Hm, I still don't like that: It'd silently bump the minimum required
> architecture. Like in the case of x86-32 freebsd, which doesn't require
> sse2 and uses clang, the maintainer wouldn't notice that they would have
> to switch to gcc to continue supporting their baseline.

Yeah, that's a fair point.

> It's not like there's that many people compiling for such platforms with
> insufficient compiler support, so forcing them to specify -msse2 if
> that's the desired escape hatch doesn't sound terrible to me.

I spent some time over the weekend investigating this, including going
so far as to build gcc 3.4.6 from source to try to reproduce the report
I had off-list.  No luck; passed all regression tests just fine.  In
fact, I was only able to produce any test failures at all with clang.
My conclusion is that it's really hard to get gcc to exhibit this type of
problem, even when using old versions that don't have -fexcess-precision.

I am now thinking that the right thing to do is nothing.  If we insist
on having -msse2 or -fexcess-precision, the net effect is going to be
to break the build on more old platforms than we fix anything on.  Yes,
updating to a newer compiler or moving your hardware baseline are both
recommendable things to do, but why should we force that on people who
aren't seeing a problem?  Especially in stable back branches?
(In HEAD this should be moot anyway, given that C99 implies
-fexcess-precision=standard.)

regards, tom lane

Re: [PATCH] Log CSV by default

[David Fetter]

On Mon, Dec 03, 2018 at 07:18:31PM +0100, Gilles Darold wrote:
> Le 30/11/2018 à 19:53, David Fetter a écrit :
> > This makes it much simpler for computers to use the logs while not
> > making it excessively difficult for humans to use them.
> 
> I'm also not very comfortable with this change. For a pgBadger point
> of view I don't know an existing library to parse csv log in
> multi-process mode. The only way I know is to split the file or load
> chunks in memory which is not really recommended with huge log
> files. I am not saying that this is not possible to have a Perl CSV
> library allowing multi-process but this require some additional days
> of work.

How about other structured log formats? Would one for JSON work
better?

> Also at this time I have not received any feature request for that,
> maybe the use of csvlog format is not so common.

Part of the reason it's uncommon is that it's not the default we ship
with, and defaults influence this very much.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: [PATCH] Log CSV by default

[Gilles Darold]

Le 30/11/2018 à 19:53, David Fetter a écrit :
> This makes it much simpler for computers to use the logs while not
> making it excessively difficult for humans to use them.


I'm also not very comfortable with this change. For a pgBadger point of
view I don't know an existing library to parse csv log in multi-process
mode. The only way I know is to split the file or load chunks in memory
which is not really recommended with huge log files. I am not saying
that this is not possible to have a Perl CSV library allowing
multi-process but this require some additional days of work. Also at
this time I have not received any feature request for that, maybe the
use of csvlog format is not so common.


Regards,

-- 
Gilles Darold
http://www.darold.net/

Re: [proposal] Add an option for returning SQLSTATE in psql error message

[Pavel Stehule]

po 3. 12. 2018 v 18:57 odesílatel didier  napsal:

> On Mon, Dec 3, 2018 at 5:51 PM Tom Lane  wrote:
>
> > No, it's in libpq, so you'd have to touch that not the server.
> libpq, not as bad as the server but nonetheless maybe a bit too much for
> this.
> Is adding value in library contract?
>
> Anyway. attached a POC adding a new value to VERBOSITY (hopefully
> nobody is using it).
>

It can works :). Please, assign it to next commitfest.

Regards

Pavel

Re: [proposal] Add an option for returning SQLSTATE in psql error message

[didier]

On Mon, Dec 3, 2018 at 5:51 PM Tom Lane  wrote:

> No, it's in libpq, so you'd have to touch that not the server.
libpq, not as bad as the server but nonetheless maybe a bit too much for this.
Is adding value in library contract?

Anyway. attached a POC adding a new value to VERBOSITY (hopefully
nobody is using it).
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 2e9fe760eb..e0c3a4fb9f 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -411,7 +411,7 @@ helpVariables(unsigned short int pager)
 	fprintf(output, _("  USER\n"
 	  "the currently connected database user\n"));
 	fprintf(output, _("  VERBOSITY\n"
-	  "controls verbosity of error reports [default, verbose, terse]\n"));
+	  "controls verbosity of error reports [default, verbose, terse, sqlstate]\n"));
 	fprintf(output, _("  VERSION\n"
 	  "  VERSION_NAME\n"
 	  "  VERSION_NUM\n"
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index e7536a8a06..293ffcc5ef 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -1088,9 +1088,11 @@ verbosity_hook(const char *newval)
 		pset.verbosity = PQERRORS_TERSE;
 	else if (pg_strcasecmp(newval, "verbose") == 0)
 		pset.verbosity = PQERRORS_VERBOSE;
+	else if (pg_strcasecmp(newval, "sqlstate") == 0)
+		pset.verbosity = PQERRORS_SQLSTATE;
 	else
 	{
-		PsqlVarEnumError("VERBOSITY", newval, "default, terse, verbose");
+		PsqlVarEnumError("VERBOSITY", newval, "default, terse, sqlstate, verbose");
 		return false;
 	}
 
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index fa44b2820b..3f0ad5192a 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3571,7 +3571,7 @@ psql_completion(const char *text, int start, int end)
 		else if (TailMatchesCS("SHOW_CONTEXT"))
 			COMPLETE_WITH_CS("never", "errors", "always");
 		else if (TailMatchesCS("VERBOSITY"))
-			COMPLETE_WITH_CS("default", "verbose", "terse");
+			COMPLETE_WITH_CS("default", "verbose", "terse", "sqlstate");
 	}
 	else if (TailMatchesCS("\\sf*"))
 		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_routines, NULL);
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 8345faface..c58eabcd10 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1017,6 +1017,15 @@ pqBuildErrorMessage3(PQExpBuffer msg, const PGresult *res,
 	val = PQresultErrorField(res, PG_DIAG_SEVERITY);
 	if (val)
 		appendPQExpBuffer(msg, "%s:  ", val);
+	if (verbosity == PQERRORS_SQLSTATE)
+	{
+		val = PQresultErrorField(res, PG_DIAG_SQLSTATE);
+		if (val)
+			appendPQExpBuffer(msg, "%s", val);
+		appendPQExpBufferChar(msg, '\n');
+		return;
+	}
+
 	if (verbosity == PQERRORS_VERBOSE)
 	{
 		val = PQresultErrorField(res, PG_DIAG_SQLSTATE);
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 3f13ddf092..f280a19b5a 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -111,7 +111,8 @@ typedef enum
 {
 	PQERRORS_TERSE,/* single-line error messages */
 	PQERRORS_DEFAULT,			/* recommended style */
-	PQERRORS_VERBOSE			/* all the facts, ma'am */
+	PQERRORS_VERBOSE,			/* all the facts, ma'am */
+	PQERRORS_SQLSTATE			/* single-line SQLSTATE error */
 } PGVerbosity;
 
 typedef enum

Re: [HACKERS] CLUSTER command progress monitor

[Alvaro Herrera]

On 2018-Dec-03, Tatsuro Yamada wrote:

> > In the meantime I'm moving it to the next CF.
> 
> Thank you for managing the CF and Sorry for the late reply.
> I'll rebase it for the next CF and also I'll clear my head because the patch
> needs design change to address the feedbacks, I guess. Therefore, the status 
> is
> reconsidering the design of the patch. :)

I think we should mark it as Returned with Feedback then.

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

Re: [proposal] Add an option for returning SQLSTATE in psql error message

[Tom Lane]

didier  writes:
> Yep, name is bad, but I'm not sure about VERBOSITY, isn't it
> controlling output from the server not the client?

No, it's in libpq, so you'd have to touch that not the server.

I agree with Andrew's thought, and would further say that just
"\set VERBOSITY sqlstate" would be a reasonable API.  Making this
separate from VERBOSITY is weird.

regards, tom lane

Re: doc - improve description of default privileges

[Tom Lane]

Fabien COELHO  writes:
>>> I feel if we're going to do anything, we should put a unified description
>>> of privileges and aclitem-reading into section 5.6, and take that material
>>> out of the various places where it lives now.  Like the attached, in which
>>> I failed to resist the temptation to wordsmith some stuff as well as move
>>> it around.

>> I looked at the psql manpage and the HTML rendering of section 5.6 and
>> it all looks good to me.

> Indeed, this looks great, a precise and full description of privileges 
> just in one place.

Pushed (with a little bit more tweaking).

regards, tom lane

Re: SQL/JSON: documentation

[Liudmila Mantrova]

On 11/29/18 7:34 PM, Dmitry Dolgov wrote:

Hi,

Any progress on that? It would be nice to have a new version of the
documentation, and I would even advocate to put it into the json path patch [1]
(especially, since there were already requests for that, and I personally don't
see any reason to keep them separately). For now I'll move the item to
the next CF.

[1]:https://www.postgresql.org/message-id/flat/fcc6fc6a-b497-f39a-923d-aa34d0c588e8%402ndQuadrant.com


Hi Dmitry,

Unfortunately, I couldn't find much time for this activity, but as far 
as I understand, thread [1] only requires jsonpath documentation right 
now. So I extracted the relevant parts from this patch, reworked path 
expression description, and moved it to func.sgml as Peter suggested 
(attached). Nikita is going to add this patch to the jsonpath thread 
together with the updated code once it's ready.


Next, I'm going to address Peter's feedback on the rest of this 
documentation patch (which probably also needs to be split for threads 
[2] and [3]).


[2] 
https://www.postgresql.org/message-id/flat/cd0bb935-0158-78a7-08b5-904886dea...@postgrespro.ru
[3] 
https://www.postgresql.org/message-id/flat/132f26c4-dfc6-f8fd-4764-2cbf455a3...@postgrespro.ru


--
Liudmila Mantrova
Technical writer at Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/doc/src/sgml/biblio.sgml b/doc/src/sgml/biblio.sgml
index 4953024..f06305d 100644
--- a/doc/src/sgml/biblio.sgml
+++ b/doc/src/sgml/biblio.sgml
@@ -136,6 +136,17 @@
 1988

 
+   
+SQL Technical Report
+Part 6: SQL support for JavaScript Object
+  Notation (JSON)
+First Edition.
+
+http://standards.iso.org/ittf/PubliclyAvailableStandards/c067367_ISO_IEC_TR_19075-6_2017.zip";>.
+
+2017.
+   
+
   
 
   
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 112d962..20ef7df 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -11285,26 +11285,661 @@ table2-mapping
  
 
  
-  JSON Functions and Operators
+  JSON Functions, Operators, and Expressions
 
-  
+  
+   The functions, operators, and expressions described in this section
+   operate on JSON data:
+  
+
+  
+   
+
+ SQL/JSON path expressions
+ (see ).
+
+   
+   
+
+ PostgreSQL-specific functions and operators for JSON
+ data types (see ).
+
+   
+  
+
+  
+To learn more about the SQL/JSON standard, see
+. For details on JSON types
+supported in PostgreSQL,
+see .
+  
+
+ 
+  SQL/JSON Path Expressions
+
+  
+   SQL/JSON path expressions specify the items to be retrieved
+   from the JSON data, similar to XPath expressions used
+   for SQL access to XML. In PostgreSQL,
+   path expressions are implemented as the jsonpath
+   data type, described in .
+  
+
+  JSON query functions and operators
+   pass the provided path expression to the path engine
+   for evaluation. If the expression matches the JSON data to be queried,
+   the corresponding SQL/JSON item is returned.
+   Path expressions are written in the SQL/JSON path language
+   and can also include arithmetic expressions and functions.
+   Query functions treat the provided expression as a
+   text string, so it must be enclosed in single quotes.
+  
+
+  
+   A path expression consists of a sequence of elements allowed
+   by the jsonpath data type.
+   The path expression is evaluated from left to right, but
+   you can use parentheses to change the order of operations.
+   If the evaluation is successful, an SQL/JSON sequence is produced,
+   and the evaluation result is returned to the JSON query function
+   that completes the specified computation.
+  
+
+  
+   To refer to the JSON data to be queried (the
+   context item), use the $ sign
+   in the path expression. It can be followed by one or more
+   accessor operators,
+   which go down the JSON structure level by level to retrieve the
+   content of context item. Each operator that follows deals with the
+   result of the previous evaluation step.
+  
+
+  
+   For example, suppose you have some JSON data from a GPS tracker that you
+   would like to parse, such as:
+
+{ "track" :
+  {
+"segments" : [ 
+  { "location":   [ 47.763, 13.4034 ],
+"start time": "2018-10-14 10:05:14",
+"HR": 73
+  },
+  { "location":   [ 47.706, 13.2635 ],
+"start time": "2018-10-14 10:39:21",
+"HR": 130
+  } ]
+  }
+}
+
+  
+
+  
+   To retrieve the available track segments, you need to use the
+   .key accessor
+   operator for all the preceding JSON objects:
+
+'$.track.segments'
+
+  
+
+  
+   If the item to retrieve is an element of an array, you have
+   to unnest this array using the [*] operator. For example,
+   the following path will return location coordinates for all
+   the available track segments:
+
+'$.track.segments[*].location'
+
+  
+
+  
+   To return the coordinates of the first segment only, you can
+   specify the corres

Re: [proposal] Add an option for returning SQLSTATE in psql error message

[didier]

Yep, name is bad, but I'm not sure about VERBOSITY, isn't it
controlling output from the server not the client?
You may want to set both VERBOSITY to 'verbose' and ECHO_ERROR to
'none' then in script do
SELECT  -- no error output
\if :ERROR
   -- do something with LAST_ERROR_MESSAGE



On Mon, Dec 3, 2018 at 4:49 PM Andrew Gierth
 wrote:
>
> > "didier" == didier   writes:
>
>  didier> Attached a POC adding a new variable ECHO_ERROR
>  didier> \set ECHO_ERROR text|none|psqlstate
>
> I wouldn't have called it that. Possibly another option to the existing
> VERBOSITY variable? \set VERBOSITY sqlstate_only or something of that
> ilk (it's already not unusual to use \set VERBOSITY terse in regression
> tests)
>
> --
> Andrew (irc:RhodiumToad)

Re: [proposal] Add an option for returning SQLSTATE in psql error message

[Pavel Stehule]

po 3. 12. 2018 v 16:49 odesílatel Andrew Gierth 
napsal:

> > "didier" == didier   writes:
>
>  didier> Attached a POC adding a new variable ECHO_ERROR
>  didier> \set ECHO_ERROR text|none|psqlstate
>
> I wouldn't have called it that. Possibly another option to the existing
> VERBOSITY variable? \set VERBOSITY sqlstate_only or something of that
> ilk (it's already not unusual to use \set VERBOSITY terse in regression
> tests)
>

It is good idea to look to this option.

Pavel


> --
> Andrew (irc:RhodiumToad)
>
>

Re: Commitfest 2018-11

[David Steele]

On 12/1/18 1:46 PM, Lætitia Avrot wrote:



Yeah, I've been annoyed by that too (not that I've not been guilty of
it myself).  We should probably tell people not to add themselves as
reviewers unless they're actively planning to review soon, because it
discourages other people from reviewing the same item.  Maybe also,
when a patch gets moved to the next CF, we should drop all reviewers
(ie make them sign up afresh if they're still interested).


+1 on that one. It's not because you had time on the previous commit 
fest that you'll have time for the next.


+1.  I've been in this situation myself.

--
-David
da...@pgmasters.net

Re: [proposal] Add an option for returning SQLSTATE in psql error message

[Andrew Gierth]

> "didier" == didier   writes:

 didier> Attached a POC adding a new variable ECHO_ERROR
 didier> \set ECHO_ERROR text|none|psqlstate

I wouldn't have called it that. Possibly another option to the existing
VERBOSITY variable? \set VERBOSITY sqlstate_only or something of that
ilk (it's already not unusual to use \set VERBOSITY terse in regression
tests)

-- 
Andrew (irc:RhodiumToad)

Re: [proposal] Add an option for returning SQLSTATE in psql error message

[didier]

Attached a POC adding a new variable ECHO_ERROR
\set ECHO_ERROR text|none|psqlstate

On Mon, Dec 3, 2018 at 2:47 AM Andrew Gierth
 wrote:
>
> > "Tom" == Tom Lane  writes:
>
>  Tom> I don't buy that argument. We use psql's normal display in all the
>  Tom> regular regression tests, and it's not a big maintenance problem.
>
> The regular regression tests have the advantage that they don't need to
> work across pg versions.
>
> It is more of a problem for extensions; I just ran into this myself in
> fact, with a test failing because "invalid input syntax for integer" got
> changed to "invalid input syntax for type integer".
>
> --
> Andrew (irc:RhodiumToad)
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 62c2928e6b..049d43086f 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -536,10 +536,22 @@ AcceptResult(const PGresult *result)
 
 	if (!OK)
 	{
-		const char *error = PQerrorMessage(pset.db);
+		const char *error = "";
+		const char *err_fmt = "%s";
+		switch (pset.echo_error) {
+		case PSQL_ECHO_ERROR_NONE:
+			break;
+		case PSQL_ECHO_ERROR_PSQLSTATE:
+			err_fmt = "%s\n";
+			error = PQresultErrorField(result, PG_DIAG_SQLSTATE);
+			break;
+		default:
+			error = PQerrorMessage(pset.db);
+			break;
+		}
 
 		if (strlen(error))
-			psql_error("%s", error);
+			psql_error(err_fmt, error);
 
 		CheckConnection();
 	}
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 2e9fe760eb..b653e37526 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -339,7 +339,7 @@ helpVariables(unsigned short int pager)
 	 * Windows builds currently print one more line than non-Windows builds.
 	 * Using the larger number is fine.
 	 */
-	output = PageOutput(156, pager ? &(pset.popt.topt) : NULL);
+	output = PageOutput(159, pager ? &(pset.popt.topt) : NULL);
 
 	fprintf(output, _("List of specially treated variables\n\n"));
 
@@ -360,6 +360,9 @@ helpVariables(unsigned short int pager)
 	fprintf(output, _("  ECHO_HIDDEN\n"
 	  "if set, display internal queries executed by backslash commands;\n"
 	  "if set to \"noexec\", just show them without execution\n"));
+	fprintf(output, _("  ECHO_ERROR\n"
+	  "controls what error msg is written to standard error\n"
+	  "[text, sqlstate, none]\n"));
 	fprintf(output, _("  ENCODING\n"
 	  "current client character set encoding\n"));
 	fprintf(output, _("  ERROR\n"
@@ -411,7 +414,7 @@ helpVariables(unsigned short int pager)
 	fprintf(output, _("  USER\n"
 	  "the currently connected database user\n"));
 	fprintf(output, _("  VERBOSITY\n"
-	  "controls verbosity of error reports [default, verbose, terse]\n"));
+	  "controls verbosity of server error reports [default, verbose, terse]\n"));
 	fprintf(output, _("  VERSION\n"
 	  "  VERSION_NAME\n"
 	  "  VERSION_NUM\n"
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 176c85afd0..dd1f5f2c55 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -48,6 +48,13 @@ typedef enum
 	PSQL_ECHO_HIDDEN_NOEXEC
 } PSQL_ECHO_HIDDEN;
 
+typedef enum
+{
+	PSQL_ECHO_ERROR_NONE,
+	PSQL_ECHO_ERROR_TEXT,
+	PSQL_ECHO_ERROR_PSQLSTATE
+} PSQL_ECHO_ERROR;
+
 typedef enum
 {
 	PSQL_ERROR_ROLLBACK_OFF,
@@ -132,6 +139,7 @@ typedef struct _psqlSettings
 	int			ignoreeof;
 	PSQL_ECHO	echo;
 	PSQL_ECHO_HIDDEN echo_hidden;
+	PSQL_ECHO_ERROR echo_error;
 	PSQL_ERROR_ROLLBACK on_error_rollback;
 	PSQL_COMP_CASE comp_case;
 	HistControl histcontrol;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index e7536a8a06..156b4665ac 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -949,6 +949,33 @@ echo_hook(const char *newval)
 	return true;
 }
 
+static char *
+echo_error_substitute_hook(char *newval)
+{
+	if (newval == NULL)
+		newval = pg_strdup("text");
+	return newval;
+}
+
+static bool
+echo_error_hook(const char *newval)
+{
+	Assert(newval != NULL);		/* else substitute hook messed up */
+	if (pg_strcasecmp(newval, "text") == 0)
+		pset.echo_error = PSQL_ECHO_ERROR_TEXT;
+	else if (pg_strcasecmp(newval, "none") == 0)
+		pset.echo_error = PSQL_ECHO_ERROR_NONE;
+	else if (pg_strcasecmp(newval, "psqlstate") == 0)
+		pset.echo_error = PSQL_ECHO_ERROR_PSQLSTATE;
+	else
+	{
+		PsqlVarEnumError("ECHO_ERROR", newval, "none, psqlstate, text");
+		return false;
+	}
+	return true;
+}
+
+
 static bool
 echo_hidden_hook(const char *newval)
 {
@@ -1167,6 +1194,9 @@ EstablishVariableSpace(void)
 	SetVariableHooks(pset.vars, "ECHO_HIDDEN",
 	 bool_substitute_hook,
 	 echo_hidden_hook);
+	SetVariableHooks(pset.vars, "ECHO_ERROR",
+	 echo_error_substitute_hook,
+	 echo_error_hook);
 	SetVariableHooks(pset.vars, "ON_ERROR_ROLLBACK",
 	 bool_substitute_hook,
 	 on_error_rollback_hook);
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index fa44b2820b..bf23c1f4bb 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3561,6 +3561,8 @@ psql_com

JSON Merge-Patch

[Charles Leifer]

I wanted to request that you all consider implementing a "merge" or
"update" function for performing in-place updates of JSON data-structures.

The relevant algorithm is described here:
https://tools.ietf.org/html/rfc7396

SQLite's json1 extension has an implementation and it's quite useful:
https://www.sqlite.org/cgi/src/artifact/3f017d2659e531d0

For example:

{k1: {x1: y1}} merge {k1: {x2: y2}} -- yields --> {k1 : {x1: y1, x2: y2}}

Thank you for all your work on Postgres. I hope you'll consider adding this
as a built-in function.

Charles

Re: Undo worker and transaction rollback

[Dilip Kumar]

On Fri, 30 Nov 2018, 20:42 Dmitry Dolgov <9erthali...@gmail.com wrote:

> > On Mon, Nov 5, 2018 at 1:32 PM Dilip Kumar 
> wrote:
> >
> > Updated patch, include defect fix from Kuntal posted on [1].
> >
> > [1]
> https://www.postgresql.org/message-id/CAGz5QCKpCG6egFAdazC%2BJgyk7YSE1OBN9h-QpwCkg-NnSWN5AQ%40mail.gmail.com
>
> Thanks for working on this feature. Unfortunately, looks like the current
> version of patch has some conflicts with the current master, could you
> please
> post an updated version again?
>

Currently, I am in process rebasing undolog patch set[1]. Post that I will
rebase this patch on top of those patch set.

[1]
https://www.postgresql.org/message-id/CAFiTN-syRxU3jTpkOxHQsEqKC95LGd86JTdZ2stozyXWDUSffg%40mail.gmail.com

Delay locking partitions during query execution

[David Rowley]

Over on [1] I'm proposing to delay locking partitions of a partitioned
table that's the target of an INSERT or UPDATE command until we first
route a tuple to the partition.   Currently, we go and lock all
partitions, even if we just insert a single tuple to a single
partition. The patch in [1] improves the situation when there are many
partitions and only a few tuples to route to just to a few partitions.

Over here and along similar lines to the above, but this time I'd like
to take this even further and change things so we don't lock *any*
partitions during AcquireExecutorLocks() and instead just lock them
when we first access them with ExecGetRangeTableRelation().  This
improves the situation when many partitions get run-time pruned, as
we'll never bother locking those at all since we'll never call
ExecGetRangeTableRelation() on them. We'll of course still lock the
partitioned table so that plan invalidation works correctly.

This does make the locking order less well defined, but I'm already
proposing similar in [1] and over there I've mentioned that I can't
quite see any huge issues with doing that.  We already don't lock all
partitions inside AcquireExecutorLocks() during INSERT with VALUES
anyway.

The attached patch implements this delayed locking.  A quick benchmark
shows a pretty good performance improvement when there are a large
number of partitions and run-time pruning prunes almost all of them.

Setup:
create table hashp (a int) partition by hash(a);
select 'create table hashp'|| x::text || ' partition of hashp for
values with (modulus 1, remainder ' || x ::text || ');' from
generate_Series(0,) x;
\gexec
insert into hashp select generate_Series(1,100)

bench.sql:
\set p_a 13315
select * from hashp where a = :p_a;

Master: 1 parts

$ pgbench -n -f bench.sql -M prepared -T 60 postgres
tps = 108.882749 (excluding connections establishing)
tps = 108.245437 (excluding connections establishing)

delaylock: 1 parts

$ pgbench -n -f bench.sql -M prepared -T 60 postgres
tps = 1068.289505 (excluding connections establishing)
tps = 1092.797015 (excluding connections establishing)

More could be done to make this quite a bit faster again, but that
mostly requires the range table coming directly from the planner as an
array and marking which array elements require locking with a
Bitmapset. This'll save having to loop over the entire large array
that mostly does not need anything locked. Similar can be done for
ExecCheckRTPerms(), but that's also for another day. With those
changes and some further tweaks done on the Append/MergeAppend code,
tps is about 22k on my machine, which is just slightly slower than
with an equivalent non-partitioned table.

I'll add the attached patch to the January commitfest

[1] 
https://www.postgresql.org/message-id/CAKJS1f-%3DFnMqmQP6qitkD%2BxEddxw22ySLP-0xFk3JAqUX2yfMw%40mail.gmail.com

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


v1-0001-Allow-lock-acquisitions-for-partitions-to-be-dela.patch
Description: Binary data

Re: Reviving the "Stopping logical replication protocol" patch from Vladimir Gordichuk

[Dave Cramer]

Dmitry,

Please see attached rebased patches

Dave Cramer


On Fri, 30 Nov 2018 at 18:52, Dmitry Dolgov <9erthali...@gmail.com> wrote:

> >On Sat, Dec 1, 2018 at 12:49 AM Dave Cramer  wrote:
> >
> > Thanks, I have done a preliminary check and it seems pretty
> straightforward.
> >
> > I will clean it up for Monday
>
> Great, thank you!
>


0004-Add-test-for-pg_recvlogical-to-stop-replication.patch
Description: Binary data


0003-Add-ability-for-pg_recvlogical-to-stop-replication-f.patch
Description: Binary data


0001-Respect-client-initiated-CopyDone-in-walsender.patch
Description: Binary data


0002-Client-initiated-CopyDone-during-transaction-decodin.patch
Description: Binary data

Re: postgres_fdw: oddity in costing aggregate pushdown paths

[Etsuro Fujita]

(2018/11/30 18:51), Etsuro Fujita wrote:
> (2018/11/28 13:38), Etsuro Fujita wrote:
>> BTW another thing I noticed is this comment on costing aggregate
>> pushdown paths using local statistics in estimate_path_cost_size:
>>
>>* Also, core does not care about costing HAVING expressions 
>> and
>>* adding that to the costs.  So similarly, here too we are not
>>* considering remote and local conditions for costing.
>>
>> I think this was true when aggregate pushdown went in, but isn't anymore
>> because of commit 7b6c07547190f056b0464098bb5a2247129d7aa2.  So we
>> should update estimate_path_cost_size so that it accounts for the
>> selectivity and cost of the HAVING expressions as well?
> 
> There seems to be no objections, I updated the patch as such.  Attached
> is an updated version of the patch.

I revised some comments a bit and added the commit message.  Attached is
an updated patch.  If there are no objections, I'll apply this to HEAD only.

Best regards,
Etsuro Fujita
>From 1596c46b0531b9ca2433352e47e70cf1f22d4f44 Mon Sep 17 00:00:00 2001
From: Etsuro Fujita 
Date: Mon, 3 Dec 2018 19:52:47 +0900
Subject: [PATCH] postgres_fdw: Improve cost and size estimation for aggregte
 pushdown

In commit 7012b132d07c2b4ea15b0b3cb1ea9f3278801d98, which added aggregate
pushdown to postgres_fdw, we didn't account for the evaluation cost and the
selectivity of HAVING quals attached to ForeignPaths performing aggregate
pushdown, as core had never accounted for that for AggPaths and GroupPaths.
And we didn't set these values of the locally-checked quals (ie, fpinfo's
local_conds_cost and local_conds_sel), which were initialized to zeros, but
since estimate_path_cost_size factors in these to estimate the result size
and the evaluation cost of such a ForeignPath when the use_remote_estimate
option is enabled, this caused it to produce underestimated results in that
mode.

By commit 7b6c07547190f056b0464098bb5a2247129d7aa2 core was changed so that
it accounts for the evaluation cost and the selectivity of HAVING quals in
aggregation paths, so change the postgres_fdw's aggregate pushdown code as
well as such.  This not only fixes the underestimation issue mentioned
above, but improves the estimation using local statistics in that function
when that option is diabled.

This would be a bug fix rather than an improvement, but apply it to HEAD
only to avoid destabilizing existing plan choices.

Author: Etsuro Fujita
Discussion: https://postgr.es/m/5BFD3EAD.2060301%40lab.ntt.co.jp
---
 contrib/postgres_fdw/expected/postgres_fdw.out |  2 +
 contrib/postgres_fdw/postgres_fdw.c| 56 ++
 contrib/postgres_fdw/sql/postgres_fdw.sql  |  3 ++
 3 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index e653c30..dfa6201 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -3209,6 +3209,8 @@ select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6
  Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE (("C 1" < 100)) AND ((c2 = 6))
 (6 rows)
 
+-- Update local stats on ft2
+ANALYZE ft2;
 -- Add into extension
 alter extension postgres_fdw add operator class my_op_class using btree;
 alter extension postgres_fdw add function my_op_cmp(a int, b int);
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index d22c974..159bf4b 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2844,10 +2844,6 @@ estimate_path_cost_size(PlannerInfo *root,
 			 * strategy will be considered at remote side, thus for
 			 * simplicity, we put all startup related costs in startup_cost
 			 * and all finalization and run cost are added in total_cost.
-			 *
-			 * Also, core does not care about costing HAVING expressions and
-			 * adding that to the costs.  So similarly, here too we are not
-			 * considering remote and local conditions for costing.
 			 */
 
 			ofpinfo = (PgFdwRelationInfo *) fpinfo->outerrel->fdw_private;
@@ -2880,10 +2876,26 @@ estimate_path_cost_size(PlannerInfo *root,
 			input_rows, NULL);
 
 			/*
-			 * Number of rows expected from foreign server will be same as
-			 * that of number of groups.
+			 * Get the retrieved-rows and rows estimates.  If there are HAVING
+			 * quals, account for their selectivity.
 			 */
-			rows = retrieved_rows = numGroups;
+			if (root->parse->havingQual)
+			{
+/* Factor in the selectivity of the remotely-checked quals */
+retrieved_rows =
+	clamp_row_est(numGroups *
+  clauselist_selectivity(root,
+		 fpinfo->remote_conds,
+		 0,
+		 JOIN_INNER,
+		 NULL));
+/* Factor in the selectivity of the locally-checked quals */
+rows = clamp_row_est(retrieved_rows * fpinfo->l

Re: [HACKERS] CLUSTER command progress monitor

[Tatsuro Yamada]

On 2018/11/29 21:20, Dmitry Dolgov wrote:

On Fri, Aug 24, 2018 at 7:06 AM Tatsuro Yamada  
wrote:

On 2017/11/22 6:07, Peter Geoghegan wrote:

On Mon, Oct 2, 2017 at 6:04 AM, Robert Haas  wrote:

Progress reporting on sorts seems like a tricky problem to me, as I
said before.  In most cases, a sort is going to involve an initial
stage where it reads all the input tuples and writes out quicksorted
runs, and then a merge phase where it merges all the output tapes into
a sorted result.  There are some complexities; for example, if the
number of tapes is really large, then we might need multiple merge
phases, only the last of which will produce tuples.


This would ordinarily be the point at which I'd say "but you're very
unlikely to require multiple passes for an external sort these days".
But I won't say that on this thread, because CLUSTER generally has
unusually wide tuples, and so is much more likely to be I/O bound, to
require multiple passes, etc. (I bet the v10 enhancements
disproportionately improved CLUSTER performance.)


Hi,

I came back to develop the feature for community.
V4 patch is corrected these following points:

- Rebase on master (143290efd)
- Fix document
- Replace the column name scan_index_relid with cluster_index_relid.
Thanks to Jeff Janes!

I'm now working on improving the patch based on Robert's comment related to
"Seqscan and Sort case" and also considering how to handle the "Index scan 
case".


Thank you,

Unfortunately, this patch has some conflicts now, could you rebase it? Also
what's is the status of your work on improving it based on the
provided feedback?

In the meantime I'm moving it to the next CF.


Thank you for managing the CF and Sorry for the late reply.
I'll rebase it for the next CF and also I'll clear my head because the patch
needs design change to address the feedbacks, I guess. Therefore, the status is
reconsidering the design of the patch. :)

Regards,
Tatsuro Yamada
NTT Open Source Software Center

Re: make installcheck-world in a clean environment

[Alexander Lakhin]

Hello,

01.12.2018 09:12, Alexander Lakhin wrote:
> 30.11.2018 23:59, Dmitry Dolgov wrote:
>> Hi,
>>
>> I've noticed that for this patch cfbot show strange error
>>
>> USE_INSTALLED_ASSETS=1 make all
> I've fixed the patch.
Rebased the patch once more after d3c09b9b.

Best regards,
Alexander

diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml
index 673a8c2164..9444dee351 100644
--- a/doc/src/sgml/regress.sgml
+++ b/doc/src/sgml/regress.sgml
@@ -111,7 +111,12 @@ make installcheck-parallel
default port number, unless directed otherwise by PGHOST and
PGPORT environment variables.  The tests will be run in a
database named regression; any existing database by this name
-   will be dropped.
+   will be dropped.  The tests in the installcheck mode will use the executables,
+   libraries, and headers, which are already present in the installation tree,
+   not their copies in the source tree (if any).  This mode is suitable for
+   testing distribution packages of PostgreSQL installed on user systems as
+   it allows to check the installation in its entirety without rebuilding
+   any parts of it.
   
 
   
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 956fd274cd..3f5b779823 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -245,14 +245,16 @@ PG_SYSROOT = @PG_SYSROOT@
 
 override CPPFLAGS := $(ICU_CFLAGS) $(CPPFLAGS)
 
-ifdef PGXS
+# For PGXS and in the installcheck mode (when we use the installed assets)
+# we need to target already installed headers and libraries
+ifneq (,$(PGXS)$(USE_INSTALLED_ASSETS))
 override CPPFLAGS := -I$(includedir_server) -I$(includedir_internal) $(CPPFLAGS)
-else # not PGXS
+else # not PGXS and not USE_INSTALLED_ASSETS
 override CPPFLAGS := -I$(top_srcdir)/src/include $(CPPFLAGS)
 ifdef VPATH
 override CPPFLAGS := -I$(top_builddir)/src/include $(CPPFLAGS)
 endif
-endif # not PGXS
+endif # not PGXS and USE_INSTALLED_ASSETS
 
 CC = @CC@
 GCC = @GCC@
@@ -306,7 +308,7 @@ with_gnu_ld = @with_gnu_ld@
 # "LDFLAGS := something" anywhere, ditto for LDFLAGS_INTERNAL.
 # These initial assignments must be "=" type, and elsewhere we must only do
 # "LDFLAGS += something" or "LDFLAGS_INTERNAL += something".
-ifdef PGXS
+ifneq (,$(PGXS)$(USE_INSTALLED_ASSETS))
   LDFLAGS_INTERNAL = -L$(libdir)
 else
   LDFLAGS_INTERNAL = -L$(top_builddir)/src/port -L$(top_builddir)/src/common
@@ -379,6 +381,9 @@ endif
 # install tree just once in any recursive "make check".  The additional test
 # on abs_top_builddir prevents doing anything foolish to the root directory.
 
+# In the installcheck mode we should use what is already installed
+# in the DESTDIR (namely, pg_regress).
+
 check: temp-install
 
 .PHONY: temp-install
@@ -421,7 +426,7 @@ ifeq ($(enable_tap_tests),yes)
 define prove_installcheck
 rm -rf '$(CURDIR)'/tmp_check
 $(MKDIR_P) '$(CURDIR)'/tmp_check
-cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(CURDIR)/$(top_builddir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+cd $(srcdir) && TESTDIR='$(CURDIR)' PATH="$(bindir):$$PATH" PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' PG_REGRESS='$(DESTDIR)$(pgxsdir)/src/test/regress/pg_regress' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
 endef
 
 define prove_check
@@ -564,6 +569,8 @@ endif
 #
 # Commonly used submake targets
 
+# build these libraries only when we are not going to use the installed ones
+ifndef USE_INSTALLED_ASSETS
 submake-libpq: | submake-generated-headers
 	$(MAKE) -C $(libpq_builddir) all
 
@@ -575,6 +582,7 @@ submake-libpgfeutils: | submake-generated-headers
 	$(MAKE) -C $(top_builddir)/src/port all
 	$(MAKE) -C $(top_builddir)/src/common all
 	$(MAKE) -C $(top_builddir)/src/fe_utils all
+endif
 
 .PHONY: submake-libpq submake-libpgport submake-libpgfeutils
 
@@ -617,7 +625,7 @@ pg_regress_check = \
 $(TEMP_CONF) \
 $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS)
 pg_regress_installcheck = \
-$(top_builddir)/src/test/regress/pg_regress \
+   '$(DESTDIR)$(pgxsdir)/src/test/regress/pg_regress' \
 --inputdir=$(srcdir) \
 --bindir='$(bindir)' \
 $(pg_regress_locale_flags) $(EXTRA_REGRESS_OPTS)
diff --git a/src/bin/pg_upgrade/test.sh b/src/bin/pg_upgrade/test.sh
index 1d3272dabf..0ec38ef929 100644
--- a/src/bin/pg_upgrade/test.sh
+++ b/src/bin/pg_upgrade/test.sh
@@ -169,7 +169,7 @@ createdb "$dbname1" || createdb_status=$?
 createdb "$dbname2" || createdb_status=$?
 createdb "$dbname3" || createdb_status=$?
 
-if "$MAKE" -C "$oldsrc" installcheck-parallel; then
+if "$MAKE" -C "$oldsrc" installcheck-parallel DESTDIR="$temp_install"; then
 	oldpgversion=`psql -X -A -t -d regression -c "SHOW server_version_num"`
 
 	# before dumping, get rid of objects not existing in later versions
diff --git a/src/interfaces/ecpg/test/Makefile b/src/interfac

Re: error message when subscription target is a partitioned table

[Magnus Hagander]

On Mon, Dec 3, 2018 at 7:39 AM Tatsuo Ishii  wrote:

> > Hi,
> >
> > Could we improve the error message that's output when the subscription
> > target relation is a partitioned table?  Currently, we get:
> >
> > ERROR:  logical replication target relation "public.foo" is not a table
> >
> > I think it'd be more helpful to get:
> >
> > ERROR: "public.foo" is a partitioned table
> > DETAIL: Partitioned tables are not supported as logical replication
> targets
> >
> > Thoughts?
>
> +1


+1 as well. That is definitely confusing -- to most people, that is still a
table...


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