Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

[Kyotaro HORIGUCHI]

At Thu, 26 Jan 2017 22:34:57 -0500, Tom Lane  wrote in 
<23778.1485488...@sss.pgh.pa.us>
> Craig Ringer  writes:
> > So perhaps:
> 
> > "The same query string may be passed to multiple invocations of
> > ProcessUtility if a utility statement invokes subcommands (e.g. ALTER
> > TABLE), in which case context will be set to
> > PROCESS_UTILITY_SUBCOMMAND, or if the user supplied a query string
> > containing multiple semicolon-separated statements in a single
> > protocol message. It is also possible for the query text to contain
> > other non-utility-statement text like comments, empty  statements, and
> > plannable statements that don't pass through ProcessUtility. Hooks
> > that use the queryString should use pstmt->stmt_location and
> > pstmt->stmt_len to extract the text for the statement of interest and
> > should pay attention to the context to avoid repeatedly handling the
> > same query string in subcommands."
> 
> Meh.  I really don't think that a comment about the query string is
> the place to get into promises that are unrelated to the string, and
> that ProcessUtility is in no position to enforce anyway.  How about
> we just say this:
> 
> "The same queryString may be passed to multiple invocations of
> ProcessUtility when processing a query string containing multiple
> semicolon-separated statements; one should use pstmt->stmt_location and
> pstmt->stmt_len to identify the substring containing the current
> statement.  Keep in mind also that some utility statements (e.g.,
> CREATE SCHEMA) will recurse to ProcessUtility to process sub-statements,
> often passing down the same queryString, stmt_location, and stmt_len
> that were given for the whole statement."

Tom's rewrite is easier to read for me and seems telling me
enough as a user of the hook.

By the way the existing comment for the hook,

> *
> * We provide a function hook variable that lets loadable plugins get
> * control when ProcessUtility is called.  Such a plugin would normally
> * call standard_ProcessUtility().
> */

This is quite a matter of course for developers. planner_hook and
other ones don't have such a comment or have a one-liner at
most. Since this hook gets a good amount of commnet, it seems
better to just remove the existing one..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Kuntal Ghosh]

On Wed, Jan 25, 2017 at 6:00 PM, Haribabu Kommi
 wrote:

> Corrected as suggested.
>
> Updated patch attached. There is no change in the contrib patch.
Got whitspace error warning while applying contrib_macaddr8_1.patch:184.

diff --git a/contrib/btree_gist/btree_gist.control
b/contrib/btree_gist/btree_gist.control
index ddbf83d..fdf0e6a 100644
--- a/contrib/btree_gist/btree_gist.control
+++ b/contrib/btree_gist/btree_gist.control
@@ -1,5 +1,5 @@
 # btree_gist extension
 comment = 'support for indexing common datatypes in GiST'
-default_version = '1.3'
+default_version = '1.4'
btree_gin.control should be updated as well. Otherwise, make
check-world is throwing error.

After making the above changes, I'm able to run regression test
successfully without any error/warning.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


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

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

[Mithun Cy]

On Thu, Jan 26, 2017 at 8:45 PM, Peter Eisentraut
 wrote:
> Just a thought with an additional use case:  If I want to set up a
> standby for offloading queries, could I take the dump file from the
> primary or another existing standby, copy it to the new standby, and
> have it be warmed up to the state of the other instance from that?
>
> In my experience, that kind of use is just as interesting as preserving
> the buffers across a restart.

Initially, I did not think about this thanks for asking. For now, we
dump the buffer pool info in the format
; If BlockNum in
new standby correspond to the same set of rows as it was with the
server where the dump was produced, I think we can directly use the
dump file in new standby. All we need to do is just drop the ".save"
file in data-directory and preload the library. Buffer pool will be
warmed with blocks mentioned in ".save".

-- 
Thanks and Regards
Mithun C Y
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] Parallel bitmap heap scan

[Dilip Kumar]

On Tue, Jan 24, 2017 at 10:18 AM, Dilip Kumar  wrote:
> I have changed as per the comments. 0002 and 0003 are changed, 0001 is
> still the same.

2 days back my colleague Rafia, reported one issue (offlist) that
parallel bitmap node is not scaling as good as other nodes e.g
parallel sequence scan and parallel index scan.

After some perf analysis, I found that there was one unconditional
spin lock in parallel bitmap patch which we were taking for checking
the prefetch target. Basically, we were always taking the lock and
checking the prefetch_target is reached to prefetch_maximum. So even
after it will reach to prefetch_maximum we will acquire the lock and
check after every tuple. I have changed that logic, now I will check
the condition first if we need to increase the target then will take
the lock and recheck the condition.

There is just one line change in 0003 compared to older version, all
other patches are the same.

Some performance data to show that new parallel bitmap patch performs
way better than the previous version.
TPCH scale 20, work_mem 64MB, shared buffers 8GB (4 workers)
machine intel 8 socket machine

v13(time in ms)   v14 (time in ms)
Q637065.84118202.903

Q14 15229.569 11341.121


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


0001-opt-parallelcost-refactoring-v14.patch
Description: Binary data


0002-hash-support-alloc-free-v14.patch
Description: Binary data


0003-parallel-bitmap-heap-scan-v14.patch
Description: Binary data

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

[HACKERS] nodes.h - comments comment

[Erik Rijkers]

Orthography fix in nodes.h comment block.--- src/include/nodes/nodes.h.orig	2017-01-27 06:54:38.704059681 +0100
+++ src/include/nodes/nodes.h	2017-01-27 06:57:17.535250383 +0100
@@ -560,7 +560,7 @@
 
 /*
  * castNode(type, ptr) casts ptr to type and, if cassert is enabled, verifies
- * that the the c actually has the appropriate type (using it's nodeTag()).
+ * that the c actually has the appropriate type (using its nodeTag()).
  *
  * Use an inline function when assertions are enabled, to avoid multiple
  * evaluations of the ptr argument (which could e.g. be a function call).

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

Re: [HACKERS] Logical Replication WIP

[Petr Jelinek]

On 25/01/17 18:16, Peter Eisentraut wrote:
> On 1/22/17 8:11 PM, Petr Jelinek wrote:
>> 0001 - Changes the libpqrcv_connect to use async libpq api so that it
>> won't get stuck forever in case of connect is stuck. This is preexisting
>> bug that also affects walreceiver but it's less visible there as there
>> is no SQL interface to initiate connection there.
> 
> Probably a mistake here:
> 
> +   case PGRES_POLLING_READING:
> +   extra_flag = WL_SOCKET_READABLE;
> +   /* pass through */
> +   case PGRES_POLLING_WRITING:
> +   extra_flag = WL_SOCKET_WRITEABLE;
> 
> extra_flag gets overwritten in the reading case.
> 

Eh, reworked that to just if statement as switch does not really buy us
anything there.

> Please elaborate in the commit message what this change is for.
> 

Okay.

>> 0002 - Close replication connection when CREATE SUBSCRIPTION gets
>> canceled (otherwise walsender on the other side may stay in idle in
>> transaction state).
> 
> committed

Thanks!

> 
>> 0003 - Fixes buffer initialization in walsender that I found when
>> testing the above two. This one should be back-patched to 9.4 since it's
>> broken since then.
> 
> Can you explain more in which code path this problem occurs?

With existing code base, anything that calls WalSndWaitForWal (it calls
ProcessRepliesIfAny()) which is called from logical_read_xlog_page which
is given as callback to logical decoding in CreateReplicationSlot and
StartLogicalReplication.

The reason why I decided to put it into init is that following up all
the paths to where buffers are used is rather complicated due to various
callbacks so if anybody else starts poking around in the future it might
get easily broken again if we don't initialize those unconditionally
(plus the memory footprint is few kB and in usual use of WalSender they
will eventually be initialized anyway as they are needed for streaming).

> I think we should get rid of the global variables and give each function
> its own buffer that it initializes the first time through.  Otherwise
> we'll keep having to worry about this.
> 

Because of above, it would mean some refactoring in logical decoding
APIs not just in WalSender so that would not be backpatchable (and in
general it's much bigger patch then).

>> 0004 - Fixes the foreign key issue reported by Thom Brown and also adds
>> tests for FK and trigger handling.
> 
> I think the trigger handing should go into execReplication.c.
> 

Not in the current state, eventually (and I am afraid that PG11 material
at this point as we still have partitioned tables support and initial
data copy to finish in this release) we'll want to move all the executor
state code to execReplication.c and do less of reinitialization but in
the current code the trigger stuff belongs to worker IMHO.

>> 0005 - Adds support for renaming publications and subscriptions.
> 
> Could those not be handled in the generic rename support in
> ExecRenameStmt()?

Yes seems they can.

Attached updated version of the uncommitted patches.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 980ce872862e7a9abcbec14864721103507b5136 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Fri, 20 Jan 2017 21:20:56 +0100
Subject: [PATCH 1/4] Use asynchronous connect API in libpqwalreceiver

This makes the connection attempt from CREATE SUBSCRIPTION and from
WalReceiver interruptable by user in case the libpq connection is
hanging. The previous coding required immediate shutdown (SIGQUIT) of
PostgreSQL in that situation.
---
 src/backend/postmaster/pgstat.c|  4 +-
 .../libpqwalreceiver/libpqwalreceiver.c| 51 +-
 src/include/pgstat.h   |  2 +-
 3 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 7176cf1..19ad6b5 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3340,8 +3340,8 @@ pgstat_get_wait_client(WaitEventClient w)
 		case WAIT_EVENT_WAL_RECEIVER_WAIT_START:
 			event_name = "WalReceiverWaitStart";
 			break;
-		case WAIT_EVENT_LIBPQWALRECEIVER_READ:
-			event_name = "LibPQWalReceiverRead";
+		case WAIT_EVENT_LIBPQWALRECEIVER:
+			event_name = "LibPQWalReceiver";
 			break;
 		case WAIT_EVENT_WAL_SENDER_WAIT_WAL:
 			event_name = "WalSenderWaitForWAL";
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 44a89c7..536324c 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -112,6 +112,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
  char **err)
 {
 	WalReceiverConn 

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

[Beena Emerson]

On Fri, Jan 27, 2017 at 8:14 AM, Amit Kapila 
wrote:

> On Thu, Jan 26, 2017 at 8:45 PM, Peter Eisentraut
>  wrote:
> > On 1/24/17 3:26 AM, Mithun Cy wrote:
> >> In my code by default, we only dump at shutdown time. If we want to
> >> dump at regular interval then we need to set the GUC
> >> pg_autoprewarm.buff_dump_interval to > 0.
> >
> > Just a thought with an additional use case:  If I want to set up a
> > standby for offloading queries, could I take the dump file from the
> > primary or another existing standby, copy it to the new standby, and
> > have it be warmed up to the state of the other instance from that?
> >
> > In my experience, that kind of use is just as interesting as preserving
> > the buffers across a restart.
> >
>
> An interesting use case.  I am not sure if somebody has tried that way
> but it appears to me that the current proposed patch should work for
> this use case.
>
>
Even I feel this should work.
In that case, we could add the file location parameter.  By default it
would store in the cluster directory else in the location provided. We can
update this parameter in standby for it to access the file.
Thoughts?


-- 
Thank you,

Beena Emerson

Have a Great Day!

[HACKERS] Typo in comment in postgres_fdw.c

[Etsuro Fujita]

Hi,

I ran into a typo in a comment in contrib/postgres_fdw/postgres_fdw.c.  
Attached is a small patch for fixing that.


Best regards,
Etsuro Fujita
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index fbe6929..7cb9dc5 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4218,7 +4218,7 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 	/*
 	 * If user is willing to estimate cost for a scan of either of the joining
 	 * relations using EXPLAIN, he intends to estimate scans on that relation
-	 * more accurately. Then, it makes sense to estimate the cost the join
+	 * more accurately. Then, it makes sense to estimate the cost of the join
 	 * with that relation more accurately using EXPLAIN.
 	 */
 	fpinfo->use_remote_estimate = fpinfo_o->use_remote_estimate ||

-- 
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] [GENERAL] C++ port of Postgres

[Andres Freund]

Hi Peter,

On 2016-09-30 15:24:09 -0400, Peter Eisentraut wrote:
> Yeah, I have committed a few of the patches now and I'll close the CF
> entry now.  Thanks for your research.

Are you planning to push more of these at some point?

- Andres


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

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

[Tom Lane]

Craig Ringer  writes:
> So perhaps:

> "The same query string may be passed to multiple invocations of
> ProcessUtility if a utility statement invokes subcommands (e.g. ALTER
> TABLE), in which case context will be set to
> PROCESS_UTILITY_SUBCOMMAND, or if the user supplied a query string
> containing multiple semicolon-separated statements in a single
> protocol message. It is also possible for the query text to contain
> other non-utility-statement text like comments, empty  statements, and
> plannable statements that don't pass through ProcessUtility. Hooks
> that use the queryString should use pstmt->stmt_location and
> pstmt->stmt_len to extract the text for the statement of interest and
> should pay attention to the context to avoid repeatedly handling the
> same query string in subcommands."

Meh.  I really don't think that a comment about the query string is
the place to get into promises that are unrelated to the string, and
that ProcessUtility is in no position to enforce anyway.  How about
we just say this:

"The same queryString may be passed to multiple invocations of
ProcessUtility when processing a query string containing multiple
semicolon-separated statements; one should use pstmt->stmt_location and
pstmt->stmt_len to identify the substring containing the current
statement.  Keep in mind also that some utility statements (e.g.,
CREATE SCHEMA) will recurse to ProcessUtility to process sub-statements,
often passing down the same queryString, stmt_location, and stmt_len
that were given for the whole statement."

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] Proposal : For Auto-Prewarm.

[Amit Kapila]

On Thu, Jan 26, 2017 at 8:45 PM, Peter Eisentraut
 wrote:
> On 1/24/17 3:26 AM, Mithun Cy wrote:
>> In my code by default, we only dump at shutdown time. If we want to
>> dump at regular interval then we need to set the GUC
>> pg_autoprewarm.buff_dump_interval to > 0.
>
> Just a thought with an additional use case:  If I want to set up a
> standby for offloading queries, could I take the dump file from the
> primary or another existing standby, copy it to the new standby, and
> have it be warmed up to the state of the other instance from that?
>
> In my experience, that kind of use is just as interesting as preserving
> the buffers across a restart.
>

An interesting use case.  I am not sure if somebody has tried that way
but it appears to me that the current proposed patch should work for
this use case.

-- 
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] Microvacuum support for Hash Index

[Amit Kapila]

On Thu, Jan 26, 2017 at 6:38 PM, Jesper Pedersen
 wrote:
> On 01/23/2017 02:53 PM, Jesper Pedersen wrote:
>>
>> I have done some more testing with this, and have moved to the patch
>> back to 'Needs Review' pending Amit's comments.
>>
>
> Moved to "Ready for Committer".
>

Don't you think we should try to identify the reason of the deadlock
error reported by you up thread [1]?  I know that you and Ashutosh are
not able to reproduce it, but still I feel some investigation is
required to find the reason.  It is quite possible that the test case
is such that the deadlock is expected in rare cases, if that is the
case then it is okay.  I have not spent enough time on that to comment
whether it is a test or code issue.


[1]  - 
https://www.postgresql.org/message-id/dc6d7247-050f-4014-8c80-a4ee676eb384%40redhat.com


-- 
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] pg_hba_file_settings view patch

[Michael Paquier]

On Thu, Jan 26, 2017 at 11:36 PM, Tom Lane  wrote:
> Haribabu Kommi  writes:
>> This patch currently doesn't have the code for reporting the two log
>> messages that can occur in tokenize_file function. To support the same,
>> I am thinking of changing line_nums list to line_info list that can
>> contain both line number and the error message that occurred during the
>> tokenize. This list data is used to identify whether that line is having
>> any error or not before parsing that hba line, and directly report that
>> line as error in the view.
>
> Yeah, perhaps.  tokenize_file() has pushed the return-parallel-lists
> notion to the limit of sanity already.  It would make more sense to
> change it to return a single list containing one struct per line,
> which would include the token list, raw line text, and line number.
>
> It might make sense to proceed by writing a separate patch that just
> refactors the existing code to have an API like that, and then revise
> this patch to add an error message field to the per-line struct.  Or
> maybe that's just extra work, not sure.

Beginning with a cleaner state the feature implementation would likely
facilitate the restructuring work of pg_hba_rules and its overall
size, so doing the refactoring work first would make the most sense.
-- 
Michael


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

Re: [HACKERS] [COMMITTERS] pgsql: Check interrupts during hot standby waits

[Michael Paquier]

On Fri, Jan 27, 2017 at 4:06 AM, Andres Freund  wrote:
> Hi,
>
> On 2017-01-26 19:00:34 +, Simon Riggs wrote:
>> Check interrupts during hot standby waits
>>
>> Branch
>> --
>> master
>>
>> Details
>> ---
>> http://git.postgresql.org/pg/commitdiff/e8ee3d6b859a18d7f7375ceb9e04d256eb18aaec
>>
>> Modified Files
>> --
>> src/backend/storage/ipc/standby.c | 2 ++
>> 1 file changed, 2 insertions(+)
>
> It seems that the actual fix here would be to replace the pg_usleep loop
> with a latch wait Then we also don't need to needlessly loop, and
> actually react promptly to signals.

For the record, a patch has been posted on the original thread:
https://www.postgresql.org/message-id/CAB7nPqRNcd+FR=8_lyb65jbcrpx+59hxobi5g-wf4fxrak_...@mail.gmail.com
-- 
Michael


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Michael Paquier]

On Fri, Jan 27, 2017 at 2:21 AM, Robert Haas  wrote:
> On Tue, Jan 24, 2017 at 4:47 PM, Robert Haas  wrote:
>>> But I don't see any proposals to actually change all uses of "xlog" to
>>> "wal".  What about program names, command line options, etc.?  If the
>>> argument is, we changed one thing, we should change the rest, then let's
>>> see that.  I think that argument itself is flawed, but if that's what
>>> we're going with, let's see the whole plan.
>>
>> I'm happy to go change every last bit of it.  I was expecting after I
>> committed the initial rename that somebody would provide a follow-on
>> patch to do the rest of it in short order.  Instead, months went by
>> and we still don't have a complete patch.  But I don't see why that
>> has to take more than a day's work, probably just a few hours.  I'd
>> like to do that and move on.
>
> And here are patches for that.
> 0001 renames everything that contains "xlog" in pg_proc.h to refer to "wal".
> 0002 renames programs whose names contains "xlog".
> 0003 renames the dtrace probes whose names contain "xlog".
> 0004 renames command line options which contain "xlog".
>
> There are probably a few more things that could be done afterwards to
> clean up other odds and ends, but I think this gets the vast bulk of
> the user-visible references to xlog.

Thanks for doing this work. Jumping on the train, here is a review for
those patches. All of them are very mechanical changes. Just:
-
+
 
A nit about the number of spaces here.

In protocol.sgml:
  
  
   xlogpos (text)
  
  
  
   Current xlog flush location. Useful to get a known location in the
   transaction log where streaming can start.
  
  
  
You want to say WAL here instead of xlog.

In storage.sgml, similar thing:
  
   pd_lsn
   PageXLogRecPtr
   8 bytes
   LSN: next byte after last byte of xlog record for last change
   to this page
  
s/xlog/WAL/

In pg_standby.c:
pg_standby.c:   printf("  %s [OPTION]... ARCHIVELOCATION NEXTWALFILE
XLOGFILEPATH [RESTARTWALFILE]\n", progname);
s/XLOGFILEPATH/WALFILEPATH/.

All the other references to "xlog" are in the code comments, for the
user-facing changes your set of patches is enough.
-- 
Michael


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

Re: [HACKERS] Parallel Index Scans

[Amit Kapila]

On Fri, Jan 27, 2017 at 6:53 AM, Haribabu Kommi
 wrote:
>
>
> On Mon, Jan 23, 2017 at 5:07 PM, Amit Kapila 
> wrote:
>>
>> On Fri, Jan 20, 2017 at 7:29 AM, Haribabu Kommi
>>  wrote:
>> > I didn't find any better names other than the following,
>> >
>> > _bt_get_next_parallel_page
>> > _bt_set_next_parallel_page
>> >
>>
>> I am not sure using *_next_* here will convey the message because for
>> backward scans we set the last page.  I am open to changing the names
>> of functions if committer and or others prefer the names suggested by
>> you.
>
>
> OK. I am fine with it.
> I don't have any other comments on the patch.
>

Thanks for the review.

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


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

Re: [HACKERS] increasing the default WAL segment size

[Michael Paquier]

On Fri, Jan 27, 2017 at 4:20 AM, Andres Freund  wrote:
> On 2017-01-25 12:26:21 +0900, Michael Paquier wrote:
>> diff --git a/src/backend/access/common/tupdesc.c 
>> b/src/backend/access/common/tupdesc.c
>> index 083c0303dc..2eb3a420ac 100644
>> --- a/src/backend/access/common/tupdesc.c
>> +++ b/src/backend/access/common/tupdesc.c
>> @@ -629,6 +629,14 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
>>   att->attstorage = 'p';
>>   att->attcollation = InvalidOid;
>>   break;
>> +
>> + case INT8OID:
>> + att->attlen = 8;
>> + att->attbyval = true;
>> + att->attalign = 'd';
>> + att->attstorage = 'p';
>> + att->attcollation = InvalidOid;
>> + break;
>>   }
>>  }
>
> INT8 isn't unconditionally byval, is it?

Doh. Of course.

>>   /* slot_name */
>> - len = strlen(NameStr(MyReplicationSlot->data.name));
>> - pq_sendint(, len, 4);   /* col1 len */
>> - pq_sendbytes(, NameStr(MyReplicationSlot->data.name), len);
>> + values[0] = 
>> PointerGetDatum(cstring_to_text(NameStr(MyReplicationSlot->data.name)));
>
> That seems a bit long.

Sure. What about that:
-   len = strlen(NameStr(MyReplicationSlot->data.name));
-   pq_sendint(, len, 4);   /* col1 len */
-   pq_sendbytes(, NameStr(MyReplicationSlot->data.name), len);
+   slot_name = NameStr(MyReplicationSlot->data.name);
+   values[0] = PointerGetDatum(cstring_to_text(slot_name));

> I've not done like the most careful review ever, but I'm in favor of the
> general change (provided the byval thing is fixed obviously).

Thanks for the review.
-- 
Michael


refactor-repl-cmd-output-v2.patch
Description: Binary data

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

Re: [HACKERS] Allow interrupts on waiting standby

[Michael Paquier]

On Fri, Jan 27, 2017 at 4:36 AM, Simon Riggs  wrote:
> On 26 January 2017 at 19:20, Andres Freund  wrote:
>> On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
>>> On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
>>> > Currently a waiting standby doesn't allow interrupts.
>>> >
>>> > Patch implements that.
>>> >
>>> > Barring objection, patching today with backpatches.
>>>
>>> "today" is a little quick, but the patch looks fine.  I doubt anyone's
>>> going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.
>>
>> I don't quite get asking for agreement, and then not waiting as
>> suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
>> for now, but I think it'd better to replace it with a latch.
>
> I have waited, so not sure what you mean. Tomorrow is too late.

This gives really little time for any feedback :(

> Replacing with a latch wouldn't be backpatchable, IMHO.
> I've no problem if you want to work on a deeper fix for future versions.

A deeper fix for HEAD proves to not be that complicated. Please see
the attached. The other two calls of pg_usleep() in standby.c are
waiting with 5ms and 10ms, it is not worth switching them to a latch.
-- 
Michael


standby-delay-latch.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] safer node casting

[Andres Freund]

On 2017-01-26 20:29:06 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> >> This is inspired by the dynamic_cast operator in C++, but follows the
> >> syntax of the well-known makeNode() macro.
> 
> > The analogy to dynamic_cast goes only so far, because we don't actually
> > support inheritance.  I.e. in c++ we could successfully cast SeqScanState 
> > to a
> > PlanState, ScanState and SeqScanState - but with our model only
> > SeqScanState can be checked.
> 
> Yeah, I was thinking about that earlier --- this can only be used to cast
> to a concrete node type, not one of the "abstract" types like Plan * or
> Expr *.  Not sure if that's worth worrying about though; I don't think
> I've ever seen actual bugs in PG code from casting the wrong thing in that
> direction.  For the most part, passing the wrong thing would end up firing
> a default: case in a switch, or some such, so we already do have some
> defenses for that direction.

Yea, I'm not actually worried about it - I was more generally remarking
on the analogy made by Peter. For a second I was considering bringing up
the analogy in a comment or such, and this was one of a number of
arguments that made me disregard that idea.

Andres


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

Re: [HACKERS] safer node casting

[Tom Lane]

Andres Freund  writes:
> On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
>> This is inspired by the dynamic_cast operator in C++, but follows the
>> syntax of the well-known makeNode() macro.

> The analogy to dynamic_cast goes only so far, because we don't actually
> support inheritance.  I.e. in c++ we could successfully cast SeqScanState to a
> PlanState, ScanState and SeqScanState - but with our model only
> SeqScanState can be checked.

Yeah, I was thinking about that earlier --- this can only be used to cast
to a concrete node type, not one of the "abstract" types like Plan * or
Expr *.  Not sure if that's worth worrying about though; I don't think
I've ever seen actual bugs in PG code from casting the wrong thing in that
direction.  For the most part, passing the wrong thing would end up firing
a default: case in a switch, or some such, so we already do have some
defenses for that direction.

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] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Tue, Jan 24, 2017 at 3:59 PM, Dilip Kumar  wrote:

>
> Overall patch looks fine to me and marking it "ready for committer".
>
> There is two design decision, which I leave it for committer's decision.
>
> 1.  EXECUTE statement should show only as EXECUTE count, not the
> actual SQL query.
> 2.  should we count statement execute from Functions or only statement
> what is executed directly by the user as it's doing now?


Thanks for the review.
Let's wait for the committer's opinion.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Parallel Index Scans

[Haribabu Kommi]

On Mon, Jan 23, 2017 at 5:07 PM, Amit Kapila 
wrote:

> On Fri, Jan 20, 2017 at 7:29 AM, Haribabu Kommi
>  wrote:
> >
> > On Thu, Jan 19, 2017 at 1:18 AM, Amit Kapila 
> > wrote:
> >> >
> >> > +extern BlockNumber _bt_parallel_seize(IndexScanDesc scan, bool
> >> > *status);
> >> > +extern void _bt_parallel_release(IndexScanDesc scan, BlockNumber
> >> > scan_page);
> >> >
> >> > Any better names for the above functions, as these function will
> >> > provide/set
> >> > the next page that needs to be read.
> >> >
> >>
> >> These functions also set the state of scan.  IIRC, these names were
> >> being agreed between Robert and Rahila as well (suggested offlist by
> >> Robert).  I am open to change if you or others have any better
> >> suggestions.
> >
> >
> > I didn't find any better names other than the following,
> >
> > _bt_get_next_parallel_page
> > _bt_set_next_parallel_page
> >
>
> I am not sure using *_next_* here will convey the message because for
> backward scans we set the last page.  I am open to changing the names
> of functions if committer and or others prefer the names suggested by
> you.


OK. I am fine with it.
I don't have any other comments on the patch.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] safer node casting

[Andres Freund]

Hi,

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> This is inspired by the dynamic_cast operator in C++, but follows the
> syntax of the well-known makeNode() macro.

The analogy to dynamic_cast goes only so far, because we don't actually
support inheritance.  I.e. in c++ we could successfully cast SeqScanState to a
PlanState, ScanState and SeqScanState - but with our model only
SeqScanState can be checked.

Greetings,

Andres


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

Re: [HACKERS] safer node casting

[Andres Freund]

On 2017-01-26 17:27:45 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > #if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
> > is probably a better gatekeeper in the back-branches, than gcc?
> 
> Ah, yeah, that would work --- I'd already swapped out that business ;-)

Done that way.

- Andres


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

Andres,

* Andres Freund (and...@anarazel.de) wrote:
> On 2017-01-26 19:01:54 -0500, Stephen Frost wrote:
> > * Andres Freund (and...@anarazel.de) wrote:
> > > I hear these complaints about postgres most frequently: 1) replication
> > > sucks. 2) way too slow on analytics queries. 3) existing admin tools
> > > suck. 4) self written admin tools (required due to 3)) constantly break.
> > > 
> > > There's a lot being done on 1) and 2). There's very little in-core
> > > progress about 3). We're getting worse on 4).
> > 
> > I certainly hear some of these complaints also, and I'd love it if '3'
> > were something that the project was focused on, but, well, I really
> > don't see pgAdmin ever being in core, and 99% of the time that I'm
> > talking to end users, that's really what they're looking for (or, well,
> > something like it).  I don't recall, off-hand at least, ever running
> > into a user complaining that their pgAdmin-like admin tool broke.
> 
> For me it's it's not a pgadmin alike (or rather a more featureful
> version) that's breaking the camel's back.  By far the biggest one I
> hear is auto-HA.  There's no in-core support, and pretty much all of the
> stuff out there is immature as hell.

Oh, that's not what I tend to think of as 'admin tool'.  I completely
agree with you on #1 and also that we need something better for HA.

> And even if it weren't people for
> good reason don't trust such core aspects to $random_project.  Good,
> easy to set up, monitoring is another thing - and I don't thing
> check_postgres is a significant portion of that, it doesn't have much in
> the way of guidelines, and actually pointing people to where problems
> are.

My comment with check_postgres was more of a 'it seems unlikely we would
get in something as simple as this', not to suggest in any way that it's
the end-all, be-all of monitoring.

And now to, unashamedly, push my own agenda- any ideas about something
that would help with any of this that could be accomplished over a
summer by a student?  Even if you don't want to mentor, having a good
description, what skills are needed, the difficulty level, and the
expected outcomes might allow someone else to mentor...

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On 2017-01-26 19:01:54 -0500, Stephen Frost wrote:
> Andres,
> 
> * Andres Freund (and...@anarazel.de) wrote:
> > I hear these complaints about postgres most frequently: 1) replication
> > sucks. 2) way too slow on analytics queries. 3) existing admin tools
> > suck. 4) self written admin tools (required due to 3)) constantly break.
> > 
> > There's a lot being done on 1) and 2). There's very little in-core
> > progress about 3). We're getting worse on 4).
> 
> I certainly hear some of these complaints also, and I'd love it if '3'
> were something that the project was focused on, but, well, I really
> don't see pgAdmin ever being in core, and 99% of the time that I'm
> talking to end users, that's really what they're looking for (or, well,
> something like it).  I don't recall, off-hand at least, ever running
> into a user complaining that their pgAdmin-like admin tool broke.

For me it's it's not a pgadmin alike (or rather a more featureful
version) that's breaking the camel's back.  By far the biggest one I
hear is auto-HA.  There's no in-core support, and pretty much all of the
stuff out there is immature as hell. And even if it weren't people for
good reason don't trust such core aspects to $random_project.  Good,
easy to set up, monitoring is another thing - and I don't thing
check_postgres is a significant portion of that, it doesn't have much in
the way of guidelines, and actually pointing people to where problems
are.

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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

Andres,

* Andres Freund (and...@anarazel.de) wrote:
> I hear these complaints about postgres most frequently: 1) replication
> sucks. 2) way too slow on analytics queries. 3) existing admin tools
> suck. 4) self written admin tools (required due to 3)) constantly break.
> 
> There's a lot being done on 1) and 2). There's very little in-core
> progress about 3). We're getting worse on 4).

I certainly hear some of these complaints also, and I'd love it if '3'
were something that the project was focused on, but, well, I really
don't see pgAdmin ever being in core, and 99% of the time that I'm
talking to end users, that's really what they're looking for (or, well,
something like it).  I don't recall, off-hand at least, ever running
into a user complaining that their pgAdmin-like admin tool broke.

I have doubts that we could even get something like check_postgres or
another monitoring type of tool into core, though it would be so
nice to have tight integration between montioring and the core code.
That would require some kind of pluggable system which could support the
montioring-of-the-day and an easy way to extend it and a way to set up
what should be monitored and what shouldn't along with something to
actually gather that info while the system is running (eg: a background
worker of some kind), or at least, those are my general thoughts.

Maybe we'll get to the point where we'd have a fully-baked, featureful
backup solution that works with large-scale systems in core; I've at
least got some hopes of that happening, but not any particular timeline
or expectation of it happening soon.

Having a good way to do logging of different levels to different
locations would also be really nice, because our current logging
situation is really rather terrible.

> > I actually took your response as: "why the f**k is he talking about
> > camels?" and started laughing...
> 
> That's good then, and let's raise a $beverage_of_choice tonight to the
> fallability of e-mail conversation ;)

Hah, now that I can certainly agree with..

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[David G. Johnston]

On Thu, Jan 26, 2017 at 4:19 PM, Andres Freund  wrote:

>
> We decided s/pg_xlog/pg_wal/ was necessary because people lost their
> data, and we couldn't come up with a reasonable way to change it without
> the name. The tradeoff is dataloss vs. dealing with directory renaming
> in a few lowlevel tools.  So we decided to change the name.  It seems
> breakage was unavoidable.
>
> For me the renaming of functions, binaries, etc, is different because
> the benefit is long-term avoidance of a bit of confusion, with the
> shorter term price being tool breakages and confusion because
> documentation/guides/... for different versions don't apply anymore, and
> the search terms don't help anymore.  But we're still changing this,
> even though breakage seems avoidable.
>

Well stated and compelling.
​

>
> And that fits into the bigger topic of us doing minor cleanups without a
> lot of concern for backward compatibility, e.g. like us whacking things
> around in pg_stat_activity for most of the last releases - nearly all of
> those could have been done in a compatible manner, without even smelling
> that badly.
>
> I hear these complaints about postgres most frequently: 1) replication
> sucks. 2) way too slow on analytics queries. 3) existing admin tools
> suck. 4) self written admin tools (required due to 3)) constantly break.
>
> There's a lot being done on 1) and 2). There's very little in-core
> progress about 3). We're getting worse on 4).
>

​I would posit, broadly, and without any evidence, that changes like we are
discussing here, will go toward helping on 3 since (hopefully) as
PostgreSQL becomes a more appealing option in the marketplace more quality
developers will be drawn toward using their skills to improve its
ecosystem.  Mandating uniformity in areas like this speaks toward
professionalism.

Given that "there's a lot being done on #1" it would seem that right now is
an excellent time to make these changes - so that the new guides and tools
that leverage those enhancements can use the WAL terminology and have its
presence be consistently present throughout the system.

If there was a reasonably short horizon for features or capabilities that
make this kind of renaming breaking easier to accommodate I would say we
could probably wait for it to be completed first.  But AFAIK there isn't
anything in the works that would allow individual users to easily enable a
backward-compatibility mode for the mid-level API that we are largely
touching here.

It may be that this is a straw that breaks the camel's back for some users
of PostgreSQL who are fed up with #4 ... I don't know ... but I'm
reasonably confident that new users a couple of years from now would have a
better experience with our product with these changes in place.  Its an
aggressive play for growth - and that entails risk and upsetting those
invested in the status-quo (which are are already doing per your #1).

David J.

Re: [HACKERS] Speedup twophase transactions

[Michael Paquier]

On Thu, Jan 26, 2017 at 5:02 PM, Nikhil Sontakke
 wrote:
>>> Umm, AFAICS, CheckPointTwoPhase() does not get called in the "standby
>>> promote" code path.
>>
>> CreateRestartPoint() calls it via CheckPointGuts() while in recovery.
>
> May be that I am missing something.
>
> But, I put the recovery process and the checkpointer process of the
> standby under gdb with breakpoints on these functions, but both did
> not hit CreateRestartPoint() as well as CheckPointGuts() when I issued
> a promote :-|

No end-of-recovery checkpoints happen at promotion since 9.3. You can
still use fallback_promote as promote file to trigger the pre-9.2 (9.2
included) behavior.
-- 
Michael


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

Re: [HACKERS] logical decoding of two-phase transactions

[Craig Ringer]

On 26 January 2017 at 19:34, Stas Kelvich  wrote:

> Imagine following scenario:
>
> 1. PREPARE happend
> 2. PREPARE decoded and sent where it should be sent
> 3. We got all responses from participating nodes and issuing COMMIT/ABORT
> 4. COMMIT/ABORT decoded and sent
>
> After step 3 there is no more memory state associated with that prepared tx, 
> so if will fail
> between 3 and 4 then we can’t know GID unless we wrote it commit record (or 
> table).


If the decoding session crashes/disconnects and restarts between 3 and
4, we know the xact is now committed or rolled backand we don't care
about its gid anymore, we can decode it as a normal committed xact or
skip over it if aborted. If Pg crashes between 3 and 4 the same
applies, since all decoding sessions must restart.

No decoding session can ever start up between 3 and 4 without passing
through 1 and 2, since we always restart decoding at restart_lsn and
restart_lsn cannot be advanced past the assignment (BEGIN) of a given
xid until we pass its commit record and the downstream confirms it has
flushed the results.

The reorder buffer doesn't even really need to keep track of the gid
between 3 and 4, though it should do to save the output plugin and
downstream the hassle of keeping an xid to gid mapping. All it needs
is to know if we sent a given xact's data to the output plugin at
PREPARE time, so we can suppress sending them again at COMMIT time,
and we can store that info on the ReorderBufferTxn. We can store the
gid there too.

We'll need two new output plugin callbacks

   prepare_cb
   rollback_cb

since an xact can roll back after we decode PREPARE TRANSACTION (or
during it, even) and we have to be able to tell the downstream to
throw the data away.

I don't think the rollback callback should be called
abort_prepared_cb, because we'll later want to add the ability to
decode interleaved xacts' changes as they are made, before commit, and
in that case will also need to know if they abort. We won't care if
they were prepared xacts or not, but we'll know based on the
ReorderBufferTXN anyway.

We don't need a separate commit_prepared_cb, the existing commit_cb is
sufficient. The gid will be accessible on the ReorderBufferTXN.

Now, if it's simpler to just xlog the gid at COMMIT PREPARED time when
wal_level >= logical I don't think that's the end of the world. But
since we already have almost everything we need in memory, why not
just stash the gid on ReorderBufferTXN?

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


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

Re: [HACKERS] Performance improvement for joins where outer side is unique

[Tom Lane]

To re-familiarize myself with this patch, I've been re-reading the thread,
which has gotten quite long.  It seemed like it would be a good idea to
stop and try to summarize what the patch ought to accomplish, because
there's been some drift over the more than 2 years the patch has been
in the works.

So: ISTM there are two core ideas at this point.

1. We should recognize when the inner side of a join is unique (that is,
there is provably no more than one inner tuple joining to any given outer
tuple), and make use of that knowledge to short-circuit execution in the
same way we already do for JOIN_SEMI and JOIN_ANTI cases.  That is, once
we find a match we can immediately move on to the next outer tuple rather
than continuing to scan for inner-side matches.

2. In these same cases (unique/semi/anti joins), it is possible to avoid
mark/restore overhead in a mergejoin, because we can tweak the executor
logic to not require backing up the inner side.  This goes further than
just tweaking the executor logic, though, because if we know we don't
need mark/restore then that actually makes some plan shapes legal that
weren't before: we don't need to interpose a Material node to protect
join inputs that can't mark/restore.

Maybe I missed something, but it doesn't look like the current patch
(unique_joins_2017-01-27_no_outer_unique.patch) has anything concerning
point #2 at all.  It might make sense to address that idea as a follow-on
patch, but I think it can be a quite significant win and we shouldn't
just lose track of it.

Anyway, having laid out that scope of work, I have some concerns:

* The patch applies point #1 to only INNER and LEFT join modes, but
I don't really see why the idea wouldn't work for RIGHT and FULL modes,
ie the optimization seems potentially interesting for all executable join
types.  Once you've got a match, you can immediately go to the next outer
tuple instead of continuing to scan inner.  (Am I missing something?)

* Particularly in view of the preceding point, I'm not that happy with
the way that management/caching of the "is it unique" knowledge is
done completely differently for INNER and LEFT joins.  I wonder if
there's actually a good argument for that or is it mostly a development
sequence artifact.  IOW, would it hurt to drop the SpecialJoinInfo
tie-in and just rely on the generic cache?

* Because of where we apply the short-circuit logic in the executor,
it's only safe to consider the inner rel as unique if it is provably
unique using only the join clauses that drive the primary join mechanism
(ie, the "joinquals" not the "otherquals").  We already do ignore quals
that are pushed-down to an outer join, so that's good, but there's an
oversight: we will use a qual that is mergejoinable even if it's not
hashjoinable. That means we could get the wrong answers in a hash join.
I think probably the appropriate fix for the moment is just to consider
only clauses that are both mergeable and hashable while trying to prove
uniqueness.  We do have some equality operators that support only one
or the other, but they're corner cases, and I'm dubious that it's worth
having to make separate proofs for merge and hash joins in order to
cater to those cases.

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] BUG: pg_stat_statements query normalization issues with combined queries

[Craig Ringer]

On 26 January 2017 at 21:42, Tom Lane  wrote:
> Craig Ringer  writes:
>> One suggestion: it's currently non-obvious that ProcessUtility_hook
>> gets called with the full text of all parts of a multi-statement.
>
> OK, we can improve that ...
>
>>   The same query string may be passed to multiple invocations of 
>> ProcessUtility
>>   if a utility statement in turn invokes other utility statements, or if the
>>   user supplied a query string containing multiple semicolon-separated
>>   statements in a single protocol message. It is also possible for the query
>>   text to contain other non-utility-statement text like comments, empty
>>   statements, and plannable statements. Callers that use the queryString
>>   should use pstmt->stmt_location and pstmt->stmt_len to extract the text for
>>   the statement of interest and should guard against re-entrant invocation.
>
> Not sure about the reference to re-entrancy.  It's not especially relevant
> to query texts AFAICS, and wouldn't a utility statement know darn well if
> it was doing something that could end up invoking another instance of
> itself?

The utility statement does, but the hooks don't necessarily. If you fire an

ALTER TABLE ...
  ADD COLUMN ..
  ADD COLUMN ..
  ADD CONSTRAINT ..;

for example.

However, I was wrong to say we must guard against re-entrancy. We
should only enter ProcessUtility once with context ==
PROCESS_UTILITY_QUERY, and that's what hooks should be looking at
rather than keeping track of re-entrant invocations.

So perhaps:


"The same query string may be passed to multiple invocations of
ProcessUtility if a utility statement invokes subcommands (e.g. ALTER
TABLE), in which case context will be set to
PROCESS_UTILITY_SUBCOMMAND, or if the user supplied a query string
containing multiple semicolon-separated statements in a single
protocol message. It is also possible for the query text to contain
other non-utility-statement text like comments, empty  statements, and
plannable statements that don't pass through ProcessUtility. Hooks
that use the queryString should use pstmt->stmt_location and
pstmt->stmt_len to extract the text for the statement of interest and
should pay attention to the context to avoid repeatedly handling the
same query string in subcommands."



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


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On 2017-01-26 15:45:15 -0700, David G. Johnston wrote:
> On Thu, Jan 26, 2017 at 3:28 PM, Andres Freund  wrote:
>
> > I *personally* don't think it's worth
> > changing all this without taking more care about backward compat than
> > we're apparently willing to do.  I'm ok with loosing that argument.  I
> > just don't think the previous concensus for a narrower change can be
> > used for the wider one
> > ​.
> >
>
> ​It would probably help others if you could brain dump a bit as to the
> benefit of the status-quo compared to both the 9.6 behavior and the all-in
> one.

Well, I think I've lost that argument, so I'm not sure how much effort
we should spent on litigating my opinion on it.  But here we go:

As I said somewhere upthread (when discussing the function names), I
think we're playing it too fast and loose with unnecessary changes.

We decided s/pg_xlog/pg_wal/ was necessary because people lost their
data, and we couldn't come up with a reasonable way to change it without
the name. The tradeoff is dataloss vs. dealing with directory renaming
in a few lowlevel tools.  So we decided to change the name.  It seems
breakage was unavoidable.

For me the renaming of functions, binaries, etc, is different because
the benefit is long-term avoidance of a bit of confusion, with the
shorter term price being tool breakages and confusion because
documentation/guides/... for different versions don't apply anymore, and
the search terms don't help anymore.  But we're still changing this,
even though breakage seems avoidable.


And that fits into the bigger topic of us doing minor cleanups without a
lot of concern for backward compatibility, e.g. like us whacking things
around in pg_stat_activity for most of the last releases - nearly all of
those could have been done in a compatible manner, without even smelling
that badly.

I hear these complaints about postgres most frequently: 1) replication
sucks. 2) way too slow on analytics queries. 3) existing admin tools
suck. 4) self written admin tools (required due to 3)) constantly break.

There's a lot being done on 1) and 2). There's very little in-core
progress about 3). We're getting worse on 4).


> I actually took your response as: "why the f**k is he talking about
> camels?" and started laughing...

That's good then, and let's raise a $beverage_of_choice tonight to the
fallability of e-mail conversation ;)


Andres


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

Re: [HACKERS] IndexBuild Function call fcinfo cannot access memory

[Jia Yu]

Dear Hackers,

I have solved this problem. I just want to post my answer here for people
who want to write their own backend index access method.

When we write a backend index access method (Let's call this method ABC),
we need to implement two interfaces: ABCbuild, ABCbuildcallback.

ABCbuild is the main index initialization function and it will make some
preparation and call (not directly call, ask Postgres to call)
ABCbuildcallback function per each parent table tuple. For a given parent
table tuple, ABCbuildcallback will generate the corresponding index entry
and insert it into the index main body.

When all parent table tuples have been processed by ABCbuildcallback,
Postgres will return the control back to ABCbuild.

As we can see, ABCbuild and ABCbuildcallback need to communicate with each
other to transfer some information. This information is stored in a data
structure called Struct ABCbuildstate. This ABCbuildstate is first
initialized in ABCbuild and then passed to ABCbuildcallback. NOTE THAT:
this Struct ABCbuildstate has to be initialized by palloc() in
ABCbuild *because
ABCbuild is not directly calling ABCbuildcallback*. My mistake is that I
initialize/declare this ABCbuildstate struct by using regular C like this
"ABCbuildstate my ABCbuildstate;" This will lead to segmentation fault sent
by OS. *However, this wrong declare method still works when we add some
unit SQL script in Postgres regression test (execute "make check"). We
still are able to build ABC index "create index ABC_idx on ... using ABC"
in Postgres regression test script. *But it will make us fail at normal
Postgres server and general debug methods, such as set breakpoint or print
debug information, cannot find this error because they even cannot reach
the ABCbuild function.

I believe there are some special settings in Postgres regression test
program "src/test/regress/pg_regress.c" but I haven't found it. *These
special settings will make Postgres regression test tolerates some memory
errors. *


Hope this will help you!

Thanks,
Jia Yu


On Mon, Jan 23, 2017 at 1:40 PM, Jia Yu  wrote:

> Dear hackers,
>
> Currently, I am developing a backend index access method for my research
> project.
>
> I built corresponding routines such as MyIndexbuild, MyIndexInsert, and so
> on and put them in "src/backend/access/hippo" (hippo is my index's name). I
> also added new entries in corresponding catalog files:
>
> src/include/catalog/pg_am.h
> src/include/catalog/pg_amop.h
> src/include/catalog/pg_amproc.h
> src/include/catalog/pg_opclass.h
> src/include/catalog/pg_opfamily.h
> src/include/catalog/pg_proc.h
>
> Now I can successfully build/query/update my new index by executing PG
> source code regression test (make check). Everything looks fine.
>
>
> However, these methods don't work in the normal PG server. It gave me
> "segmentation fault"
>
> I entered this SQL command in psql:
> create index hippo_idx on hippo_tbl using hippo(id2);
>
> Then got:
> Segmentation fault (core dumped)
>
> Here is my backtrace. It looks like I cannot access fcinfo. Can you help
> me about this? Or just some hints? I have been struggling with this problem
> for weeks.
>
> Thank you all in advance!
>
> 
> #0  0x004673d2 in hippobuild (
> fcinfo= 0x7fff3092c188>) at hippo.c:154
> #1  0x00956e64 in OidFunctionCall3Coll (functionId=5005,
> collation=0,
> arg1=140162511383536, arg2=140162511390256, arg3=19885792) at
> fmgr.c:1649
> #2  0x00555809 in index_build (heapRelation=0x7f7a20b3abf0,
> indexRelation=0x7f7a20b3c630, indexInfo=0x12f6ee0, isprimary=0 '\000',
> isreindex=0 '\000') at index.c:2025
> #3  0x00554551 in index_create (heapRelation=0x7f7a20b3abf0,
> indexRelationName=0x12f9378 "hippo_idx", indexRelationId=20668,
> relFileNode=0, indexInfo=0x12f6ee0, indexColNames=0x12f7370,
> accessMethodObjectId=5000, tableSpaceId=0,
> collationObjectId=0x12f7990,
> classObjectId=0x12f79b0, coloptions=0x12f79d0, reloptions=19895952,
> isprimary=0 '\000', isconstraint=0 '\000', deferrable=0 '\000',
> initdeferred=0 '\000', allow_system_table_mods=0 '\000',
> skip_build=0 '\000', concurrent=0 '\000', is_internal=0 '\000',
> if_not_exists=0 '\000') at index.c:1100
> #4  0x0062727b in DefineIndex (relationId=12476, stmt=0x12f6ff8,
> indexRelationId=0, is_alter_table=0 '\000', check_rights=1 '\001',
> skip_build=0 '\000', quiet=0 '\000') at indexcmds.c:606
> #5  0x008077a6 in ProcessUtilitySlow (parsetree=0x127c740,
> queryString=0x127ba48 "create index hippo_idx on hippo_tbl using
> hippo(id2) with (density=20);\n", context=PROCESS_UTILITY_TOPLEVEL,
> params=0x0,
> dest=0xe19a40 , completionTag=0x7fff7819aae0 "")
> at utility.c:1260
> #6  0x00806c30 in standard_ProcessUtility (parsetree=0x127c740,
> queryString=0x127ba48 "create index hippo_idx on hippo_tbl using
> hippo(id2) 

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[David G. Johnston]

On Thu, Jan 26, 2017 at 3:28 PM, Andres Freund  wrote:

> I *personally* don't think it's worth
> changing all this without taking more care about backward compat than
> we're apparently willing to do.  I'm ok with loosing that argument.  I
> just don't think the previous concensus for a narrower change can be
> used for the wider one
> ​.
>

​It would probably help others if you could brain dump a bit as to the
benefit of the status-quo compared to both the 9.6 behavior and the all-in
one.

Honestly, the reference to the camel sticking their nose in a tent was
supposed to be taken humorously, the serious explanation that it meant to
convey was already very well presented by Robert.  The only reason I added
the wiki link was to give those who would have no clue about the metaphor a
chance at understanding it.

I actually took your response as:  "why the f**k is he talking about
camels?" and started laughing...

Dave

Re: [HACKERS] pg_ls_dir & friends still have a hard-coded superuser check

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Wed, Jan 25, 2017 at 10:31 PM, Stephen Frost  wrote:
> > Frankly, I get quite tired of the argument essentially being made here
> > that because pg_ls_dir() wouldn't grant someone superuser rights, that
> > we should remove superuser checks from everything.  As long as you are
> > presenting it like that, I'm going to be quite dead-set against any of
> > it.
> 
> I'm not presenting it like that, and I think I've been quite clear
> about that.  I'm making two separate arguments which you keep
> conflating.  

The only function you mentioned in the last email was pg_ls_dir(), this
one looks better in that you're at least calling out the other changes
you wish to make explicitly with reasoning behind them.

> The first is that restricting the ability to GRANT access
> to a function, even a function that allows escalation to superuser
> privileges, doesn't improve security, because the superuser can still
> grant those privileges with more work.

Having various inconsistent and unclear ways to grant access to a
privileged user is making security worse and that is very much part of
my concern.  I don't agree, regardless of how many times you claim it,
that because the superuser could still grant superuser to someone (or
could grant the individual privileges with more work) keeps things
status-quo regarding security, or somehow that not making the changes
you are proposing reduces existing security.

I've certainly learned and come to accept that security, particularly
extremely privileged security, is much better when it's kept simple and
having multiple ways for someone to become a superuser, even if we admit
that there are and document the different ways, certainly isn't simpler
and doesn't make me feel like we're improving security.

> The second is that if we were to
> accept as official policy the idea that functions should have built-in
> superuser() checks when and only when they allow escalation to
> superuser, then those checks should be removed from those functions
> which do not allow such an escalation.  That argument favors removing
> at least some of the superuser checks as inconsistent with policy.  If
> there's a policy -- even one that I don't like -- it should be
> enforced consistently.

This I tend to agree with and I do believe there are may be additional
superuser() checks which could be removed.  In my initial work, I
focused on just the back-end, with some subsequent work on a particular
contrib module which I was asked to look at.  There are likely further
contrib modules which could also be changed, as long as those changes
are reviewed and done carefully, with consideration that we don't want
the admin to end up granting away superuser rights when they really only
intended to grant some specific subset of privileges.

> I audited the core backend for hard-coded superuser() checks that made
> it categorically impossible to call a certain SQL function.  I looked
> only for functions where the check was precisely if (!superuser())
> ereport(ERROR, ...), so things that were contingent on rolreplication
> or anything other than superuser-ness are not included in this
> analysis.  I found a total of five such functions, of which I can
> think of possible escalation-to-superuser risk for two.  I did not
> examine contrib although that might be worth doing at some point.

> 1. pg_ls_dir.  I cannot see how this can ever be used to get superuser
> privileges.

> 2. pg_stat_file.  I cannot see how this can ever be used to get
> superuser privileges.

I appreciate your efforts, but, for my part, I still don't agree with
removing the checks from functions which are, essentially, providing
filesystem-level access to non-superusers.  The right way to support
this would be to work through the issues that were already brought up
when Adam and I proposed similar capabilities on this very list,
complete with patches, three years ago:

https://www.postgresql.org/message-id/CAKRt6CRCQ1jmh3o8gkBBHxWqBJz4StnYUQjO0W8Kauru_SfPKA%40mail.gmail.com

As it relates specifically to *monitoring* tools, I continue to be the
opinion that we should provide better ways for those tools to get at
the information they need.

> 3. pg_read_binary_file.  This could potentially let you get superuser
> or other privileges.  Most obviously, if you read and parse the
> contents of pg_authid, you might be able to find passwords and then
> break into things.  There might be other methods as well.
> 
> 4. pg_read_file.  Essentially the same risks, although reading data
> files will be a little harder with this function because any NUL byte
> is going to make the read error out.  But given time and determination
> you can probably still ascertain the contents of every byte in the
> database, so the risks are about the same.

Yes, these are clearly functions which can, in most if not all cases, be
used to get superuser-level access, and I don't believe we 

Re: [HACKERS] safer node casting

[Tom Lane]

Andres Freund  writes:
> #if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
> is probably a better gatekeeper in the back-branches, than gcc?

Ah, yeah, that would work --- I'd already swapped out that business ;-)

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] safer node casting

[Andres Freund]

On 2017-01-26 16:55:33 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > How about something like the attached? The first patch just contains
> > castNode(), the second one a rebased version of Peter's changes (with
> > some long lines broken up).
> 
> Looks generally good.  A couple quibbles from a quick read-through:
> 
> * All but the first change in ProcessCopyOptions seem rather pointless:
> 
>   else if (defel->arg && IsA(defel->arg, List))
> - cstate->force_quote = (List *) defel->arg;
> + cstate->force_quote = castNode(List, 
> defel->arg);
> 
> In these places, castNode() isn't checking anything the if-condition
> didn't.  Maybe it's good style anyway, but I'm not really convinced.

Agreed that it's not not necessary - I didn't add this one (or any
castNode actually). But I don't think it matters much.


> * In ExecInitAgg:
> 
>   aggnode = list_nth(node->chain, phase - 1);
> - sortnode = (Sort *) aggnode->plan.lefttree;
> - Assert(IsA(sortnode, Sort));
> + sortnode = castNode(Sort, aggnode->plan.lefttree);
> 
> it seems like the assignment to aggnode ought to have a castNode on it
> too

Yea, looks good.


> (the fact that it lacks any cast now is sloppy and not per project style,
> IMO).

There's a lot of these missing :(.  This is one of these things that'd
be a lot easier to enforce if we'd be able to compile in a c++
compatible mode (-Wc++-compat), because there void * to X * casts
have to be done explicitly.


> BTW, maybe it's just the first flush of enthusiasm, but I can see us
> using this so much that the lack of it in back branches will become
> a serious PITA for back-patching.

Might, yea.


> So I'm strongly tempted to propose
> that your 0001 should be back-patched.  However, before 9.6 we didn't
> have the compiler requirement that "static inline" in headers must be
> handled sanely.  Maybe a useful compromise would be to put 0001 in 9.6,
> and before that just add
> 
> #define castNode(_type_,nodeptr)  ((_type_ *)(nodeptr))
> 
> which would allow the notation to be used safely, albeit without
> any assertion backup.  Alternatively, we could enable the assertion
> code only for gcc, which would probably be plenty good enough for
> finding bugs in stable branches.

#if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
is probably a better gatekeeper in the back-branches, than gcc? Then we
can just remove the defined(PG_USE_INLINE) and it's associated comment
in >= 9.6.

Regards,

Andres


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On 2017-01-26 16:55:37 -0500, Robert Haas wrote:
> On Thu, Jan 26, 2017 at 2:37 PM, Andres Freund  wrote:
> > On 2017-01-26 14:28:01 -0500, Robert Haas wrote:
> >> On Thu, Jan 26, 2017 at 2:24 PM, Andres Freund  wrote:
> >> >> Whether the voters recognized that fact at the time I would have to 
> >> >> concur
> >> >> that if we are going to change from xlog to wal we should be all-in.  If
> >> >> you want to vote to reject putting the whole camel in the tent I would 
> >> >> say
> >> >> its a vote for reverting the change that put the camel's nose in there 
> >> >> in
> >> >> the first place.
> >> >
> >> > WTF.
> >>
> >> I think that response is unwarranted.  I happen to agree entirely with
> >> his position.
> >
> > I don't. Considering intent imo is important. David (and you?) is
> > basically saying "screw it, you voted for that person, you aren't
> > allowed to have an opinion anymore", and that's way outside of what I
> > consider acceptable.  So, because you think it doesn't make sense to
> > view renaming pg_xlog vs pg_wal as separate from a global s/xlog/wal/,
> > nobody else can have that position.  And on top of that David's
> > underlying that argument with a metaphor that basically implies the
> > other party is getting screwed over.  Sorry, that's not the way I want
> > decisions to be made here.
>
> I'm not saying that people aren't allowed to have an opinion any more.
> I'm saying that when somebody has an opinion that is different than
> yours, you should politely disagree with it rather than saying "WTF",
> which just as a reminder expands to "What The Fuck".  Frankly, I think
> WTF is generally not a particularly useful contribution to most
> discussions, but at the very least I think it should be used with some
> kind of context.  Sending an email that says "WTF" and nothing else
> conveys nothing other than that you don't respect the author of the
> email to which you are replying, and David's response was not so
> outlandish as to deserve that.  You might as well send an email that
> says "go dire in a fire".

The WTF wasn't about David wanting to go all in or not, but the way he
framed the general discussion.  And I do find it outlandish enough to
deserve that.  To me, especially with that methaphor and link, it still
reads as "you voted for it, even if phrased a lot more narrowly, so you
now get to eat all of it". *Especially* as it's a reply to me saying
that I'm concerned about my tepid yes to s/pg_xlog/pg_wal/ being used
for a larger change.

Using the normal ~concensus type of process for a mildly controversial
breaking change is fine with me.  I *personally* don't think it's worth
changing all this without taking more care about backward compat than
we're apparently willing to do.  I'm ok with loosing that argument.  I
just don't think the previous concensus for a narrower change can be
used for the wider one.

Andres


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[David Steele]

On 1/26/17 5:07 PM, Stephen Frost wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> And I think that's all pretty reasonable.  I don't consider this a
>> done deal yet.  I don't consider your -1 irrelevant.  But I don't
>> think it's fair to present this as if I am somehow running roughshod
>> over community process, either.  If a large crew of people show up to
>> insist that we should rename only the directories and nothing else, I
>> guess I'll have to live with that.  But I think that's a bad decision
>> that will never survive the passage of time, and there seem to be
>> several people who agree with me.
> 
> +1 (to the above, and the rest of Robert's comments on that email).
> 
> For my 2c, I'm also in the crowd of forward or back, but don't leave it
> half-done.  My preference is certainly to move forward with these
> changes, but I understand the process and if there's enough agreement
> that we just shouldn't make these changes, then we really shouldn't make
> any of them.

+1 for forward or back.  I prefer forward but would take back over the
state we're in now.

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> And I think that's all pretty reasonable.  I don't consider this a
> done deal yet.  I don't consider your -1 irrelevant.  But I don't
> think it's fair to present this as if I am somehow running roughshod
> over community process, either.  If a large crew of people show up to
> insist that we should rename only the directories and nothing else, I
> guess I'll have to live with that.  But I think that's a bad decision
> that will never survive the passage of time, and there seem to be
> several people who agree with me.

+1 (to the above, and the rest of Robert's comments on that email).

For my 2c, I'm also in the crowd of forward or back, but don't leave it
half-done.  My preference is certainly to move forward with these
changes, but I understand the process and if there's enough agreement
that we just shouldn't make these changes, then we really shouldn't make
any of them.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] safer node casting

[Tom Lane]

Andres Freund  writes:
> How about something like the attached? The first patch just contains
> castNode(), the second one a rebased version of Peter's changes (with
> some long lines broken up).

Looks generally good.  A couple quibbles from a quick read-through:

* All but the first change in ProcessCopyOptions seem rather pointless:

else if (defel->arg && IsA(defel->arg, List))
-   cstate->force_quote = (List *) defel->arg;
+   cstate->force_quote = castNode(List, 
defel->arg);

In these places, castNode() isn't checking anything the if-condition
didn't.  Maybe it's good style anyway, but I'm not really convinced.

* In ExecInitAgg:

aggnode = list_nth(node->chain, phase - 1);
-   sortnode = (Sort *) aggnode->plan.lefttree;
-   Assert(IsA(sortnode, Sort));
+   sortnode = castNode(Sort, aggnode->plan.lefttree);

it seems like the assignment to aggnode ought to have a castNode on it too
(the fact that it lacks any cast now is sloppy and not per project style,
IMO).

There were a bunch of places in ab1f0c822 where I wished I had this,
but I can go back and back-fill that later; doesn't need to be in the
first commit.

BTW, maybe it's just the first flush of enthusiasm, but I can see us
using this so much that the lack of it in back branches will become
a serious PITA for back-patching.  So I'm strongly tempted to propose
that your 0001 should be back-patched.  However, before 9.6 we didn't
have the compiler requirement that "static inline" in headers must be
handled sanely.  Maybe a useful compromise would be to put 0001 in 9.6,
and before that just add

#define castNode(_type_,nodeptr)  ((_type_ *)(nodeptr))

which would allow the notation to be used safely, albeit without
any assertion backup.  Alternatively, we could enable the assertion
code only for gcc, which would probably be plenty good enough for
finding bugs in stable branches.

regards, tom lane


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Robert Haas]

On Thu, Jan 26, 2017 at 2:37 PM, Andres Freund  wrote:
> On 2017-01-26 14:28:01 -0500, Robert Haas wrote:
>> On Thu, Jan 26, 2017 at 2:24 PM, Andres Freund  wrote:
>> >> Whether the voters recognized that fact at the time I would have to concur
>> >> that if we are going to change from xlog to wal we should be all-in.  If
>> >> you want to vote to reject putting the whole camel in the tent I would say
>> >> its a vote for reverting the change that put the camel's nose in there in
>> >> the first place.
>> >
>> > WTF.
>>
>> I think that response is unwarranted.  I happen to agree entirely with
>> his position.
>
> I don't. Considering intent imo is important. David (and you?) is
> basically saying "screw it, you voted for that person, you aren't
> allowed to have an opinion anymore", and that's way outside of what I
> consider acceptable.  So, because you think it doesn't make sense to
> view renaming pg_xlog vs pg_wal as separate from a global s/xlog/wal/,
> nobody else can have that position.  And on top of that David's
> underlying that argument with a metaphor that basically implies the
> other party is getting screwed over.  Sorry, that's not the way I want
> decisions to be made here.

I'm not saying that people aren't allowed to have an opinion any more.
I'm saying that when somebody has an opinion that is different than
yours, you should politely disagree with it rather than saying "WTF",
which just as a reminder expands to "What The Fuck".  Frankly, I think
WTF is generally not a particularly useful contribution to most
discussions, but at the very least I think it should be used with some
kind of context.  Sending an email that says "WTF" and nothing else
conveys nothing other than that you don't respect the author of the
email to which you are replying, and David's response was not so
outlandish as to deserve that.  You might as well send an email that
says "go dire in a fire".

The substantive issue here is whether we should go forward with this
change, back out the change we already did, or leave things as they
are.  Tom, David, and I seem to be in lock step on at least the
following conclusion: halfway in between is bad.  So I have every
intention of continuing to push very hard for us to go either forward
or backward.  I hope to do that politely and respectfully, but I am
not prepared to give up on that basic point unless there are a WHOLE
LOT of contrary votes, and that is just not where we are at present.
While a variety of opinions have been expressed on the patch
originally posted and while many of those people took subtly different
positions which I'm unable to summarize concisely, there was ZERO
pushback against my email volunteering to go make all of this
consistent and pushing for it to be made consistent until you showed
up to complain.  Now there is one person objecting and several people
in favor, and I think it is pretty fair to say that many of the people
who were arguing for Vladimir's original patch are still in favor of
proceeding.  Even Peter, who wasn't super-excited about Vladimir's
patch in isolation, conceded that the xlog terminology sucked and
asked for a complete patch set.  I don't think he's unequivocally in
favor of this, but he seemed to be willing to sit still for it.  After
waiting two days for further opinions, I went and wrote the patches as
promised.

And I think that's all pretty reasonable.  I don't consider this a
done deal yet.  I don't consider your -1 irrelevant.  But I don't
think it's fair to present this as if I am somehow running roughshod
over community process, either.  If a large crew of people show up to
insist that we should rename only the directories and nothing else, I
guess I'll have to live with that.  But I think that's a bad decision
that will never survive the passage of time, and there seem to be
several people who agree with me.

-- 
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] Failure in commit_ts tap tests

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > It is really quite annoying that the buildfarm doesn't do what stock
> > tests do.  What about pushing a bit stronger for having these
> > optimizations as part of the standard build run, instead of being only
> > in the buildfarm client script?
> 
> Huh?  It's just a question of "make check" vs "make installcheck".
> They each have their uses.

The current problem is because the buildfarm uses a different database.
See https://postgr.es/m/e6084b89-07a7-7e57-51ee-d7b8fc9ec...@2ndquadrant.com
(When I wrote the para quoted above, I thought this was for performance,
but now I realize it's not so.  Anyway the point stands that buildfarm
does things differently.)

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


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

Re: [HACKERS] Failure in commit_ts tap tests

[Tom Lane]

Alvaro Herrera  writes:
> It is really quite annoying that the buildfarm doesn't do what stock
> tests do.  What about pushing a bit stronger for having these
> optimizations as part of the standard build run, instead of being only
> in the buildfarm client script?

Huh?  It's just a question of "make check" vs "make installcheck".
They each have their uses.

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] Performance improvement for joins where outer side is unique

[David Rowley]

On 27 January 2017 at 08:34, Tom Lane  wrote:
> David Rowley  writes:
>> I've attached a version without outer unique.
>
> I looked through this a bit, and the first thing I noticed was it doesn't
> touch costsize.c at all.  That seems pretty wrong; it's little help to
> have a performance improvement if the planner won't pick the right plan
> type.  There were costsize.c changes in there back in April; what happened
> to them?

hmm. I must've taken them out when I changed everything around to use
the join types. Nothing special was needed when changing JOIN_INNER to
JOIN_SEMI.

I must've forgotten to put them back again... I'll do that now.

-- 
 David Rowley   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] safer node casting

[Andres Freund]

On 2017-01-25 19:21:40 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> >> RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
>
> > Are you planning to add this / update this patch? Because I really would
> > have liked this a number of times already...  I can update it according
> > to my suggestions (to avoid multiple eval scenarios) if helpful
>
> Yeah, I'd like that in sooner rather than later, too.  But we do need
> it to be foolproof - no multiple evals.  The first draft had
> inadequate-parenthesization hazards,

How about something like the attached? The first patch just contains
castNode(), the second one a rebased version of Peter's changes (with
some long lines broken up).

Andres
>From a2a2ef62ecb8a349eae0cd90df75c44f5e7e83a5 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 26 Jan 2017 13:13:18 -0800
Subject: [PATCH 1/2] Add castNode(type, ptr) for safe casting between NodeTag
 based types.

Author: Peter Eisentraut and Andres Freund
Discussion: https://postgr.es/m/c5d387d9-3440-f5e0-f9d4-71d53b9fb...@2ndquadrant.com
---
 src/include/nodes/nodes.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index fa4932a902..b462a5ffde 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -558,6 +558,26 @@ extern PGDLLIMPORT Node *newNodeMacroHolder;
 
 #define IsA(nodeptr,_type_)		(nodeTag(nodeptr) == T_##_type_)
 
+/*
+ * castNode(type, ptr) casts ptr to type and, if cassert is enabled, verifies
+ * that the the c actually has the appropriate type (using it's nodeTag()).
+ *
+ * Use an inline function when assertions are enabled, to avoid multiple
+ * evaluations of the ptr argument (which could e.g. be a function call).
+ */
+#ifdef USE_ASSERT_CHECKING
+static inline Node*
+castNodeImpl(enum NodeTag type, void *ptr)
+{
+	Assert(ptr == NULL || nodeTag(ptr) == type);
+	return ptr;
+}
+#define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(T_##_type_, nodeptr))
+#else
+#define castNode(_type_,nodeptr)  ((_type_ *)(nodeptr))
+#endif
+
+
 /* 
  *	  extern declarations follow
  * 
-- 
2.11.0.22.g8d7a455.dirty

>From 732c0bfd2cbe6cb2e7b9730d5802be115713a350 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 26 Jan 2017 13:12:10 -0800
Subject: [PATCH 2/2] Use the new castNode() macro in a subset of places.

Author: Peter Eisentraut, with some minor changes by me
Discussion: https://postgr.es/m/c5d387d9-3440-f5e0-f9d4-71d53b9fb...@2ndquadrant.com
---
 contrib/pg_stat_statements/pg_stat_statements.c | 10 --
 contrib/postgres_fdw/deparse.c  |  5 +
 contrib/postgres_fdw/postgres_fdw.c | 12 
 src/backend/catalog/pg_proc.c   |  3 +--
 src/backend/commands/aggregatecmds.c|  4 ++--
 src/backend/commands/analyze.c  |  6 +++---
 src/backend/commands/async.c|  8 
 src/backend/commands/collationcmds.c|  2 +-
 src/backend/commands/constraint.c   |  2 +-
 src/backend/commands/copy.c | 14 +++---
 src/backend/commands/createas.c |  5 ++---
 src/backend/commands/dropcmds.c |  4 +---
 src/backend/commands/explain.c  | 16 +++-
 src/backend/commands/functioncmds.c |  7 ++-
 src/backend/commands/matview.c  |  3 +--
 src/backend/commands/opclasscmds.c  |  9 +++--
 src/backend/commands/tablecmds.c| 16 +---
 src/backend/commands/trigger.c  |  4 +---
 src/backend/commands/user.c |  4 +---
 src/backend/commands/view.c |  3 +--
 src/backend/executor/execAmi.c  |  7 ---
 src/backend/executor/execQual.c |  6 ++
 src/backend/executor/execTuples.c   |  5 +
 src/backend/executor/functions.c|  6 ++
 src/backend/executor/nodeAgg.c  |  6 ++
 src/backend/executor/nodeCtescan.c  |  3 +--
 src/backend/executor/nodeCustom.c   |  4 ++--
 src/backend/executor/nodeHashjoin.c |  7 ++-
 src/backend/executor/nodeLockRows.c |  4 +---
 src/backend/executor/nodeModifyTable.c  |  4 +---
 src/backend/executor/nodeSubplan.c  | 15 +--
 src/backend/executor/nodeWorktablescan.c|  4 ++--
 32 files changed, 77 insertions(+), 131 deletions(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6edc3d926b..a65b52968a 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ 

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Thu, Jan 26, 2017 at 3:55 PM, Fabien COELHO  wrote:

>
> Hello Daniel,
>
> A comment about control flow and variables: in branches that are not
>> taken, variables are expanded nonetheless, in a way that can be surprising.
>> Case in point:
>>
>> \set var 'ab''cd'
>> -- select :var;
>> \if false
>>  select :var ;
>> \else
>>  select 1;
>> \endif
>>
>> To avoid that kind of trouble, would it make sense not to expand
>> variables in untaken branches?
>>
>
> Hmmm. This case is somehow contrived (for a string, :'var' could be used,
> in which case escaping would avoid the hazard), but I think that what you
> suggest is a better behavior, if easy to implement.
>
> --
> Fabien.
>

Good question, Daniel. Variable expansion seems to be done via
psql_get_variable which is invoked via callback, which means that I might
have to move branch_block_active into PsqlSettings. That's slightly
different because the existing boolean is scoped with MainLoop(), but
there's no way to get to a new MainLoop unless you're in an executing
branch, and no way to legally exit a MainLoop with an unbalanced if-endif
state. In short, I think it's better behavior. It does prevent someone from
setting a variable to '\endif' and expecting that to work, like this:

select case
 when random() < 0.5 then '\endif'
 else E'\else\n\echo fooled you\n\endif'
end as contrived_metaprogramming
\gset

\if false
  :contrived_metaprogramming


I'm sure someone will argue that preventing that is a good thing. Unless
someone sees a good reason not to move that PsqlSettings, I'll make that
change.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Daniel,

A comment about control flow and variables: in branches that are not 
taken, variables are expanded nonetheless, in a way that can be 
surprising. Case in point:


\set var 'ab''cd'
-- select :var;
\if false
 select :var ;
\else
 select 1;
\endif

To avoid that kind of trouble, would it make sense not to expand
variables in untaken branches?


Hmmm. This case is somehow contrived (for a string, :'var' could be used, 
in which case escaping would avoid the hazard), but I think that what you 
suggest is a better behavior, if easy to implement.


--
Fabien.


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

Re: [HACKERS] Failure in commit_ts tap tests

[Alvaro Herrera]

Tom Lane wrote:
> Andrew Dunstan  writes:
> > On 01/24/2017 05:17 AM, Alvaro Herrera wrote:
> >> Maybe we can drop that line and put it back once we get COMMENT ON
> >> CURRENT_DATABASE.
> 
> > Works for me.
> 
> If that's enough to get the "make check" cases passing in the buildfarm,
> then +1.

Okay, done.

It is really quite annoying that the buildfarm doesn't do what stock
tests do.  What about pushing a bit stronger for having these
optimizations as part of the standard build run, instead of being only
in the buildfarm client script?

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


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

Re: [HACKERS] Allow interrupts on waiting standby

[Alvaro Herrera]

Simon Riggs wrote:
> On 26 January 2017 at 19:20, Andres Freund  wrote:

> > I'm personally fine with going with a CHECK_FOR_INTERRUPTS
> > for now, but I think it'd better to replace it with a latch.
> 
> I have waited, so not sure what you mean. Tomorrow is too late.
> 
> Replacing with a latch wouldn't be backpatchable, IMHO.

Hmm, you pushed this to the master branch as commit e8ee3d6b859a, but I
see no backpatch.

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


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

Re: [HACKERS] simplify sequence test

[Peter Eisentraut]

On 1/25/17 5:29 AM, Petr Jelinek wrote:
> On 25/01/17 03:48, Peter Eisentraut wrote:
>> We maintain a separate test output file sequence_1.out because the
>> log_cnt value can vary if there is a checkpoint happening at the right
>> time.  So we have to maintain two files because of a one character
>> difference.  I propose the attached patch to restructure the test a bit
>> to avoid this, without loss of test coverage.
>>
> 
> +1, looks good.

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: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Daniel Verite]

Corey Huinker wrote:

> Revised patch

A comment about control flow and variables:
in branches that are not taken, variables are expanded 
nonetheless, in a way that can be surprising.
Case in point:

\set var 'ab''cd'
-- select :var; 
\if false
  select :var ;
\else
  select 1;
\endif

The 2nd reference to :var has a quoting hazard, but since it's within
an "\if false" branch, at a glance it seems like this script might work.
In fact it barfs with:
  psql:script.sql:0: found EOF before closing \endif(s)

AFAICS what happens is that :var gets injected and starts a
runaway string, so that as far as the parser is concerned
the \else ..\endif block slips into the untaken branch, as a part of
that unfinished string.

This contrasts with line 2: -- select :var
where the reference to :var is inoffensive.

To avoid that kind of trouble, would it make sense not to expand
variables in untaken branches?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Joshua D. Drake]

-Hackers,

From the field. I do not care what you chose, I care that:

1. It is consistent
2. It is readable/understandable
3. It is documented
4. It is done wholesale (because of usability)

That's it. So whatever meets that criteria, let's go for it. That may 
mean that certain commands look a little off but with the goal of 
consistency that is o.k.. It becomes explainable.


JD

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[David G. Johnston]

On Thu, Jan 26, 2017 at 12:37 PM, Andres Freund  wrote:

> On 2017-01-26 14:28:01 -0500, Robert Haas wrote:
> > On Thu, Jan 26, 2017 at 2:24 PM, Andres Freund 
> wrote:
> > >> Whether the voters recognized that fact at the time I would have to
> concur
> > >> that if we are going to change from xlog to wal we should be all-in.
> If
> > >> you want to vote to reject putting the whole camel in the tent I
> would say
> > >> its a vote for reverting the change that put the camel's nose in
> there in
> > >> the first place.
> > >
> > > WTF.
> >
> > I think that response is unwarranted.  I happen to agree entirely with
> > his position.
>
> I don't. Considering intent imo is important. David (and you?) is
> basically saying "screw it, you voted for that person, you aren't
> allowed to have an opinion anymore", and that's way outside of what I
> consider acceptable.  So, because you think it doesn't make sense to
> view renaming pg_xlog vs pg_wal as separate from a global s/xlog/wal/,
> nobody else can have that position.  And on top of that David's
> underlying that argument with a metaphor that basically implies the
> other party is getting screwed over.  Sorry, that's not the way I want
> decisions to be made here.
>

We can think or assume all we want about what people knew or did not know
when the prior consensus to commit the directory change was reached.  At
this point it doesn't matter.  The limited point I was trying to make is
that "the small change (i.e., the camel's nose) has been made (i.e., is now
in the tent)".  Yet here we are talking about the rest of the  camel.  We
have three options: go all-in, leave the status-quo, undo the previous
action.

The past decision is immaterial and any knowledge/experience/opinions that
it may have entailed can now, on an individual basis, be neatly wrapped up
into a vote on one of those options.  No need to make assumptions about
how/why consensus on the previous decision was reached.  And, to be fair,
it is not an unreasonable assumption for Robert to make that having voted
for the small action that most of the well-informed persons on this list
were aware and implicitly understood that the underlying intent was not the
directory but rather removing "xlog" from our vocabulary and replacing it
with the more accurate "wal".  In that vein assumed consensus makes sense.

I didn't give as much credence to the "status quo" option in my original
response as I probably should have - because, frankly, I find it
unappealing.  I do accept that it is a valid position to hold, though.

David J.

Re: [HACKERS] Allow interrupts on waiting standby

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2017-01-26 19:36:11 +, Simon Riggs wrote:
> > Tomorrow is too late.
> 
> Huh? We're not wrapping today/tomorrow, are we?  If I missed something
> and we are, then sure, it makes sense to push ahead.

I haven't seen anyone suggest that we're changing from the regular
release cycle, which would mean that we wrap on Monday, Feb 6th, for
release on Feb 9th.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Alvaro Herrera]

Robert Haas wrote:
> On Wed, Jan 25, 2017 at 4:08 PM, Alvaro Herrera
>  wrote:
> > I think the way WARM works has been pretty well hammered by now, other
> > than the CREATE INDEX CONCURRENTLY issues, so I'm looking at the code
> > from a maintainability point of view only.
> 
> Which senior hackers have previously reviewed it in detail?

The previous thread,
https://postgr.es/m/caboikdmop5rb_rns2xfdaxmzgsqcj-p-by2rumd+buukj4i...@mail.gmail.com
contains some discussion of it, which uncovered bugs in the initial idea
and gave rise to the current design.

> Where would I go to get a good overview of the overall theory of operation?

The added README file does a pretty good job, I thought.

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


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 26, 2017 at 2:24 PM, Andres Freund  wrote:
>>> Whether the voters recognized that fact at the time I would have to concur
>>> that if we are going to change from xlog to wal we should be all-in.  If
>>> you want to vote to reject putting the whole camel in the tent I would say
>>> its a vote for reverting the change that put the camel's nose in there in
>>> the first place.

>> WTF.

> I think that response is unwarranted.  I happen to agree entirely with
> his position.

Me three.  If we're changing this at all, we should try to eliminate
"xlog" from user-visible names and docs altogether.  I was okay with the
proposal to have some redundant names in place for awhile, and I'm equally
okay with not having any; but not with being permanently schizophrenic
on the point.

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] Allow interrupts on waiting standby

[Andres Freund]

On 2017-01-26 19:36:11 +, Simon Riggs wrote:
> On 26 January 2017 at 19:20, Andres Freund  wrote:
> > On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
> >> On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
> >> > Currently a waiting standby doesn't allow interrupts.
> >> >
> >> > Patch implements that.
> >> >
> >> > Barring objection, patching today with backpatches.
> >>
> >> "today" is a little quick, but the patch looks fine.  I doubt anyone's
> >> going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.
> >
> > I don't quite get asking for agreement, and then not waiting as
> > suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
> > for now, but I think it'd better to replace it with a latch.
> 
> I have waited, so not sure what you mean.

Well, Robert today said >> "today" is a little quick <<.


> Tomorrow is too late.

Huh? We're not wrapping today/tomorrow, are we?  If I missed something
and we are, then sure, it makes sense to push ahead.


> Replacing with a latch wouldn't be backpatchable, IMHO.

Hm, don't quite see why - isn't it just like three lines?


Andres


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On 2017-01-26 14:28:01 -0500, Robert Haas wrote:
> On Thu, Jan 26, 2017 at 2:24 PM, Andres Freund  wrote:
> >> Whether the voters recognized that fact at the time I would have to concur
> >> that if we are going to change from xlog to wal we should be all-in.  If
> >> you want to vote to reject putting the whole camel in the tent I would say
> >> its a vote for reverting the change that put the camel's nose in there in
> >> the first place.
> >
> > WTF.
> 
> I think that response is unwarranted.  I happen to agree entirely with
> his position.

I don't. Considering intent imo is important. David (and you?) is
basically saying "screw it, you voted for that person, you aren't
allowed to have an opinion anymore", and that's way outside of what I
consider acceptable.  So, because you think it doesn't make sense to
view renaming pg_xlog vs pg_wal as separate from a global s/xlog/wal/,
nobody else can have that position.  And on top of that David's
underlying that argument with a metaphor that basically implies the
other party is getting screwed over.  Sorry, that's not the way I want
decisions to be made here.

Andres


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

Re: [HACKERS] Allow interrupts on waiting standby

[Simon Riggs]

On 26 January 2017 at 19:20, Andres Freund  wrote:
> On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
>> On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
>> > Currently a waiting standby doesn't allow interrupts.
>> >
>> > Patch implements that.
>> >
>> > Barring objection, patching today with backpatches.
>>
>> "today" is a little quick, but the patch looks fine.  I doubt anyone's
>> going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.
>
> I don't quite get asking for agreement, and then not waiting as
> suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
> for now, but I think it'd better to replace it with a latch.

I have waited, so not sure what you mean. Tomorrow is too late.

Replacing with a latch wouldn't be backpatchable, IMHO.

I've no problem if you want to work on a deeper fix for future versions.

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


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

Re: [HACKERS] Performance improvement for joins where outer side is unique

[Tom Lane]

David Rowley  writes:
> I've attached a version without outer unique.

I looked through this a bit, and the first thing I noticed was it doesn't
touch costsize.c at all.  That seems pretty wrong; it's little help to
have a performance improvement if the planner won't pick the right plan
type.  There were costsize.c changes in there back in April; what happened
to them?

regards, tom lane


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Robert Haas]

On Thu, Jan 26, 2017 at 2:24 PM, Andres Freund  wrote:
>> Whether the voters recognized that fact at the time I would have to concur
>> that if we are going to change from xlog to wal we should be all-in.  If
>> you want to vote to reject putting the whole camel in the tent I would say
>> its a vote for reverting the change that put the camel's nose in there in
>> the first place.
>
> WTF.

I think that response is unwarranted.  I happen to agree entirely with
his position.

-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Robert Haas]

On Thu, Jan 26, 2017 at 2:13 PM, Andres Freund  wrote:
> On 2017-01-26 14:05:43 -0500, Robert Haas wrote:
>> I completely understand that position.  I have always been doubtful of
>> the value of renaming pg_xlog to pg_wal, and I'm not any more
>> dedicated to the idea now than I was when I committed that patch.  But
>> there was overwhelming support for it, consensus on a level rarely
>> seen here.
>
> I think that consistency was based on the change being a narrow
> proposition, not a license to run around and change a lot of stuff
> including the names of binary.

I'm not so sure about that.  There are a lot of people who have
supported the idea of making this consistent on THIS thread.  It's not
clear how much of a majority there is, but it's certainly no worse
than 50-50.  It's got far more affirmative votes than most patches
that go in though, I will grant, also far more negative votes.

>> I do not think it can be right to rename the directory and not
>> anything else.  I stand by what I wrote in
>>
>> https://www.postgresql.org/message-id/ca+tgmobehp2qbtmvyxg2x8pm_9utjrya-rom5xl4quya26c...@mail.gmail.com
>
> I'm tempted to quote Emerson ;).  I don't think the naming of pg_xlog
> vs. pg_wal doesn't actually have that large an impact, to change the
> dynamics of the wal vs xlog dichotomy.  Sure it's nothing you'd do in a
> new program, but neither is it very bad.

Gee, I can't imagine what Emerson quote you might be thinking about.  :-)

I think it's just never going to work to imagine that we can
indefinitely go on having a pg_resetxlog binary to reset a thing that
is no longer called xlog.  Sure, for a few years that will seem like
it makes sense, but if we didn't change it now, eventually there would
be a push to do it later.  And if that one gets shut down, there will
eventually be another push.  We'll repeatedly relitigate this whole
debate, and maybe eventually the result will be one or two more things
get changed ... and then later we'll do it again for what's left.  I
am not entirely excited about the backward-compatibility pain we're
incurring here, but I think if we don't do it all at once it's just
going to get spread out over time.  Maybe it in a universe where
PostgreSQL was controlled by a small number of people acting as one
you could hold the line, but in the actual universe people just keep
calling a vote on this sort of thing every two or three until they
win.  I don't think there's anything you or I or anyone else can say
or do that will prevent that from happening, so I'd as soon just be
done with 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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On 2017-01-26 12:23:24 -0700, David G. Johnston wrote:
> On Thu, Jan 26, 2017 at 12:13 PM, Andres Freund  wrote:
> 
> > On 2017-01-26 14:05:43 -0500, Robert Haas wrote:
> > > I completely understand that position.  I have always been doubtful of
> > > the value of renaming pg_xlog to pg_wal, and I'm not any more
> > > dedicated to the idea now than I was when I committed that patch.  But
> > > there was overwhelming support for it, consensus on a level rarely
> > > seen here.
> >
> > I think that consistency was based on the change being a narrow
> > proposition, not a license to run around and change a lot of stuff
> > including the names of binary.
> >
> >
> ​Whether the voters recognized that fact at the time I would have to concur
> that if we are going to change from xlog to wal we​ should be all-in.  If
> you want to vote to reject putting the whole camel in the tent I would say
> its a vote for reverting the change that put the camel's nose in there in
> the first place.

WTF.


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[David G. Johnston]

On Thu, Jan 26, 2017 at 12:13 PM, Andres Freund  wrote:

> On 2017-01-26 14:05:43 -0500, Robert Haas wrote:
> > I completely understand that position.  I have always been doubtful of
> > the value of renaming pg_xlog to pg_wal, and I'm not any more
> > dedicated to the idea now than I was when I committed that patch.  But
> > there was overwhelming support for it, consensus on a level rarely
> > seen here.
>
> I think that consistency was based on the change being a narrow
> proposition, not a license to run around and change a lot of stuff
> including the names of binary.
>
>
​Whether the voters recognized that fact at the time I would have to concur
that if we are going to change from xlog to wal we​ should be all-in.  If
you want to vote to reject putting the whole camel in the tent I would say
its a vote for reverting the change that put the camel's nose in there in
the first place.

https://en.wikipedia.org/wiki/Camel's_nose


> > I do not think it can be right to rename the directory and not
> > anything else.  I stand by what I wrote in
> >
> > https://www.postgresql.org/message-id/CA+TgmobeHP2qbtMvYxG2x8Pm_
> 9utjrya-rom5xl4quya26c...@mail.gmail.com
>
> I'm tempted to quote Emerson ;).  I don't think the naming of pg_xlog
> vs. pg_wal doesn't actually have that large an impact, to change the
> dynamics of the wal vs xlog dichotomy.  Sure it's nothing you'd do in a
> new program, but neither is it very bad.
>

​Once I learned what "write ahead log" was it wasn't that big a deal to
understand that this particular historical implementation detail ​means I
have to associate xlog with it.  Causing wide-spread pain to lower the
comprehension bar doesn't seems like a simple win here.  I have no real
feel for how wide-spread that would be, though.  I personally wouldn't mind
it being consistent but I am not representative of the larger user base.

David J.

Re: [HACKERS] Allow interrupts on waiting standby

[Andres Freund]

On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
> On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
> > Currently a waiting standby doesn't allow interrupts.
> >
> > Patch implements that.
> >
> > Barring objection, patching today with backpatches.
> 
> "today" is a little quick, but the patch looks fine.  I doubt anyone's
> going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.

I don't quite get asking for agreement, and then not waiting as
suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
for now, but I think it'd better to replace it with a latch.

Andres


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

Re: [HACKERS] increasing the default WAL segment size

[Andres Freund]

Hi,

On 2017-01-25 12:26:21 +0900, Michael Paquier wrote:
> diff --git a/src/backend/access/common/tupdesc.c 
> b/src/backend/access/common/tupdesc.c
> index 083c0303dc..2eb3a420ac 100644
> --- a/src/backend/access/common/tupdesc.c
> +++ b/src/backend/access/common/tupdesc.c
> @@ -629,6 +629,14 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
>   att->attstorage = 'p';
>   att->attcollation = InvalidOid;
>   break;
> +
> + case INT8OID:
> + att->attlen = 8;
> + att->attbyval = true;
> + att->attalign = 'd';
> + att->attstorage = 'p';
> + att->attcollation = InvalidOid;
> + break;
>   }
>  }

INT8 isn't unconditionally byval, is it?

>   /* slot_name */
> - len = strlen(NameStr(MyReplicationSlot->data.name));
> - pq_sendint(, len, 4);   /* col1 len */
> - pq_sendbytes(, NameStr(MyReplicationSlot->data.name), len);
> + values[0] = 
> PointerGetDatum(cstring_to_text(NameStr(MyReplicationSlot->data.name)));

That seems a bit long.


I've not done like the most careful review ever, but I'm in favor of the
general change (provided the byval thing is fixed obviously).

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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On 2017-01-26 14:05:43 -0500, Robert Haas wrote:
> I completely understand that position.  I have always been doubtful of
> the value of renaming pg_xlog to pg_wal, and I'm not any more
> dedicated to the idea now than I was when I committed that patch.  But
> there was overwhelming support for it, consensus on a level rarely
> seen here.

I think that consistency was based on the change being a narrow
proposition, not a license to run around and change a lot of stuff
including the names of binary.


> I do not think it can be right to rename the directory and not
> anything else.  I stand by what I wrote in
> 
> https://www.postgresql.org/message-id/ca+tgmobehp2qbtmvyxg2x8pm_9utjrya-rom5xl4quya26c...@mail.gmail.com

I'm tempted to quote Emerson ;).  I don't think the naming of pg_xlog
vs. pg_wal doesn't actually have that large an impact, to change the
dynamics of the wal vs xlog dichotomy.  Sure it's nothing you'd do in a
new program, but neither is it very bad.

Andres


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

Re: [HACKERS] increasing the default WAL segment size

[Andres Freund]

Hi,

On 2017-01-23 11:35:11 +0530, Beena Emerson wrote:
> Please find attached an updated WIP patch. I have incorporated almost all
> comments. This is to be applied over Robert's patches. I will post
> performance results later on.
>
> 1. shift (>>) and AND (&) operations: The assign hook of wal_segment_size
> sets the WalModMask and WalShiftBit. All the modulo and division operations
> using XLogSegSize has been replaced with these. However, there are many
> preprocessors which divide with XLogSegSize in xlog_internal.h. I have not
> changed these because it would mean I will have to reassign the WalShiftBit
> along with XLogSegSize in all the modules which use these macros. That does
> not seem to be a good idea. Also, this means shift operator can be used
> only in couple of places.

I think it'd be better not to have XLogSegSize anymore. Silently
changing a macros behaviour from being a compile time constant to
something runtime configurable is a bad idea.


> 8. Declaring XLogSegSize: There are 2 internal variables for the same
> parameter. In original code XLOG_SEG_SIZE is defined in the auto-generated
> file src/include/pg_config.h. And xlog_internal.h defines:
>
> #define XLogSegSize ((uint32) XLOG_SEG_SIZE)
>
> To avoid renaming all parts of code, I made the following change in
> xlog_internal.h
>
> + extern uint32 XLogSegSize;
>
> +#define XLOG_SEG_SIZE XLogSegSize
>
>  would it be better to just use one variable XLogSegSize everywhere. But
> few external modules could be using XLOG_SEG_SIZE. Thoughts?

They'll quite possibly break with configurable size anyway.  So I'd
rather have those broken explicitly.



> +/*
> + * These variables are set in assign_wal_segment_size
> + *
> + * WalModMask: It is an AND mask for XLogSegSize to allow for faster modulo
> + *   operations using it.
> + *
> + * WalShiftBit: It is an shift bit for XLogSegSize to allow for faster
> + *   division operations using it.
> + *
> + * UsableBytesInSegment: It is the number of bytes in a WAL segment usable 
> for
> + *   WAL data.
> + */
> +uint32   WalModMask;
> +static int   UsableBytesInSegment;
> +static int   WalShiftBit;

This could use some editorializing. "Faster modulo operations" isn't an
explaining how/why it's actually being used. Same for WalShiftBit.

>  /*
>   * Private, possibly out-of-date copy of shared LogwrtResult.
> @@ -957,6 +975,7 @@ XLogInsertRecord(XLogRecData *rdata,
>   if (!XLogInsertAllowed())
>   elog(ERROR, "cannot make new WAL entries during recovery");
>
> +
>   /*--
>*

Spurious newline change.

>   if (ptr % XLOG_BLCKSZ == SizeOfXLogShortPHD &&
> - ptr % XLOG_SEG_SIZE > XLOG_BLCKSZ)
> + (ptr & WalModMask) > XLOG_BLCKSZ)
>   initializedUpto = ptr - SizeOfXLogShortPHD;
>   else if (ptr % XLOG_BLCKSZ == SizeOfXLogLongPHD &&
> -  ptr % XLOG_SEG_SIZE < XLOG_BLCKSZ)
> +  (ptr & WalModMask) < XLOG_BLCKSZ)
>   initializedUpto = ptr - SizeOfXLogLongPHD;
>   else
>   initializedUpto = ptr;

How about we introduce a XLogSegmentOffset(XLogRecPtr) function like
macro in a first patch?  That'll reduce the amount of change in the
commit actually changing things quite noticeably, and makes it easier to
adjust things later.  I see very little benefit for in-place usage of
either % XLOG_SEG_SIZE or & WalModMask.


> @@ -1794,6 +1813,7 @@ XLogBytePosToRecPtr(uint64 bytepos)
>   uint32  seg_offset;
>   XLogRecPtr  result;
>
> +
>   fullsegs = bytepos / UsableBytesInSegment;
>   bytesleft = bytepos % UsableBytesInSegment;

spurious change.

> @@ -1878,7 +1898,7 @@ XLogRecPtrToBytePos(XLogRecPtr ptr)
>
>   XLByteToSeg(ptr, fullsegs);
>
> - fullpages = (ptr % XLOG_SEG_SIZE) / XLOG_BLCKSZ;
> + fullpages = (ptr & WalModMask) / XLOG_BLCKSZ;
>   offset = ptr % XLOG_BLCKSZ;
>
>   if (fullpages == 0)
> @@ -2043,7 +2063,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool 
> opportunistic)
>   /*
>* If first page of an XLOG segment file, make it a long header.
>*/
> - if ((NewPage->xlp_pageaddr % XLogSegSize) == 0)
> + if ((NewPage->xlp_pageaddr & WalModMask) == 0)
>   {
>   XLogLongPageHeader NewLongPage = (XLogLongPageHeader) 
> NewPage;
>
> @@ -2095,6 +2115,7 @@ CalculateCheckpointSegments(void)
>*number of segments consumed between checkpoints.
>*---
>*/
> +
>   target = (double) max_wal_size / (2.0 + CheckPointCompletionTarget);

spurious change.


>  void
> +assign_wal_segment_size(int newval, void *extra)
> +{
> + /*
> +  * During system initialization, XLogSegSize is not set so we use
> +  * DEFAULT_XLOG_SEG_SIZE instead.
> +  */
> + int 

Re: [HACKERS] increasing the default WAL segment size

[Robert Haas]

On Thu, Jan 26, 2017 at 1:34 PM, Andres Freund  wrote:
> On 2017-01-26 13:16:13 -0500, Robert Haas wrote:
>> > OK, I have done this refactoring effort as attached because I think
>> > that's really worth it. And here are the diff numbers:
>> >  3 files changed, 113 insertions(+), 162 deletions(-)
>> > That's a bit less than what I thought first because of all the
>> > singularities of bytea in its output and the way TIMELINE_HISTORY
>> > takes advantage of the message level routines. Still for
>> > IDENTIFY_SYSTEM, START_REPLICATION and CREATE_REPLICATION_SLOT the
>> > gains in readability are here.
>>
>> Seems OK to me, but I think I'd want to hear a few other opinions
>> before committing it.
>
> Just to be absolutely sure: We're talking about Michael's cleanup patch,
> not the thread's original topic?

Correct.

-- 
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] [PATCH] Generic type subscription

[Robert Haas]

On Thu, Jan 26, 2017 at 1:38 PM, Tom Lane  wrote:
> So I'd really prefer that the functionality
> involve a parser callout, and that would certainly need "internal"
> argument(s).

Thanks, I see now.

-- 
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] [COMMITTERS] pgsql: Check interrupts during hot standby waits

[Andres Freund]

Hi,

On 2017-01-26 19:00:34 +, Simon Riggs wrote:
> Check interrupts during hot standby waits
>
> Branch
> --
> master
>
> Details
> ---
> http://git.postgresql.org/pg/commitdiff/e8ee3d6b859a18d7f7375ceb9e04d256eb18aaec
>
> Modified Files
> --
> src/backend/storage/ipc/standby.c | 2 ++
> 1 file changed, 2 insertions(+)

It seems that the actual fix here would be to replace the pg_usleep loop
with a latch wait Then we also don't need to needlessly loop, and
actually react promptly to signals.

Andres


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Robert Haas]

On Thu, Jan 26, 2017 at 1:21 PM, Andres Freund  wrote:
> On 2017-01-24 16:47:29 -0500, Robert Haas wrote:
>> I'm happy to go change every last bit of it.
>
> I quite regret not aggressively opining against the renaming of pg_xlog
> to pg_wal. I think the few users deleting their data don't weigh against
> renaming a bunch of tools and function for some sense of
> consistency. This will cause pain for a couple years - maybe it'll be
> worth it 6-7 releases down the line, but I doubt before that.  But then
> the original discussion was about renaming pg_xlog, not about a general
> s/xlog/$something/.

I completely understand that position.  I have always been doubtful of
the value of renaming pg_xlog to pg_wal, and I'm not any more
dedicated to the idea now than I was when I committed that patch.  But
there was overwhelming support for it, consensus on a level rarely
seen here.

I do not think it can be right to rename the directory and not
anything else.  I stand by what I wrote in

https://www.postgresql.org/message-id/ca+tgmobehp2qbtmvyxg2x8pm_9utjrya-rom5xl4quya26c...@mail.gmail.com

-- 
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] Cache Hash Index meta page.

[Mithun Cy]

On Tue, Jan 24, 2017 at 3:10 PM, Amit Kapila  wrote:
> 1.
> @@ -505,26 +505,22 @@ hashbulkdelete(IndexVacuumInfo *info,

> In the above flow, do we really need an updated metapage, can't we use
> the cached one?  We are already taking care of bucket split down in
> that function.

Yes, we can use the old cached metap entry, the only reason I decided
to use the latest metapage content is because the old code used to do
that. And, cached metap is used to avoid ad-hoc local saving of same
and hence unify the cached metap API. I did not intend to save the
metapage read here which I thought will not be much useful if new
buckets are added anyway we need to read the metapage at the end. I
have taken you comments now I only read metap cache which is already
set.

> 2.
> The above two chunks of code look worse as compare to your previous
> patch.  I think what we can do is keep the patch ready with both the
> versions of _hash_getcachedmetap API (as you have in _v11 and _v12)
> and let committer take the final call.

_v11 API's was self-sustained one but it does not hold pins on the
metapage buffer. Whereas in _v12 we hold the pin for two consecutive
reads of metapage. I have taken your advice and producing 2 different
patches.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


cache_hash_index_meta_page_13_donotholdpin.patch
Description: Binary data


cache_hash_index_meta_page_13_holdpin.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] [PATCH] Generic type subscription

[Tom Lane]

Robert Haas  writes:
> On Mon, Jan 23, 2017 at 2:07 PM, Tom Lane  wrote:
>> Can we arrange to do that differently?  I'd prefer something in which the
>> argument and result types are visibly connected to the actual datatypes
>> at hand, for instance
>> array_subscript(anyarray, internal) returns anyelement
>> array_assign(anyarray, internal, anyelement) returns anyarray

> What about having no internal arguments here at all?  Like if you want
> to support foo[4] define a subscript function that takes (mytype, int)
> and returns whatever.  You might have to allow for multiple
> subscripting functions with different argument types for this to
> really work, though.

Yeah, the problem is if you're trying to support the full generality
of the array slice syntax, it gets kind of painful to shoehorn that
into simple SQL types.  Consider array[1][2:3][4:] or something like
that.  We could say that extensions only have access to the basic
non-slice notation, but I'm sure someone would be unhappy with that
--- and even then, you don't really want to define six functions to
deal with six possible numbers of subscripts.

Another issue is that, if someone is trying to use this facility to define
array semantics that are less screwball than what Berkeley bequeathed us,
they might not be happy with the idea that a single function
"array_subscript(anyarray, internal) returns anyelement" is what
determines the result type of a subscripting operation.  It might be for
example that you need to look at the subscripted object, as well as the
number of subscripts, before you know whether the result is an element or
a lower-dimension array.  So I'd really prefer that the functionality
involve a parser callout, and that would certainly need "internal"
argument(s).

Given that it's a parser callout, what the signatures of the runtime
function(s) are is really not our problem; the callout can construct
any darn expression tree it pleases.  There would only be some subset
of that space that ruleutils.c would know how to print as a subscripting
construct, but many people might be happy with the results reverse-listing
as a regular function or operator call.

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] increasing the default WAL segment size

[Andres Freund]

On 2017-01-26 13:16:13 -0500, Robert Haas wrote:
> > OK, I have done this refactoring effort as attached because I think
> > that's really worth it. And here are the diff numbers:
> >  3 files changed, 113 insertions(+), 162 deletions(-)
> > That's a bit less than what I thought first because of all the
> > singularities of bytea in its output and the way TIMELINE_HISTORY
> > takes advantage of the message level routines. Still for
> > IDENTIFY_SYSTEM, START_REPLICATION and CREATE_REPLICATION_SLOT the
> > gains in readability are here.
> 
> Seems OK to me, but I think I'd want to hear a few other opinions
> before committing it.

Just to be absolutely sure: We're talking about Michael's cleanup patch,
not the thread's original topic?

Andres


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2017-01-24 16:47:29 -0500, Robert Haas wrote:
> > I'm happy to go change every last bit of it.
> 
> I quite regret not aggressively opining against the renaming of pg_xlog
> to pg_wal. I think the few users deleting their data don't weigh against
> renaming a bunch of tools and function for some sense of
> consistency. This will cause pain for a couple years - maybe it'll be
> worth it 6-7 releases down the line, but I doubt before that.  But then
> the original discussion was about renaming pg_xlog, not about a general
> s/xlog/$something/.

For my 2c, I'm actually quite happy that we're finally going to start
reducing the confusion between xlog and WAL.

Let's not forget to go update the documentation to be sensible and
consistent as we're making these mechanical changes to the program
names.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] increasing the default WAL segment size

[Robert Haas]

On Tue, Jan 24, 2017 at 10:26 PM, Michael Paquier
 wrote:
> On Wed, Jan 25, 2017 at 6:58 AM, Robert Haas  wrote:
>> On Fri, Jan 20, 2017 at 7:00 PM, Michael Paquier
>>  wrote:
 No, because the output of SHOW is always of type text, regardless of
 the type of the GUC.
>>>
>>> Thinking about that over night, that looks pretty nice. What would be
>>> nicer though would be to add INT8OID and BYTEAOID in the list, and
>>> convert as well the other replication commands. At the end, I think
>>> that we should finish by being able to remove all pq_* routine
>>> dependencies in walsender.c, saving quite a couple of lines.
>>
>> Might be worth investigating, but I don't feel any obligation to do
>> that right now.  Thanks for the review; committed.
>
> OK, I have done this refactoring effort as attached because I think
> that's really worth it. And here are the diff numbers:
>  3 files changed, 113 insertions(+), 162 deletions(-)
> That's a bit less than what I thought first because of all the
> singularities of bytea in its output and the way TIMELINE_HISTORY
> takes advantage of the message level routines. Still for
> IDENTIFY_SYSTEM, START_REPLICATION and CREATE_REPLICATION_SLOT the
> gains in readability are here.

Seems OK to me, but I think I'd want to hear a few other opinions
before committing 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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Tom Lane]

Robert Haas  writes:
> Yeah, I thought about that, too, but it doesn't really seem worth it.
> If we had pg_receive_wal and pg_receive_logical, they'd be nicely
> consistent with each other, but inconsistent with practically every
> other utility we have: pg_basebackup, pg_archivecleanup,
> pg_controldata, etc.

Mmm, good point.  I was looking at pg_test_fsync and pg_test_timing,
but those are the only exceptions, and they're only quasi-user-facing
anyway.

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

[Simon Riggs]

On 26 January 2017 at 17:37, Peter Eisentraut
 wrote:
> On 1/24/17 8:19 AM, Tom Lane wrote:
>> What about just saying that the database owner has those privileges?
>> After all, the ultimate privilege of an owner is to drop the object
>> (and then remake it as she pleases), and the DB owner has that option
>> w.r.t. the whole database.  So I'm not sure we need to invent a new
>> concept.
>
> A database owner does not necessarily have the permission to create a
> new database.

So the concept I was looking for is already there: on pg_database the
database owner is referred to as the DBA.

I'd like to be able to give permision to someone to control all
user-owned objects in the database, yet not be allowed to do anything
that touches filesystem or creates potentially unsafe functions.

This allows a separation of duty between people that run a service and
people that use it.

That should include the ability to dump all objects, yet without any
security details. And it should allow someone to setup logical
replication easily, including both trigger based and new logical
replication. And GRANT ON ALL should work.

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


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

Re: [HACKERS] [PATCH] Generic type subscription

[Robert Haas]

On Mon, Jan 23, 2017 at 2:07 PM, Tom Lane  wrote:
> Can we arrange to do that differently?  I'd prefer something in which the
> argument and result types are visibly connected to the actual datatypes
> at hand, for instance
>   array_subscript(anyarray, internal) returns anyelement
>   array_assign(anyarray, internal, anyelement) returns anyarray

What about having no internal arguments here at all?  Like if you want
to support foo[4] define a subscript function that takes (mytype, int)
and returns whatever.  You might have to allow for multiple
subscripting functions with different argument types for this to
really work, though.

/me ducks

-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On 2017-01-24 16:47:29 -0500, Robert Haas wrote:
> I'm happy to go change every last bit of it.

I quite regret not aggressively opining against the renaming of pg_xlog
to pg_wal. I think the few users deleting their data don't weigh against
renaming a bunch of tools and function for some sense of
consistency. This will cause pain for a couple years - maybe it'll be
worth it 6-7 releases down the line, but I doubt before that.  But then
the original discussion was about renaming pg_xlog, not about a general
s/xlog/$something/.

- Andres


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

Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

[Fabrízio de Royes Mello]

On Mon, Jan 9, 2017 at 6:14 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
>
> On 1/9/17 1:34 PM, Robert Haas wrote:
> > On Fri, Jan 6, 2017 at 7:29 PM, Peter Eisentraut
> >  wrote:
> >> On 1/3/17 11:52 PM, Ashutosh Bapat wrote:
> >>> We will need to make CURRENT_DATABASE a reserved keyword. But I like
> >>> this idea more than COMMENT ON CURRENT DATABASE.
> >>
> >> We already have the reserved key word CURRENT_CATALOG, which is the
> >> standard spelling.  But I wouldn't be bothered if we made
> >> CURRENT_DATABASE somewhat reserved as well.
> >
> > Maybe I'm just lacking in imagination, but what's the argument against
> > spelling it CURRENT DATABASE?
>
> To achieve consistent support for specifying the current database, we
> would need to change the grammar for every command involving databases.
> And it would also set a precedent for similar commands, such as current
> user/role.  Plus support in psql, pg_dump, etc. would get more
complicated.
>
> Instead, it would be simpler to define a grammar symbol like
>
> database_name: ColId | CURRENT_DATABASE
>
> and make a small analogous change in objectaddress.c and you're done.
>
> Compare rolespec in gram.y.
>

Ok, but doing in that way the syntax would be:

COMMENT ON DATABASE CURRENT_DATABASE IS 'comment';

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

Re: [HACKERS] Superowners

[David G. Johnston]

On Thursday, January 26, 2017, Michael Banck 
wrote:

> On Thu, Jan 26, 2017 at 12:37:44PM -0500, Peter Eisentraut wrote:
> > On 1/24/17 8:19 AM, Tom Lane wrote:
> > > What about just saying that the database owner has those privileges?
> > > After all, the ultimate privilege of an owner is to drop the object
> > > (and then remake it as she pleases), and the DB owner has that option
> > > w.r.t. the whole database.  So I'm not sure we need to invent a new
> > > concept.
> >
> > A database owner does not necessarily have the permission to create a
> > new database.
>
> Right.
>
> Would a "TRUNCATE ;" (i.e. empty the database, but don't
> delete it) make sense/be useful for that maybe?
>
>
Conceptually we might already have that.

 https://www.postgresql.org/docs/current/static/sql-drop-owned.html

Drop Owned ...

Though I suspect there might be caveats since indications are it wasn't
designed with this particular use case in mind,

David J.

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> What I think might be worth considering is inserting underscores,
>> eg "pg_receive_wal", anywhere that we are running the abbreviation
>> directly against another word.  We won't get another chance.

> Wouldn't that make it 'pg_recv_wal'?  Or were you referring to the 'wal'
> as being the abbreviation?

The latter.  As far as the programs go, that would be

pg_receive_wal
pg_reset_wal
pg_wal_dump

The other cases you mention are, for the most part, words that we're
running together ("db" is the only exception) so they're not committing
double sins against readability.  Anyway, I'm not suggesting that we
should rename anything this patch isn't touching already.

regards, tom lane

PS: I'm trying hard not to open the can of worms labeled "pg_dump_wal".


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Robert Haas]

On Thu, Jan 26, 2017 at 12:41 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Jan 26, 2017 at 12:27 PM, Alvaro Herrera
>>  wrote:
>>> There have been complaints that pg_receivexlog's name is not consistent
>>> with pg_recvlogical, and I seem to recall there were some votes for
>>> renaming pg_receivexlog to match.  We could make it "pg_recvwal" now.
>
>> ... I would prefer not to go there.
>
> I agree.  "pg_recvlogical" was a badly chosen name; let's not double
> down on the error.
>
> What I think might be worth considering is inserting underscores,
> eg "pg_receive_wal", anywhere that we are running the abbreviation
> directly against another word.  We won't get another chance.

Yeah, I thought about that, too, but it doesn't really seem worth it.
If we had pg_receive_wal and pg_receive_logical, they'd be nicely
consistent with each other, but inconsistent with practically every
other utility we have: pg_basebackup, pg_archivecleanup,
pg_controldata, etc.  I'm not prepared to endorse renaming all of that
stuff just to add underscores, and frankly I don't think the style
pg_foobarbaz is really a problem.  It's a lot easier to remember "the
only underscore is after the initial pg" than it is to remember
exactly how each word was abbreviated in each context.

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

[Michael Banck]

On Thu, Jan 26, 2017 at 12:37:44PM -0500, Peter Eisentraut wrote:
> On 1/24/17 8:19 AM, Tom Lane wrote:
> > What about just saying that the database owner has those privileges?
> > After all, the ultimate privilege of an owner is to drop the object
> > (and then remake it as she pleases), and the DB owner has that option
> > w.r.t. the whole database.  So I'm not sure we need to invent a new
> > concept.
> 
> A database owner does not necessarily have the permission to create a
> new database.

Right.

Would a "TRUNCATE ;" (i.e. empty the database, but don't
delete it) make sense/be useful for that maybe?


Michael

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

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


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Thu, Jan 26, 2017 at 12:27 PM, Alvaro Herrera
> >  wrote:
> >> There have been complaints that pg_receivexlog's name is not consistent
> >> with pg_recvlogical, and I seem to recall there were some votes for
> >> renaming pg_receivexlog to match.  We could make it "pg_recvwal" now.
> 
> > ... I would prefer not to go there.
> 
> I agree.  "pg_recvlogical" was a badly chosen name; let's not double
> down on the error.
> 
> What I think might be worth considering is inserting underscores,
> eg "pg_receive_wal", anywhere that we are running the abbreviation
> directly against another word.  We won't get another chance.

Wouldn't that make it 'pg_recv_wal'?  Or were you referring to the 'wal'
as being the abbreviation?  Or did you mean underscores between words in
general?

If we want to switch to using underscores to seperate words, then that
implies a whole lot of *additional* renaming that I'm not sure I can
really get behind...

initdb
pg_archivecleanup
pg_basebackup
pgbench
pg_controldata
pgevent (eh, not really a binary, but it's in src/bin)
pg_dumpall
pg_isready

All of those, to me at least seem fine, and I'd further be fine with
"pg_receivewal".

I haven't got any great answers wrt pg_recvlogical, though I don't
particullarly like the existing name, but that's more because I have
to ask myself how the heck one can receive a "logical"..

Then again 'pg_controldata' only works if you realize it's really
"pg_print_pg_control" or something.  It's hardly controlling any data.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 26, 2017 at 12:27 PM, Alvaro Herrera
>  wrote:
>> There have been complaints that pg_receivexlog's name is not consistent
>> with pg_recvlogical, and I seem to recall there were some votes for
>> renaming pg_receivexlog to match.  We could make it "pg_recvwal" now.

> ... I would prefer not to go there.

I agree.  "pg_recvlogical" was a badly chosen name; let's not double
down on the error.

What I think might be worth considering is inserting underscores,
eg "pg_receive_wal", anywhere that we are running the abbreviation
directly against another word.  We won't get another chance.

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

[Peter Eisentraut]

On 1/24/17 8:19 AM, Tom Lane wrote:
> What about just saying that the database owner has those privileges?
> After all, the ultimate privilege of an owner is to drop the object
> (and then remake it as she pleases), and the DB owner has that option
> w.r.t. the whole database.  So I'm not sure we need to invent a new
> concept.

A database owner does not necessarily have the permission to create a
new database.

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


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

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Robert Haas]

On Thu, Jan 26, 2017 at 12:27 PM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>
>> 0002 renames programs whose names contains "xlog".
>
> There have been complaints that pg_receivexlog's name is not consistent
> with pg_recvlogical, and I seem to recall there were some votes for
> renaming pg_receivexlog to match.  We could make it "pg_recvwal" now.

Or prw.  :-)

Personally, I generally push to avoid abbreviations like receive ->
recv.  Yeah, it's a few extra characters of typing, but you can use
tab completion so it's not really a big deal I think.  And the problem
with abbreviating things is that people then have to remember whether
it was abbreviated and in exactly which way.  "Wait, was it recv or
rcv this time?"  Moreover, some of the abbreviations we've used are
potentially opaque to non-English-speakers, or in some cases even to
English speakers.  So I would prefer not to go there.  But if the
consensus is otherwise, so be 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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Alvaro Herrera]

Robert Haas wrote:

> 0002 renames programs whose names contains "xlog".

There have been complaints that pg_receivexlog's name is not consistent
with pg_recvlogical, and I seem to recall there were some votes for
renaming pg_receivexlog to match.  We could make it "pg_recvwal" now.

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


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Robert Haas]

On Wed, Jan 25, 2017 at 4:08 PM, Alvaro Herrera
 wrote:
> I think the way WARM works has been pretty well hammered by now, other
> than the CREATE INDEX CONCURRENTLY issues, so I'm looking at the code
> from a maintainability point of view only.

Which senior hackers have previously reviewed it in detail?

Where would I go to get a good overview of the overall theory of operation?

-- 
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] Allow interrupts on waiting standby

[Robert Haas]

On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
> Currently a waiting standby doesn't allow interrupts.
>
> Patch implements that.
>
> Barring objection, patching today with backpatches.

"today" is a little quick, but the patch looks fine.  I doubt anyone's
going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.

-- 
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] [COMMITTERS] pgsql: Reindent table partitioning code.

[Robert Haas]

On Thu, Jan 26, 2017 at 10:16 AM, Peter Eisentraut
 wrote:
> On 1/24/17 10:51 AM, Robert Haas wrote:
>> On Tue, Jan 24, 2017 at 10:28 AM, Tom Lane  wrote:
>>> Robert Haas  writes:
 Reindent table partitioning code.
>>>
>>> Oh, thank you, I was starting to get annoyed with that too.
>>
>> Glad you like.  The pgindent damage introduced by the logical
>> replication commits is even more extensive, but I didn't want to touch
>> that without discussion.  Peter might want to look for a time when not
>> too many patches are pending to do something similar, though.
>
> Can you give me an example of a file or area that looks bad now?

What I did was just run pgindent on the whole tree and then undid all
the stuff that was unrelated to table partitioning with 'git checkout'
and/or 'git checkout -p'.

-- 
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] Patch: Write Amplification Reduction Method (WARM)

[Alvaro Herrera]

Alvaro Herrera wrote:

> I wonder if heap_hot_search_buffer() and heap_hot_search() should return
> a tri-valued enum instead of boolean; that idea looks reasonable in
> theory but callers have to do more work afterwards, so maybe not.
> 
> I think heap_hot_search() sometimes leaving the buffer pinned is
> confusing.  Really, the whole idea of having heap_hot_search have a
> buffer output argument is an important API change that should be better
> thought.  Maybe it'd be better to return the buffer pinned always, and
> the caller is always in charge of unpinning if not InvalidBuffer.  Or
> perhaps we need a completely new function, given how different it is to
> the original?  If you tried to document in the comment above
> heap_hot_search how it works, you'd find that it's difficult to
> describe, which'd be an indicator that it's not well considered.

Even before your patch, heap_hot_search claims to have the same API as
heap_hot_search_buffer "except that caller does not provide the buffer."
But this is a lie and has been since 9.2 (more precisely, since commit
4da99ea4231e).  I think WARM makes things even worse and we should fix
that.  Not yet sure which direction to fix it ...

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Reindent table partitioning code.

[Peter Eisentraut]

On 1/24/17 10:51 AM, Robert Haas wrote:
> On Tue, Jan 24, 2017 at 10:28 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> Reindent table partitioning code.
>>
>> Oh, thank you, I was starting to get annoyed with that too.
> 
> Glad you like.  The pgindent damage introduced by the logical
> replication commits is even more extensive, but I didn't want to touch
> that without discussion.  Peter might want to look for a time when not
> too many patches are pending to do something similar, though.

Can you give me an example of a file or area that looks bad 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] Proposal : For Auto-Prewarm.

[Peter Eisentraut]

On 1/24/17 3:26 AM, Mithun Cy wrote:
> In my code by default, we only dump at shutdown time. If we want to
> dump at regular interval then we need to set the GUC
> pg_autoprewarm.buff_dump_interval to > 0.

Just a thought with an additional use case:  If I want to set up a
standby for offloading queries, could I take the dump file from the
primary or another existing standby, copy it to the new standby, and
have it be warmed up to the state of the other instance from that?

In my experience, that kind of use is just as interesting as preserving
the buffers across a restart.

-- 
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] CONNECTION LIMIT and Parallel Query don't play well together

[Robert Haas]

On Thu, Jan 26, 2017 at 7:59 AM, David Rowley
 wrote:
> On 12 January 2017 at 15:24, David Rowley  
> wrote:
>> I've attached a patch which intended to assist discussions on this topic.
>>
>> The patch adds some notes to the docs to mention that background
>> workers and prepared xacts are not counted in CONNECTION LIMIT, it
>> then goes on and makes CountUserBackends() ignore bgworkers. It was
>> already ignoring prepared xacts. There's a bit of plumbing work to
>> make the proc array aware of the background worker status. Hopefully
>> this is suitable. I'm not all that close to that particular area of
>> the code.
>
> Wondering you've had any time to glance over this?
>
> If you think the patch needs more work, or goes about things the wrong
> way, let me know, and I'll make the changes.

Sorry, this had slipped through the cracks -- I'm having a very hard
time keeping up with the flow of patches and emails.  But it looks
good to me, except that it seems like CountDBBackends() needs the same
fix (and probably a corresponding documentation update).

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