Re: TupleTableSlot abstraction

[Amit Khandekar]

On Wed, 26 Sep 2018 at 05:15, Andres Freund  wrote:
>
> Hi,
>
> On 2018-09-04 18:35:34 +0530, Amit Khandekar wrote:
> > Subject: [PATCH 05/14] Use tts_flags instead of previous bool members
> >
> > Pack the boolean members in TupleTableSlot into a 16 bit tts_flags.
> > This reduces the size of TupleTableSlot since each bool member takes
> > at least a byte on the platforms where bool is defined as a char.
> >
> > Ashutosh Bapat and Andres Freund
>
> > +
> > +/* true = slot is empty */
> > +#define  TTS_ISEMPTY (1 << 1)
> > +#define IS_TTS_EMPTY(slot)   ((slot)->tts_flags & TTS_ISEMPTY)
> > +#define SET_TTS_EMPTY(slot) ((slot)->tts_flags |= TTS_ISEMPTY)
> > +#define RESET_TTS_EMPTY(slot) ((slot)->tts_flags &= ~TTS_ISEMPTY)
>
> The flag stuff is the right way, but I'm a bit dubious about the
> accessor functions.  I can see open-coding the accesses, using the
> macros, or potentially going towards using bitfields.
>
> Any comments?

I like this abstraction using macros, since this will allow us to
conveniently change the way this information is stored inside the
slot. I think for this same reason we have defined macros
(HeapTupleIsHotUpdated, etc) for each bit of heaptuple infomask.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

Re: DSM segment handle generation in background workers

[Tom Lane]

Thomas Munro  writes:
>> Ok, here is a sketch patch with a new function InitRandomSeeds() to do
>> that.  It is called from PostmasterMain(), InitPostmasterChild() and
>> InitStandaloneProcess() for consistency.

> Here's a version I propose to push to master and 11 tomorrow, if there
> are no objections.

The comment adjacent to the change in InitStandaloneProcess bothers me.
In particular, it points out that what BackendRun() is currently doing
creates more entropy in the process's random seed than what you have
here: MyStartTime is only good to the nearest second, whereas the code
you removed from BackendRun() factors in fractional seconds to whatever
the precision of GetCurrentTimestamp is.  This does not seem like an
improvement.

I'm inclined to think we should refactor a bit more aggressively so
that, rather than dumbing backends down to the standard of other
processes, we make them all use the better method.  A reasonable way
to approach this would be to invent a global variable MyStartTimestamp
beside MyStartTime, and initialize both of them in the places that
currently initialize the latter, using code like that in
BackendInitialize:

/* save process start time */
port->SessionStartTime = GetCurrentTimestamp();
MyStartTime = timestamptz_to_time_t(port->SessionStartTime);

and then this new InitRandomSeeds function could adopt BackendRun's
seed initialization method instead of the stupider way.

Possibly it'd be sane to merge the MyStartTime* initializations
and the srandom resets into one function, not sure.

regards, tom lane

Re: Calculate total_table_pages after set_base_rel_sizes()

[Amit Langote]

On 2018/10/07 17:43, David Rowley wrote:
> On 6 October 2018 at 18:20, Edmund Horner  wrote:
>> David Rowley said:
>>> I am considering this a bug fix, but I'm proposing this for PG12 only
>>> as I don't think destabilising plans in the back branches is a good
>>> idea. I'll add this to the September commitfest.
>>
>> I played with the new patched today with a set of large partitions.
>> It seems to work, though the effect is subtle.  The expected behaviour
>> of index_pages_fetched is a bit hard to fathom when the cache share
>> pushes it between cases A,B and C of the formula, but that's not
>> really down to this patch.
> 
> Thanks for looking at this and testing it too.
> 
> The primary purpose of this patch was step 1 in delaying the creation
> of RangeTblEntrys for partitions until after partition pruning has
> taken place.  The code I had to do this caused problems around the
> total_table_pages calculation due to the lack of RangeTblEntry for the
> partitions at the time it was being calculated. But regardless of
> that, I still believe where we currently calculate this number is
> subtlely broken as it counts all partitions, even ones that will later
> be pruned, thus decreasing the likelihood of an index being used as
> random_page_cost would be applied to a higher number of index pages.
> 
> Amit Langote has since posted a patch to delay the RangeTblEntry
> creation until after pruning. His patch happens to also move the
> total_table_pages calculation, but I believe this change should be
> made as an independent commit to anything else.  I've kept it in the
> commitfest for that reason.

Yeah, if this patch is a win independent of the other project of delaying
partition RTE creation, which seems to be true, I think we should go ahead
with applying this patch.

>> Basically I think it's ready for a committer to look at.  Should I
>> update the CF entry?
> 
> That sounds good, please do.

+1

Thanks,
Amit

Re: [HACKERS] WAL logging problem in 9.4.3?

[Kyotaro HORIGUCHI]

Hello.

At Fri, 27 Jul 2018 15:26:24 -0400, Andrew Dunstan 
 wrote in 

> 
> 
> On 07/18/2018 10:58 AM, Heikki Linnakangas wrote:
> > On 18/07/18 16:29, Robert Haas wrote:
> >> On Wed, Jul 18, 2018 at 9:06 AM, Michael Paquier 
> >> wrote:
>  What's wrong with the approach proposed in
>  http://postgr.es/m/55afc302.1060...@iki.fi ?
> >>>
> >>> For back-branches that's very invasive so that seems risky to me
> >>> particularly seeing the low number of complaints on the matter.
> >>
> >> Hmm. I think that if you disable the optimization, you're betting that
> >> people won't mind losing performance in this case in a maintenance
> >> release.  If you back-patch Heikki's approach, you're betting that the
> >> committed version doesn't have any bugs that are worse than the status
> >> quo.  Personally, I'd rather take the latter bet.  Maybe the patch
> >> isn't all there yet, but that seems like something we can work
> >> towards.  If we just give up and disable the optimization, we won't
> >> know how many people we ticked off or how badly until after we've done
> >> it.
> >
> > Yeah. I'm not happy about backpatching a big patch like what I
> > proposed, and Kyotaro developed further. But I think it's the least
> > bad option we have, the other options discussed seem even worse.
> >
> > One way to review the patch is to look at what it changes, when
> > wal_level is *not* set to minimal, i.e. what risk or overhead does it
> > pose to users who are not affected by this bug? It seems pretty safe
> > to me.
> >
> > The other aspect is, how confident are we that this actually fixes the
> > bug, with least impact to users using wal_level='minimal'? I think
> > it's the best shot we have so far. All the other proposals either
> > don't fully fix the bug, or hurt performance in some legit cases.
> >
> > I'd suggest that we continue based on the patch that Kyotaro posted at
> > https://www.postgresql.org/message-id/20180330.100646.86008470.horiguchi.kyotaro%40lab.ntt.co.jp.
> >
> 
> 
> 
> I have just spent some time reviewing Kyatoro's patch. I'm a bit
> nervous, too, given the size. But I'm also nervous about leaving
> things as they are. I suspect the reason we haven't heard more about
> this is that these days use of "wal_level = minimal" is relatively
> rare.

Thank you for lokking this (and sorry for the late response).

> I like the fact that this is closer to being a real fix rather than
> just throwing out the optimization. Like Heikki I've come round to the
> view that something like this is the least bad option.
> 
> The code looks good to me - some comments might be helpful in
> heap_xlog_update()

Thanks. It is intending to avoid PANIC for a broken record. I
reverted the part since PANIC would be preferable in the case.

> Do we want to try this on HEAD and then backpatch it? Do we want to
> add some testing along the lines Michael suggested?

44cac93464 hit this, rebased. And added Michael's TAP test
contained in [1] as patch 0001.

I regard [2] as an orthogonal issue.

The previous patch didn't care of the case of
BEGIN;CREATE;TRUNCATE;COMMIT case. This version contains a "fix"
of nbtree (patch 0003) so that FPI of the metapage is always
emitted when building an empty index. On the other hand this
emits useless one or two FPIs (136 bytes each) on TRUNCATE in a
separate transaction, but it won't matter so much.. Other index
methods don't have this problem. Some other AMs emits initialize
WALs even in minimal mode.

This still has a bit too many elog(DEBUG2)s to see how it is
working. I'm going to remove most of them in the final version.

I started to prefix the file names with version 2.

regards.

[1] https://www.postgresql.org/message-id/20180711033241.gq1...@paquier.xyz

[2] 
https://www.postgresql.org/message-id/cakjs1f9if55cwx-luorerokyi9uzesxolhufdkt0wkszn+k...@mail.gmail.com

or

https://commitfest.postgresql.org/20/1811/

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 092e7412f361c39530911d4592fb46653ca027ab Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 11 Oct 2018 10:03:21 +0900
Subject: [PATCH 1/3] TAP test for copy-truncation optimization.

---
 src/test/recovery/t/016_wal_optimize.pl | 192 
 1 file changed, 192 insertions(+)
 create mode 100644 src/test/recovery/t/016_wal_optimize.pl

diff --git a/src/test/recovery/t/016_wal_optimize.pl b/src/test/recovery/t/016_wal_optimize.pl
new file mode 100644
index 00..310772a2b3
--- /dev/null
+++ b/src/test/recovery/t/016_wal_optimize.pl
@@ -0,0 +1,192 @@
+# Test WAL replay for optimized TRUNCATE and COPY records
+#
+# WAL truncation is optimized in some cases with TRUNCATE and COPY queries
+# which sometimes interact badly with the other optimizations in line with
+# several setting values of wal_level, particularly when using "minimal" or
+# "replica".  The optimization may be enabled or disabled depending on the
+# scenarios dealt here, and should never result in 

Re: Small performance tweak to run-time partition pruning

[David Rowley]

On 11 October 2018 at 16:00, Imai, Yoshikazu
 wrote:
> On Thu, Sept 6, 2018 at 7:30 PM, David Rowley wrote:
>> I've also included an additional test to ensure the other_subplans
>> gets updated correctly. The other tests for this seem to only perform
>> run-time pruning during init plan and do no further pruning, so don't
>> fully test that other_subplans gets updated correctly.
>
> I execute the sql in this test with gdb and confirmed that it tests
> other_subplans gets updated correctly. It also performs exec run-time pruning
> and actually is through the codes in the patch which update other_subplans.
>
> I also did "check world" at the latest master e9edc1ba0b and all tests passed
> successfully.

Many thanks for checking this in detail.

> It seems to me that there is no problem in this patch as far.
> Is there another thing I have to do for the review?

There's a checklist in [1]. Perhaps there's something mentioned there
that you've missed.

[1] https://wiki.postgresql.org/wiki/Reviewing_a_Patch

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

Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction

[Michael Paquier]

On Wed, Oct 10, 2018 at 03:37:50PM +0800, Richard Guo wrote:
> This is a follow-up to the issue described in thread
> https://www.postgresql.org/message-id/CAMbWs4-Mys%3DhBQSevTA8Zpd-TYFnb%3DXuHhN2TnktXMsfMUbjiQ%40mail.gmail.com
> 
> In short, during the first transaction starting phase within a backend, if
> there is an 'ereport' after setting transaction state but before saving
> CurrentUserId into 'prevUser' in TransactionStateData, 'prevUser' will
> remain as InvalidOid. Then in AbortTransaction(), CurrentUserId is
> restored with 'prevUser'. As a result, CurrentUserId will be
> InvalidOid in the rest of the session.
> 
> Attached is a patch that fixes this issue.

I guess that's an issue showing up with Greenplum as you folks are
likely tweaking how a transaction start happens?

It is as easy as doing something like that in StartTransaction() to get
into a failed state:
s->state = TRANS_START;
s->transactionId = InvalidTransactionId;/* until assigned */

+   {
+   struct stat statbuf;
+   if (stat("/tmp/hoge", ) == 0)
+   elog(ERROR, "hoge invalid state!");
+   }

Then do something like the following:
1) Start a session
2) touch /tmp/hoge
3) Issue BEGIN, which fails and initializes CurrentUserId to InvalidOid.
4) rm /tmp/hoge
3) any DDL causes the system to crash.

Anyway, looking at the patch, I am poked by the comment on top of
GetUserIdAndSecContext which states that InvalidOid can be a possible
value.  It seems to me that the root of the problem is that TRANS_STATE
is enforced to TRANS_INPROGRESS when aborting a transaction in a
starting state, in which case we should not have to reset CurrentUserId
as it has never been set.  The main reason why this was done is to
prevent a warning message to show up.

Tom, eedb068c0 is in cause here, and that's your commit.  Could you
check if something like the attached is adapted?  I am pretty sure that
we still want the sub-transaction part to still reset CurrentUserId
unconditionally by the way.

Thanks,
--
Michael
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 6cd00d9aaa..4dbedc3e00 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1924,7 +1924,12 @@ StartTransaction(void)
 	Assert(s->prevSecContext == 0);
 
 	/*
-	 * initialize other subsystems for new transaction
+	 * Initialize other subsystems for new transaction.
+	 *
+	 * XXX: Those had better be designed to never issue an ERROR as
+	 * GetUserIdAndSecContext() has been called so as a transaction
+	 * still marked as starting is able to reset what has been set
+	 * by the previous call!
 	 */
 	AtStart_GUC();
 	AtStart_Cache();
@@ -2520,28 +2525,34 @@ AbortTransaction(void)
 	 * check the current transaction state
 	 */
 	is_parallel_worker = (s->blockState == TBLOCK_PARALLEL_INPROGRESS);
-	if (s->state != TRANS_INPROGRESS && s->state != TRANS_PREPARE)
+	if (s->state != TRANS_START &&
+		s->state != TRANS_INPROGRESS &&
+		s->state != TRANS_PREPARE)
 		elog(WARNING, "AbortTransaction while in %s state",
 			 TransStateAsString(s->state));
 	Assert(s->parent == NULL);
 
-	/*
-	 * set the current transaction state information appropriately during the
-	 * abort processing
-	 */
-	s->state = TRANS_ABORT;
-
 	/*
 	 * Reset user ID which might have been changed transiently.  We need this
 	 * to clean up in case control escaped out of a SECURITY DEFINER function
 	 * or other local change of CurrentUserId; therefore, the prior value of
 	 * SecurityRestrictionContext also needs to be restored.
 	 *
+	 * If the transaction is aborted when it has been partially started, then
+	 * the user ID does not need to be set to its previous value.
+	 *
 	 * (Note: it is not necessary to restore session authorization or role
 	 * settings here because those can only be changed via GUC, and GUC will
 	 * take care of rolling them back if need be.)
 	 */
-	SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
+	if (s->state != TRANS_START)
+		SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
+
+	/*
+	 * set the current transaction state information appropriately during the
+	 * abort processing
+	 */
+	s->state = TRANS_ABORT;
 
 	/* If in parallel mode, clean up workers and exit parallel mode. */
 	if (IsInParallelMode())
@@ -3013,15 +3024,6 @@ AbortCurrentTransaction(void)
 			}
 			else
 			{
-/*
- * We can get here after an error during transaction start
- * (state will be TRANS_START).  Need to clean up the
- * incompletely started transaction.  First, adjust the
- * low-level state to suppress warning message from
- * AbortTransaction.
- */
-if (s->state == TRANS_START)
-	s->state = TRANS_INPROGRESS;
 AbortTransaction();
 CleanupTransaction();
 			}
@@ -4355,15 +4357,6 @@ AbortOutOfAnyTransaction(void)
 }
 else
 {
-	/*
-	 * We can get here after an error during transaction start
-	 * (state will be TRANS_START).  Need to clean up 

Re: DSM segment handle generation in background workers

[Thomas Munro]

On Tue, Oct 9, 2018 at 3:21 PM Thomas Munro
 wrote:
> On Tue, Oct 9, 2018 at 1:53 PM Tom Lane  wrote:
> > Thomas Munro  writes:
> > > On Mon, Oct 8, 2018 at 1:17 AM Thomas Munro
> > >  wrote:
> > >> That's because the bgworker startup path doesn't contain a call to
> > >> srandom(...distinguishing stuff...), unlike BackendRun().  I suppose
> > >> do_start_bgworker() could gain a similar call... or perhaps that call
> > >> should be moved into InitPostmasterChild().  If we put it in there
> > >> right after MyStartTime is assigned a new value, we could use the same
> > >> incantation that PostmasterMain() uses.
> >
> > > Maybe something like this?
> >
> > I think the bit with
> >
> > #ifndef HAVE_STRONG_RANDOM
> > random_seed = 0;
> > random_start_time.tv_usec = 0;
> > #endif
> >
> > should also be done in every postmaster child, no?  If we want to hide the
> > postmaster's state from child processes, we ought to hide it from all.
>
> Ok, here is a sketch patch with a new function InitRandomSeeds() to do
> that.  It is called from PostmasterMain(), InitPostmasterChild() and
> InitStandaloneProcess() for consistency.

Here's a version I propose to push to master and 11 tomorrow, if there
are no objections.

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


0001-Use-different-random-seeds-in-all-backends.patch
Description: Binary data

Re: executor relation handling

[Robert Haas]

On Tue, Oct 9, 2018 at 2:35 PM Tom Lane  wrote:
> > That last part could *easily* change in a future release.  We've
> > already started to allow CTAS with parallel query, and there have
> > already been multiple people wanting to allow more.  It would be a
> > shame if we threw up additional obstacles in the way of that...
>
> I hardly think that this is the most serious issue in the way of
> doing non-read-only things in parallel workers.

My concern, as I said, is about adding new obstacles.

> In any case, a parallel worker would surely have to open any
> relations it is going to fire triggers for.  If it gets the correct
> lock when it does that, all is well.  If not, the Assert in
> relation_open will complain.

Well, in that case, no issues.

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

Re: background worker shudown (was Re: [HACKERS] Why does logical replication launcher exit with exit code 1?)

[Robert Haas]

On Wed, Oct 10, 2018 at 12:00 AM Thomas Munro
 wrote:
> So I suppose we should just remove it, with something like 0002.  I'm
> a bit uneasy about existing code out there that might be not calling
> CFI.  OTOH I suspect that a lot of code copied worker_spi.c and
> installed its own handler.

I agree -- I think worker_spi.c has probably been copied a lot, and
that's not a good thing.

> Maybe src/test/modules/worker_spi.c shouldn't use approach #1 (even if
> it might technically be OK for that code)?  I think I might have been
> guilty of copying that.

+1.  It's not *really* OK unless all of the SQL queries it executes
are super-short and can't possibly block ... which I guess is never
really true of any SQL queries, right?

> > Maybe we could replace this by a general-purpose hook.  So instead of
> > the various tests for process types that are there now, we would just
> > have
> >
> > if (procdie_hook != NULL)
> > (*procdie_hook)();
> >
> > And that hook can do whatever you like (but probably including dying).
>
> Ok, patch 0001 is a sketch like that, for discussion.

I was assuming that we'd replace the existing message-selection logic
with customer proc-die handlers.  Also, I think we should not indicate
in the comments that the handler is expected to proc_exit() or
ereport(FATAL), because you might want to do something else there,
then return and let the usual thing happen.

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

RE: Small performance tweak to run-time partition pruning

[Imai, Yoshikazu]

On Tue, Oct 9, 2018 at 1:24 AM, I wrote:
> On Mon, Oct 8, 2018 at 1:00 AM, David Rowley wrote:
> > I've attached an updated patch which skips the re-sequence work when
> > doing that is not required for anything.
> 
> 
> 
> I saw the patch and it seems good to me about the codes.
> I still couldn't check additional test cases in the patch.

I checked an additional test which is:

On Thu, Sept 6, 2018 at 7:30 PM, David Rowley wrote:
> I've also included an additional test to ensure the other_subplans
> gets updated correctly. The other tests for this seem to only perform
> run-time pruning during init plan and do no further pruning, so don't
> fully test that other_subplans gets updated correctly.

I execute the sql in this test with gdb and confirmed that it tests
other_subplans gets updated correctly. It also performs exec run-time pruning
and actually is through the codes in the patch which update other_subplans.

I also did "check world" at the latest master e9edc1ba0b and all tests passed 
successfully.

It seems to me that there is no problem in this patch as far.
Is there another thing I have to do for the review?


--
Yoshikazu Imai

RE: pgbench - doCustom cleanup

[Jamison, Kirk]

Hello Fabien,

I have decided to take a look into this patch.
--
patching file src/bin/pgbench/pgbench.c
Hunk #1 succeeded at 296 (offset 29 lines).
[…Snip…]
Hunk #21 succeeded at 5750 (offset 108 lines).
patching file src/include/portability/instr_time.h
….
===
 All 189 tests passed. 
===
Build success

It applies cleanly, passes the tests, and builds successfully.

The simplification and refactoring of code also seems logical to me.
That said, the code looks clean and the comments are clear.
Agree that it makes sense to move all the state changes from threadRun() to 
doCustom() for code consistency.

I noticed in your refactored code that CSTATE_CHOOSE_SCRIPT may now also change 
to CSTATE_FINISHED when timer is exceeded. (Before, it only changes to either 
CSTATE_START_THROTTLE or CSTATE_START_TX)
But the change has not been indicated YET in the typedef enum part for 
CSTATE_CHOOSE_SCRIPT.
[snip]
/*
 * The client must first choose a script to execute.  Once chosen, it 
can
 * either be throttled (state CSTATE_START_THROTTLE under --rate) or 
start
 * right away (state CSTATE_START_TX).
 */
CSTATE_CHOOSE_SCRIPT,

As for the behavioral change, it is assumed that there will be 1 script per 
transaction run. 
After code reading, I interpreted the "modified" state changes below: 

1. CSTATE_CHOOSE_SCRIPT may now switch CSTATE_FINISHED
Previously, it may only switch to CSTATE_THROTTLE or CSTATE_START_TX.

2. CSTATE_START_THROTTLE (Similar to #1) may now switch to CSTATE_FINISHED.
Previously, it may only switch to CSTATE_THROTTLE or CSTATE_START_TX.

3. CSTATE_START_TX: Transactions, once started, cannot be interrupted while 
running, and now proceeds to first command. Interrupt may only be allowed 
either after tx error or when tx run ends.

4. CSTATE_SKIP_COMMAND
No particular change in code logic, but clarified in the typedef that this 
state can move forward several commands until it meets next commands or 
finishes script.

5. CSTATE_END_TX: either starts over with new script (CSTATE_CHOOSE_SCRIPT) or 
finishes (CSTATE_FINISHED).
Previously, it either defaults to CSTATE_START_THROTTLE when choosing a new 
script, or finishes.


Regards,
Kirk Jamison




-Original Message-
From: Fabien COELHO [mailto:coe...@cri.ensmp.fr] 
Sent: Saturday, August 11, 2018 6:14 PM
To: PostgreSQL Developers 
Subject: pgbench - doCustom cleanup


Hello pgdev,

This patch rework & clarifies pgbench internal state machine.

It was indirectly triggered by Heikki review of pgbench tap tests 
(https://commitfest.postgresql.org/19/1306/), and by Marina's patch about tx 
retry on some errors (https://commitfest.postgresql.org/19/1645/) which I am 
reviewing.

- it adds more comments to the enum state definitions and to doCustom.

- there is some code cleanup/simplifications:
   . a few useless intermediate variables are removed,
   . a macro is added to avoid a repeated pattern to set the current time,
   . performance data are always collected instead of trying to be clever
 and not collect some data in some cases.

- more fundamentally, all state changes are performed within doCustom,
   prior that there was one performed by threadRun which made undertanding
   the end of run harder. Now threadRun only look at the current state
   to make decisions about a client.

- doCustom is made to always return at the end of a script to avoid
   an infinite loop on backslash-command only script, instead of hack
   with a variable to detect loops, which made it return every two
   script runs in such cases.

- there is a small behavioral change:

   prior to the patch, a script would always run to its end once started,
   with the exception of \sleep commands which could be interrupted by
   threadRun.

   Marina's patch should enforce somehow one script = one transaction so
   that a retry makes sense, so this exception is removed to avoid
   aborting a tx implicitely.

--
Fabien.

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Michael Paquier]

On Wed, Oct 10, 2018 at 02:20:53PM +0200, Daniel Gustafsson wrote:
>> On 9 Oct 2018, at 07:38, Michael Paquier  wrote:
>> In order to make a test with non-ASCII characters in the error message,
>> we could use a trick similar to xml.sql: use a function which ignores
>> the test case if server_encoding is not UTF-8 and use something like
>> chr() to pass down a messages with non-ASCII characters.  Then if the
>> function catches the error we are good.
> 
> Thats one approach, do you think it’s worth adding to ensure clipping during
> truncation?

I am not exactly sure what you mean here.

> > The facility to store messages should be in its own file, and
> > signalfuncs.c should depend on it, something like signal_message.c?
> 
> It was originally in backend_signal.c, and was moved into (what is now)
> signalfuncs.c in the refactoring that Alvaro suggested.  Re-reading
> his email I’m not sure he actually proposed merging this code with the
> signal code.

Indeed, you could understand the past suggestion both ways.  From what I
can see, there is merit in keeping the SQL-callable functions working on
signals into their own file.  The message capacity that you are
proposing here should on their own, and...
 
> Moved the new functionality to signal_message.{ch}.

That's actually a much better name than anything I could think of.

>> - More information could make this facility more generic: an elevel to
>> be able to fully manipulate the custom error message, and a breakpoint
>> definition.  As far as I can see you have two of them in the patch which
>> are the callers of ConsumeBackendSignalFeedback().  One event cancels
>> the query, and another terminates it.  In both cases, the breakpoint
>> implies the elevel.  So could we have more possible breakpoints possible
>> and should we try to make this API more pluggable from the start?
> 
> I’m not sure I follow, can you elaborate on this?

Sure.  From what I can see in your patch is that you want to generate in
some code paths an ereport() call with a given message string.  As far
as I can see, you can decide three things with what's available:
- errcode
- errstring
- the moment you want the ereport() to be generated.

What I am suggesting here is to let callers extend the possibilities a
bit more with:
- elevel
- errdetail
This way, you can override code paths with a more custom experience.
The thing could break easily Postgres, as you should not really use a
non-FATAL elevel when terminating a session, but having a flexible API
will allow to not come back to it in the future.  We may also want to
have ConsumeBackendSignalFeedback() call ereport() by itself with all
the details available in the data registered.

The main take on your feature is that it is more flexible than the elog
hook, as you can enforce custom strings at function call, and don't need
to do some weird hacks in plugins in need of manipulating the error.

>> - Is orig_length really useful in the data stored?  Why not just
>> warning/noticing the caller defining the message and just store the
>> message.
> 
> The caller is being notified, but the original length is returned such that 
> the
> consumer can format the message with the truncation in mind.  In postgres.c we
> currently do:
> 
> ereport(FATAL,
> (errcode(sqlerrcode),
>  errmsg("%s%s",
> buffer, (len > sizeof(buffer) ? "..." : "")),
>  errdetail("terminating connection due to administrator command 
> by process %d",
>pid)));

The goal is to generate an elog() at the end, so I can see your point,
not sure if that's worth the complication though..

>> So, looking at your patch, I am wondering also about splitting it into
>> a couple of pieces for clarity:
>> - The base facility to be able to register and consume messages which
>> can be attached to a backend slot, and then be accessed by other things.
>> Let's think about it as a base facility which is useful for extensions
>> and module developers.  If coupled with a hook, it would be actually
>> possible to scan a backend's slot for some information which could be
>> consumed afterwards for custom error messages...
>> - The set of breakpoints which can then be used to define if a given
>> code path should show up a custom error message.  I can think of three
>> of them: cancel, termination and extension, which is a custom path that
>> extensions can use.  The extension breakpoint would make sense as
>> something included from the start, could be stored as an integer and I
>> guess should not be an enum but a set of defined tables (see pgstat.h
>> for wait event categories for example).
>> - The set of SQL functions making use of it in-core, in your case these
>> are the SQL functions for cancellation and termination.
> 
> This does not sound like a bad idea to me.  Is that something you are
> planning to do or do you want me to cut the patch up into pieces?
> Just want to avoid stepping on each 

Re: Refactor textToQualifiedNameList()

[Michael Paquier]

On Wed, Oct 10, 2018 at 09:45:22AM -0300, Alvaro Herrera wrote:
> On 2018-Oct-10, Michael Paquier wrote:
>> On Tue, Oct 09, 2018 at 09:54:12AM -0300, Alvaro Herrera wrote:
>>> The committer of such a change will get a lot of flak for changing the
>>> #include requirements for code that calls that function, though.
>> 
>> So the patch has been switched to rejected...
> 
> I know -- I'm just disagreeing with that.

The point of moving stringToQualifiedNameList out of regproc.c to
varlena.c is actually a good one, if we don't add the extra palloc
overhead.  Now I suspect that any change of header makes plugin
maintainers unhappy, and I can see this routine extensively used on
github, like pipelineDB or such.
--
Michael


signature.asc
Description: PGP signature

Re: Postgres 11 release notes

[Bruce Momjian]

On Wed, Oct 10, 2018 at 06:13:56PM -0400, Bruce Momjian wrote:
> On Wed, Oct 10, 2018 at 06:06:01PM -0400, Bruce Momjian wrote:
> > On Wed, Oct 10, 2018 at 12:55:33PM -0700, Andres Freund wrote:
> > > Several people argued for introducing it, you're the only one that was
> > > against it. Adrien, Amit Kapila, Peter Geoghegan, and I all said we
> > > think that kind of thing should be included.
> > 
> > Then you need to start a new thread and argue that we need to include
> > more optimizer items in the release notes.  My point was that I was
> > using the existing criteria, and if we want to change it, we have to do
> > it consistently, not just jam in one item.
> > 
> > Once we agree we can go back and see what we missed in PG 11.
> 
> Years ago someone said they wanted more optimizer items in the release
> notes, so I did it, and then many more people said they didn't want it
> --- I don't want to repeat the experience.

Thinking some more, if this is this _only_ optimizer improvement you
think I missed, let's just add it and call it done, and we will not
re-litigate the criteria used for the release notes.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Postgres 11 release notes

[Bruce Momjian]

On Wed, Oct 10, 2018 at 06:06:01PM -0400, Bruce Momjian wrote:
> On Wed, Oct 10, 2018 at 12:55:33PM -0700, Andres Freund wrote:
> > Several people argued for introducing it, you're the only one that was
> > against it. Adrien, Amit Kapila, Peter Geoghegan, and I all said we
> > think that kind of thing should be included.
> 
> Then you need to start a new thread and argue that we need to include
> more optimizer items in the release notes.  My point was that I was
> using the existing criteria, and if we want to change it, we have to do
> it consistently, not just jam in one item.
> 
> Once we agree we can go back and see what we missed in PG 11.

Years ago someone said they wanted more optimizer items in the release
notes, so I did it, and then many more people said they didn't want it
--- I don't want to repeat the experience.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Postgres 11 release notes

[Bruce Momjian]

On Wed, Oct 10, 2018 at 12:55:33PM -0700, Andres Freund wrote:
> Hi,
> 
> On 2018-10-08 21:28:02 -0400, Bruce Momjian wrote:
> > On Mon, Oct  8, 2018 at 05:44:32PM +0200, Adrien NAYRAT wrote:
> > > On 10/8/18 5:20 PM, Bruce Momjian wrote:
> > > >Uh, where are you looking?  We don't rebuild the beta docs until the
> > > >next beta release, but you can see the changes in the developer docs:
> > > >
> > > > https://www.postgresql.org/docs/devel/static/release-11.html
> > > 
> > > I looked in doc/src/sgml/release-11.sgml
> > > 
> > > And even on https://www.postgresql.org/docs/devel/static/release-11.html I
> > > don't see mention of 1f6d515a67
> > 
> > I did not think there was sufficient interest in adding this item.  If
> > we add it we need to discuss adding all similar items.
> 
> Several people argued for introducing it, you're the only one that was
> against it. Adrien, Amit Kapila, Peter Geoghegan, and I all said we
> think that kind of thing should be included.

Then you need to start a new thread and argue that we need to include
more optimizer items in the release notes.  My point was that I was
using the existing criteria, and if we want to change it, we have to do
it consistently, not just jam in one item.

Once we agree we can go back and see what we missed in PG 11.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [HACKERS] SERIALIZABLE with parallel query

[Kevin Grittner]

On Mon, Oct 8, 2018 at 9:40 PM Thomas Munro
 wrote:
> Rebased.

It applies and builds clean, it passed make world with cassert and TAP
tests, and I can't see any remaining flaws.  This is true both of just
the 0001 v16 patch and that with 0002 v16 applied on top of it.

It would be great if someone with a big test machine could stress test
and benchmark this versus current production versions.

-- 
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/

Re: automatic restore point

[Peter Eisentraut]

On 05/10/2018 15:26, Peter Eisentraut wrote:
> It looked for a moment that
> 
> isCompleteQuery = (context <= PROCESS_UTILITY_QUERY)
> 
> in ProcessUtilitySlow() might be a problem, since that omits
> PROCESS_UTILITY_QUERY_NONATOMIC, but it's not actually a problem, since
> the commands that run this way (CALL and SET from PL/pgSQL) don't have
> event triggers.  But anyway, I propose the attached patch to rephrase
> that.  Also some tests that show it all works as expected.

I have committed these to master.

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

Re: logical decoding bug when mapped relation with toast contents is rewritten repeatedly

[Andres Freund]

Hi,

On 2018-09-13 19:10:46 -0700, Andres Freund wrote:
> (Tomas, CCing you because you IIRC mentioned encountered an issue like
> this)
> 
> I just spent quite a while debugging an issue where running logical
> decoding yielded a:
> ERROR:  could not map filenode "base/$X/$Y" to relation OID
> error.
> 
> After discarding like 30 different theories, I have found the cause:
> 
> During rewrites (i.e. VACUUM FULL / CLUSTER) of a mapped relation with a
> toast table with actual live toasted tuples (pg_proc in my case and
> henceforth) heap inserts with the toast data happen into the new toast
> relation, triggered by:

> At that point the new toast relation does *NOT* appear to be a system
>OA catalog, it's just appears as an "independent" table.  Therefore we do
> not trigger, in heap_insert():
> 
> /*
>  * RelationIsLogicallyLogged
>  *True if we need to log enough information to extract the data 
> from the
>  *WAL stream.
>  *
>  * We don't log information for unlogged tables (since they don't WAL log
>  * anyway) and for system tables (their content is hard to make sense of, and
>  * it would complicate decoding slightly for little gain). Note that we *do*
>  * log information for user defined catalog tables since they presumably are
>  * interesting to the user...
>  */
> #define RelationIsLogicallyLogged(relation) \
>   (XLogLogicalInfoActive() && \
>RelationNeedsWAL(relation) && \
>!IsCatalogRelation(relation))
> 
>   /*
>* For logical decoding, we need the tuple even if we're doing 
> a full
>* page write, so make sure it's included even if we take a 
> full-page
>* image. (XXX We could alternatively store a pointer into the 
> FPW).
>*/
>   if (RelationIsLogicallyLogged(relation))
>   {
>   xlrec.flags |= XLH_INSERT_CONTAINS_NEW_TUPLE;
>   bufflags |= REGBUF_KEEP_DATA;
>   }
> 
> i.e. the inserted toast tuple will be marked as
> XLH_INSERT_CONTAINS_NEW_TUPLE - which it shouldn't, because it's a
> system table. Which we currently do not allow do be logically decoded.
> 
> That normally ends up being harmless, because ReorderBufferCommit() has the
> following check:
>   if 
> (!RelationIsLogicallyLogged(relation))
>   goto change_done;
> 
> but to reach that check, we first have to map the relfilenode from the
> WAL to the corresponding OID:
>   reloid = 
> RelidByRelfilenode(change->data.tp.relnode.spcNode,
>   
> change->data.tp.relnode.relNode);
> 
> That works correctly if there's only one rewrite - the relmapper
> contains the data for the new toast table.  But if there's been *two*
> consecutive rewrites, the relmapper *does not* contain the intermediary
> relfilenode of pg_proc.  There's no such problem for non-mapped tables,
> because historic snapshots allow us to access the relevant data, but the
> relmapper isn't mvcc.
> 
> Therefore the catalog-rewrite escape hatch of:
>   /*
>* Catalog tuple without data, emitted 
> while catalog was
>* in the process of being rewritten.
>*/
>   if (reloid == InvalidOid &&
>   change->data.tp.newtuple == 
> NULL &&
>   change->data.tp.oldtuple == 
> NULL)
>   goto change_done;
> does not trigger and we run into:
>   else if (reloid == InvalidOid)
>   elog(ERROR, "could not map 
> filenode \"%s\" to relation OID",
>
> relpathperm(change->data.tp.relnode,
>   
>  MAIN_FORKNUM));
> 
> 
> commenting out this error / converting it into a warning makes this case
> harmless, but could obviously be problematic in other scenarios.
> 
> 
> I suspect the proper fix would be to have a new HEAP_INSERT_NO_LOGICAL
> option, and specify that in raw_heap_insert() iff
> RelationIsLogicallyLogged(state->rs_old_rel) or something like that.
> 
> Attached is a *prototype* patch of that approach.  Without the code
> level changes the addition to test_decoding's rewrite.sql trigger the
> bug, after it they're fixed.
> 
> 
> The only reason the scenario I was debugging hit this was that there was
> a cluster wide VACUUM FULL a couple times a day, and replication was
> several hours behind due to slow network / receiving side.

I've pushed this now.  I 

Re: Postgres 11 release notes

[Andres Freund]

Hi,

On 2018-10-08 21:28:02 -0400, Bruce Momjian wrote:
> On Mon, Oct  8, 2018 at 05:44:32PM +0200, Adrien NAYRAT wrote:
> > On 10/8/18 5:20 PM, Bruce Momjian wrote:
> > >Uh, where are you looking?  We don't rebuild the beta docs until the
> > >next beta release, but you can see the changes in the developer docs:
> > >
> > >   https://www.postgresql.org/docs/devel/static/release-11.html
> > 
> > I looked in doc/src/sgml/release-11.sgml
> > 
> > And even on https://www.postgresql.org/docs/devel/static/release-11.html I
> > don't see mention of 1f6d515a67
> 
> I did not think there was sufficient interest in adding this item.  If
> we add it we need to discuss adding all similar items.

Several people argued for introducing it, you're the only one that was
against it. Adrien, Amit Kapila, Peter Geoghegan, and I all said we
think that kind of thing should be included.

Greetings,

Andres Freund

Re: file cloning in pg_upgrade and CREATE DATABASE

[Bruce Momjian]

On Tue, Oct  2, 2018 at 11:07:06PM -0300, Alvaro Herrera wrote:
> On 2018-Oct-03, Michael Paquier wrote:
> 
> > On Tue, Oct 02, 2018 at 10:35:02PM -0300, Alvaro Herrera wrote:
> > > I'm not clear what interface are you proposing.  Maybe they would just
> > > use the clone-or-fail mode, and note whether it fails?  If that's not
> > > it, please explain.
> > 
> > Okay.  What I am proposing is to not have any kind of automatic mode to
> > keep the code simple, with a new option called --transfer-mode, able to
> > do three things:
> > - "link", which is the equivalent of the existing --link.
> > - "copy", the default and the current behavior, which copies files.
> > - "clone", the new mode proposed in this thread.
> 
> I see.  Peter is proposing to have a fourth mode, essentially
> --transfer-mode=clone-or-copy.

Uh, if you use --link, and the two data directories are on different
file systems, it fails.  No one has ever asked for link-or-copy, so why
are we considering clone-or-copy?  Are we going to need
link-or-clone-or-copy too?  I do realize that clone and copy have
non-destructive behavior on the old cluster once started, so it does
make some sense to merge them, unlike link.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Question about resource owners

[pgsql]

The way we handle resource owners seems a bit inconsistent.

* Transactions always have a resource owner (hierarchical for
subtransactions).

* Portals have resource owners where the parent is the transaction's
resource owner.

* Some auxiliary processes have one, but if that process were to
start/end transactions, it would clobber CurrentResourceOwner and it
would just be left with AuxProcessResourceOwner.

* It doesn't seem like a custom background worker process can make use
of resource owners in all cases, because the cleanup after sigsetjmp
doesn't release them unless it's attached to shared memory (which might
ordinarily be the case, but I don't know why we'd assume that).

Why don't we just use a hierarchy of resource owners, so that there's
always a TopResourceOwner and a non-NULL CurrentResourceOwner in every
process? If we don't expect some particular process to hold resources
there, we can emit a WARNING.

Regards,
Jeff Davis

Re: Requesting advanced Group By support

[Tom Lane]

Andres Freund  writes:
> On October 10, 2018 10:37:40 AM PDT, Tom Lane  wrote:
>> Tomas Vondra  writes:
>>> So, which part of this supposedly does not work in PostgreSQL?

>> The part where it infers that b.sno is unique based solely on it having
>> been equated to a.sno.

> Isn't the spec compliant thing that's missing dealing with unique not null?

IIRC, the spec has a whole bunch of "functional dependency" proof rules,
of which the only one we implement at the moment is the one about the
other columns of a table all being functionally dependent on its pkey.

I don't know if any of the spec's rules are at all close to this one.

regards, tom lane

Re: Requesting advanced Group By support

[Andres Freund]

Hi,

On October 10, 2018 10:37:40 AM PDT, Tom Lane  wrote:
>Tomas Vondra  writes:
>> On 10/09/2018 03:10 PM, Arun Kumar wrote:
>>> *SELECT a.sno,b.sno,a.name,b.location FROM Name AS a JOIN Location
>AS b 
>>> ON a.sno=b.sno GROUP BY a.sno,b.location *
>>> 
>>> In this case, a.sno is a primary key so no need to include a.name in
>
>>> GROUP By as it would be identified by the primary key and then for
>b.sno 
>>> which is again equated with a.sno (primary key) so no need to add
>this 
>>> as well but for b.location, we need to add it in GROUP BY or we
>should 
>>> use any aggregate function over this column to avoid error.
>
>> So, which part of this supposedly does not work in PostgreSQL?
>
>The part where it infers that b.sno is unique based solely on it having
>been equated to a.sno.

Isn't the spec compliant thing that's missing dealing with unique not null?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Sv: Re: Requesting advanced Group By support

[Andreas Joseph Krogh]

På onsdag 10. oktober 2018 kl. 18:46:15, skrev Tomas Vondra <
tomas.von...@2ndquadrant.com >:
Hi,

 On 10/09/2018 03:10 PM, Arun Kumar wrote:
 > Hi,
 >  From MySQL 5.7, It supports SQL standard 99 and implements the feature
 > such functional dependent on the GROUP By columns, i.e., it detects the
 > non-aggregate columns which are functionally dependent on the GROUP BY
 > columns (not included in GROUP BY) and then executes the query without
 > error.
 > For example,
 >
 > *SELECT a.sno,b.sno,a.name,b.location FROM Name AS a JOIN Location AS b
 > ON a.sno=b.sno GROUP BY a.sno,b.location *
 >
 > In this case, a.sno is a primary key so no need to include a.name in
 > GROUP By as it would be identified by the primary key and then for b.sno
 > which is again equated with a.sno (primary key) so no need to add this
 > as well but for b.location, we need to add it in GROUP BY or we should
 > use any aggregate function over this column to avoid error. For more
 > info, please check on the below link
 > https://dev.mysql.com/doc/refman/5.7/en/group-by-handling.html
 > Is there any plans on supporting this in Postgres in future versions ?
 >

 So, which part of this supposedly does not work in PostgreSQL?

 Consider this:

 test2=# create table t (id int primary key, b int, c int, d int);
 CREATE TABLE
 test2=# explain select * from t group by id, b, c;
                           QUERY PLAN
 
   HashAggregate  (cost=33.12..51.62 rows=1850 width=16)
     Group Key: id
     ->  Seq Scan on t  (cost=0.00..28.50 rows=1850 width=16)
 (3 rows)

 test2=# explain select id, count(*) from t group by id, b, c;
                           QUERY PLAN
 
   HashAggregate  (cost=37.75..56.25 rows=1850 width=20)
     Group Key: id
     ->  Seq Scan on t  (cost=0.00..28.50 rows=1850 width=12)
 (3 rows)

 So clearly we've already eliminated the functionally-dependent columns
 from the aggregation.

 regards
 
Too bad this doesn't:
 
create table t (id int NOT NULL UNIQUE, b int, c int, d int);
  
explain select * from t group by id, b, c; 
 ERROR:  column "t.d" must appear in the GROUP BY clause or be used in an 
aggregate function
 LINE 1: explain select * from t group by id, b, c;
  
 
-- Andreas Joseph Krogh
​

Re: Requesting advanced Group By support

[Tom Lane]

I wrote:
> Tomas Vondra  writes:
>> So, which part of this supposedly does not work in PostgreSQL?

> The part where it infers that b.sno is unique based solely on it having
> been equated to a.sno.

Oh, wait a second: such an inference is actually *wrong* in the general
case, or at least underdetermined.  It fails in cases where the data type
considers distinguishable values to be "equal", as for example zero vs.
minus zero in IEEE floats, or numeric values with varying numbers of
trailing zeroes, or citext, etc.  So for example if the sno columns are
type citext, we can be sure that a.sno does not contain both 'X' and 'x',
because the pkey would forbid it.  But if it contains 'X', while b.sno
contains both 'X' and 'x', then (if we allowed this case) it'd be
indeterminate which b.sno value is returned by the GROUP BY.  One might or
might not consider that OK for a particular application, but I don't think
the parser should just assume for you that it is.

regards, tom lane

Re: Requesting advanced Group By support

[Tom Lane]

Tomas Vondra  writes:
> On 10/09/2018 03:10 PM, Arun Kumar wrote:
>> *SELECT a.sno,b.sno,a.name,b.location FROM Name AS a JOIN Location AS b 
>> ON a.sno=b.sno GROUP BY a.sno,b.location *
>> 
>> In this case, a.sno is a primary key so no need to include a.name in 
>> GROUP By as it would be identified by the primary key and then for b.sno 
>> which is again equated with a.sno (primary key) so no need to add this 
>> as well but for b.location, we need to add it in GROUP BY or we should 
>> use any aggregate function over this column to avoid error.

> So, which part of this supposedly does not work in PostgreSQL?

The part where it infers that b.sno is unique based solely on it having
been equated to a.sno.

I'm not sure whether the SQL spec's definition of functional dependencies
includes such a proof rule, but I'm not very excited about adding one to
PG.  It's likely of limited use, seeing that this is the first time I can
recall anyone asking for it; and it'd create dependency problems that we
don't have today, because validity of the query would depend on the
existence of a btree operator class from which we could infer that
uniqueness of a.sno implies uniqueness of b.sno.  We have enough problems
arising from the existing case of validity of the query depending on the
existence of a primary key.  Also, a primary key is at least a
well-defined dependency (there can be only one); but since an equality
operator could belong to multiple opclasses, it's not very clear which
one the query would get marked as depending on.

In short: the cost/benefit ratio of this optimization looks pretty bad.

regards, tom lane

Re: Proposal for Signal Detection Refactoring

[Chris Travers]

On Tue, Oct 9, 2018 at 4:04 PM Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 01/10/2018 14:00, Chris Travers wrote:
> >
> >
> > On Wed, Sep 26, 2018 at 9:54 AM Chris Travers  > > wrote:
> >
> >
> >
> > On Tue, Sep 25, 2018 at 3:23 PM Tom Lane  > > wrote:
> >
> > Chris Travers  > > writes:
> > > However,  what I think one could do is use a struct of volatile
> > > sig_atomic_t members and macros for checking/setting.  Simply
> > writing a
> > > value is safe in C89 and higher.
> >
> > Yeah, we could group those flags in a struct, but what's the
> point?

>
> >
> > This was one of two things I noticed in my previous patch on
> > interrupts and loops where I wasn't sure what the best practice in
> > our code is.
> >
> > If we don't want to make this change, then would there be any
> > objection to me writing up a README describing the flags, and best
> > practices in terms of checking them in our code based on the current
> > places we use them?  If the current approach will be unlikely to
> > change in the future, then at least we can document that the way I
> > went about things is consistent with current best practices so next
> > time someone doesn't really wonder.
> >
> >
> > Attaching a first draft of a readme.  Feedback welcome.  I noticed
> > further that we used to document signals and what they did with carious
> > processes but that this was removed after 7.0, probably due to
> > complexity reasons.
>
> diff --git a/src/backend/utils/init/README b/src/backend/utils/init/README
> new file mode 100644
> index 00..0472687f53
> --- /dev/null
> +++ b/src/backend/utils/init/README
> @@ -0,0 +1,96 @@
> +src/backend/utils/misc/README
>
> Not only are the directory names mismatched here, but I wonder whether
> this file would be long in either of these directories.
>

I am open to suggestions here.  The file was put where the signal stuff
was.  Maybe moving it up and calling it signals.README?

>
> +Implementation Notes on Globals and Signal/Event Handling
> +=
> +
> +The approch to signal handling in PostgreSQL is designed to strictly
> conform
> +with the C89 standard and designed to run with as few platform
> assumptions as
> +possible.
>
> We use C99 now, so why would be design this to be conforming to C89?
>

Can or worms alert:  C89 and C99 are functionally the same here.  C99
changed the spec in part because every known C89 implementation was
compliant in this area with the C99 spec.

I am fine with bumping that up to C99.

>
>
> More generally, I'd like this material to be code comments.  It's the
> kind of stuff that gets outdated before long if it's kept separate.
>

The problem is that code comments are not going to be good places to
document "how do I check for pending actions?"  That could be moved to the
main SGML I guess.

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


-- 
Best Regards,
Chris Travers
Head of Database

Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin

Re: Requesting advanced Group By support

[Tomas Vondra]

Hi,

On 10/09/2018 03:10 PM, Arun Kumar wrote:

Hi,
 From MySQL 5.7, It supports SQL standard 99 and implements the feature 
such functional dependent on the GROUP By columns, i.e., it detects the 
non-aggregate columns which are functionally dependent on the GROUP BY 
columns (not included in GROUP BY) and then executes the query without 
error.

For example,

*SELECT a.sno,b.sno,a.name,b.location FROM Name AS a JOIN Location AS b 
ON a.sno=b.sno GROUP BY a.sno,b.location *


In this case, a.sno is a primary key so no need to include a.name in 
GROUP By as it would be identified by the primary key and then for b.sno 
which is again equated with a.sno (primary key) so no need to add this 
as well but for b.location, we need to add it in GROUP BY or we should 
use any aggregate function over this column to avoid error. For more 
info, please check on the below link 
https://dev.mysql.com/doc/refman/5.7/en/group-by-handling.html

Is there any plans on supporting this in Postgres in future versions ?



So, which part of this supposedly does not work in PostgreSQL?

Consider this:

test2=# create table t (id int primary key, b int, c int, d int);
CREATE TABLE
test2=# explain select * from t group by id, b, c;
 QUERY PLAN

 HashAggregate  (cost=33.12..51.62 rows=1850 width=16)
   Group Key: id
   ->  Seq Scan on t  (cost=0.00..28.50 rows=1850 width=16)
(3 rows)

test2=# explain select id, count(*) from t group by id, b, c;
 QUERY PLAN

 HashAggregate  (cost=37.75..56.25 rows=1850 width=20)
   Group Key: id
   ->  Seq Scan on t  (cost=0.00..28.50 rows=1850 width=12)
(3 rows)

So clearly we've already eliminated the functionally-dependent columns 
from the aggregation.


regards

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

Re: NOTIFY and pg_notify performance when deduplicating notifications

[Catalin Iacob]

On Wed, Oct 10, 2018 at 5:42 PM Catalin Iacob  wrote:
> One way could be to take inspiration from
> src/test/isolation/specs/async-notify.spec and check that
> pg_notification_queue_usage() does grow when repeating the same
> payload with collapse_mode='never' (while for always it would grow).

Sorry, the last part should be "(while for *maybe* it would *not* grow)".

Re: NOTIFY and pg_notify performance when deduplicating notifications

[Catalin Iacob]

On Tue, Oct 9, 2018 at 2:17 PM  wrote:
> I just caught an error in my patch, it's fixed in the attachment. The 'never' 
> and 'maybe' collapse modes were mixed up in one location.

Here's a partial review of this version, did not read the doc part
very carefully.

First of all, I agree that this is a desirable feature as, for a large
number of notiifications, the O(n^2) overhead quickly becomes very
noticeable.

I would expect the collapse mode to be an enum which is created from
the string early on during parsing and used for the rest of the code.
Instead the string is used all the way leading to string comparisons
in the notification dispatcher and to the need of hardcoding special
strings in various places, including the contrib module.

This comment in the beginning of async.c should also be updated:
*   Duplicate notifications from the same transaction are sent out as one
*   notification only. This is done to save work when for example a trigger

pg_notify_3args duplicates pg_notify, I would expect a helper function
to be extracted and called from both.

There are braces placed on the same line as the if, for example if
(strlen(collapse_mode) != 0) { which seems to not be the project's
style.

>
> I can't find a reasonable way to build a regression test that checks whether 
> notifications are effectively deduplicated. The output of the LISTEN command 
> lists the PID of the notifying backend for each notification, e.g. : 
> 'Asynchronous notification "foobar" received from server process with PID 
> 24917'. I can't just add this to async.out. I did test manually for all eight 
> combinations : four collapse mode values (missing, empty string, 'maybe' and 
> 'never'), both with NOTIFY and pg_notify().

One way could be to take inspiration from
src/test/isolation/specs/async-notify.spec and check that
pg_notification_queue_usage() does grow when repeating the same
payload with collapse_mode='never' (while for always it would grow).
But I'm not sure it's worth the effort.

Re: Index Skip Scan

[Dmitry Dolgov]

> On Tue, 9 Oct 2018 at 18:13, Pavel Stehule  wrote:
>
>> It looks like good idea, but then the node should be named "index scan" and
>> other info can be displayed in detail parts. It can be similar like "sort".
>> The combination of unique and index skip scan looks strange :)
>
> maybe we don't need special index skip scan node - maybe possibility to
> return unique values from index scan node can be good enough - some like
> "distinct index scan" - and the implementation there can be dynamic -skip
> scan, classic index scan,
>
> "index skip scan" is not good  name if the implementaion is dynamic.

Yeah, that's a valid point. The good part is that index skip scan is not really
a separate node, but just enhanced index only scan node. So indeed maybe it
would be better to call it Index Only Scan, but show in details that we apply
the skip scan strategy. Any other opinions about this?

>> I think it was query like
>> select count(*) from (select distinct x from tab) s

Thanks, I'll take a look.

Requesting advanced Group By support

[Arun Kumar]

Hi,
  From MySQL 5.7, It supports SQL standard 99 and implements the feature 
such functional dependent on the GROUP By columns, i.e., it detects the 
non-aggregate columns which are functionally dependent on the GROUP BY columns 
(not included in GROUP BY) and then executes the query without error.
For example,

SELECT a.sno,b.sno,a.name,b.location FROM Name AS a JOIN Location AS b ON 
a.sno=b.sno GROUP BY a.sno,b.location

In this case, a.sno is a primary key so no need to include a.name in GROUP By 
as it would be identified by the primary key and then for b.sno which is again 
equated with a.sno (primary key) so no need to add this as well but for 
b.location, we need to add it in GROUP BY or we should use any aggregate 
function over this column to avoid error. For more info, please check on the 
below link https://dev.mysql.com/doc/refman/5.7/en/group-by-handling.html
Is there any plans on supporting this in Postgres in future versions ?


Thank You,
Arun Kumar

Question regarding SQL-query minimizer

[Jinho Jung]

Hello, I am Jinho. Yesterday, I found one interesting SQL statement that
actual query execution time took much longer than expected. I also check
the result is wrong from EXPLAIN ANALYZE.

Current problem is the length of SQL (14KB) when I try to analyze the root
cause. If you want to remove unnecessary part (i.e., minimize the size of
SQL unless it contains same problem), do you use any tool?

Thanks,
Jinho Jung

Re: Refactor textToQualifiedNameList()

[Alvaro Herrera]

On 2018-Oct-10, Michael Paquier wrote:

> On Tue, Oct 09, 2018 at 09:54:12AM -0300, Alvaro Herrera wrote:
> > The committer of such a change will get a lot of flak for changing the
> > #include requirements for code that calls that function, though.
> 
> So the patch has been switched to rejected...

I know -- I'm just disagreeing with that.

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

Re: View to get all the extension control file details

[Haribabu Kommi]

On Fri, Sep 21, 2018 at 5:09 PM Haribabu Kommi 
wrote:

> On Thu, Sep 20, 2018 at 3:18 PM Kyotaro HORIGUCHI <
> horiguchi.kyot...@lab.ntt.co.jp> wrote:
>
>> Hello.
>>
>> At Mon, 17 Sep 2018 16:49:41 +1000, Haribabu Kommi <
>> kommi.harib...@gmail.com> wrote in
>> 
>> > Hi Hackers,
>> >
>> > Currently PostgreSQL provides following views to get the extension
>> specific
>> > details
>> >
>> > pg_available_extensions - Name, default_version, installed_version,
>> comment
>> >
>> > pg_available_extension_versions - Name, version, installed, superuser,
>> > relocatable, schema, requires, comment
>> >
>> > But these misses the "directory", "module_pathname" and "encoding"
>> > extension specific informations and these are not available even with
>> > extension specific functions also. There are some extension that
>> differs in
>> > extension name to library name. The pgpool_recovery extension library
>> name
>> > is pgpool-recovery.so, '_' to '-'. While we are developing some tool on
>> top
>> > of PostgreSQL, we found out this problem and it can be solved easily if
>> the
>> > server expose the details that i have and got it from the extension
>> control
>> > file.
>>
>> Nowadays we are going to provide views for such files. Howerer
>> I'm not a fan of checking extension packaging using such views,
>
>
> Thanks for your opinion.
> As we are in the process of developing a tool to find out the details
> of the extensions automatically, in that case, it will be helpful if any
> view is available.
>
>
>> I
>> agree that it's good to have at least a function/view that shows
>> all available attributes of extensions. Is there no other items
>> not in controlfiles?
>>
>
> I listed all the members of the ExtensionControlFile structure. I don't
> find anything else is required.
>
>
>> > Any opinion in adding a new view like "pg_available_extension_details"
>> to
>> > display all extension control file columns? or Adding them to the
>> existing
>> > view is good?
>>
>> I felt it's a bit too noisy at first but pg_settings is doing
>> something like. So +1 to extend the existing
>> pg_available_extensions view from me from the viewpoint of
>> consistency with other views of the similar objective.
>>
>
> OK, thanks for your view. Will do accordingly.
>

Here is the patch as per the above discussion.

Regards,
Haribabu Kommi
Fujitsu Australia


0001-pg_available_extensions-update.patch
Description: Binary data

Re: [HACKERS] [PATCH] Generic type subscripting

[Pavel Stehule]

Hi

ne 30. 9. 2018 v 8:23 odesílatel Pavel Stehule 
napsal:

>
>
> ne 30. 9. 2018 v 0:21 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> napsal:
>
>> > On Fri, 20 Jul 2018 at 23:32, Dmitry Dolgov <9erthali...@gmail.com>
>> wrote:
>> >
>> > > On Thu, 26 Apr 2018 at 16:44, Dmitry Dolgov <9erthali...@gmail.com>
>> wrote:
>> > >
>> > > > On 22 March 2018 at 23:25, Dmitry Dolgov <9erthali...@gmail.com>
>> wrote:
>> > > >
>> > > > Here is the updated version of patch, rebased after recent
>> conflicts and with
>> > > > suggested documentation improvements.
>> > >
>> > > Another rebased version of the patch.
>> >
>> > I've noticed, that I never updated llvmjit code for the arrayref
>> expressions,
>> > and it's important to do so, since the patch introduces another layer of
>> > flexibility. Hence here is the new version.
>>
>> Here is another rebased version, and a bit of history: the first
>> prototypes of
>> this patch were sent more than 3 years ago. Of course the patch evolved
>> significantly over this period, and I take it as a good sign that it
>> wasn't
>> rejected and keeps moving through the commitfests. At the same time the
>> lack of
>> attention makes things a bit frustrating. I have an impression that it's
>> sort
>> of regular situation and wonder if there are any ideas (besides the well
>> known
>> advice of putting some efforts into review patches from other people,
>> since I'm
>> already doing my best and enjoying this) how to make progress in such
>> cases?
>>
>
> This feature looks nice, and it can be great when some values of some not
> atomic type should be updated.
>

I am playing with this feature little bit

I have one idea - can be possible to use integer subscript for record
fields? It can helps with iteration over record.

example:

select ('{"a":{"a":[10,20]}}'::jsonb)[0];--> NULL, but can be more
practical if it returns same like select
('{"a":{"a":[10,"20"]}}'::jsonb)['a'];

I don't like quite ignoring bad subsript in update

postgres=# insert into test(v) values( '[]');
INSERT 0 1
postgres=# update test set v[1000] = 'a';
UPDATE 1
postgres=# update test set v[1000] = 'a';
UPDATE 1
postgres=# update test set v[1000] = 'a';
UPDATE 1
postgres=# select * from test;
┌┬─┐
│ id │v│
╞╪═╡
││ ["a", "a", "a"] │
└┴─┘
(1 row)

It should to raise exception in this case. Current behave allows append
simply, but can be source of errors. For this case we can introduce some
special symbol - some like -0 :)

It is maybe strange, but I prefer less magic syntax like

update test set v['a']['a'] =  v['a']['a'] || '1000';

more readable than

update test set v['a']['a'][100] = 1000;

My first impression is very good - update jsonb, xml documents can be very
friendly.

Regards

Pavel






>
> Regards
>
> Pavel
>

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 9 Oct 2018, at 07:38, Michael Paquier  wrote:
> 
> On Fri, Oct 05, 2018 at 10:11:45AM +0200, Daniel Gustafsson wrote:
>> Thanks!  Attached is a v17 which rebases the former 0002 patch on top of
>> current master, along with the test fix for Windows that Thomas reported
>> upthread (no other changes introduced over earlier versions).
> 
> Thanks for the new version.

Thanks for reviewing!

> In order to make a test with non-ASCII characters in the error message,
> we could use a trick similar to xml.sql: use a function which ignores
> the test case if server_encoding is not UTF-8 and use something like
> chr() to pass down a messages with non-ASCII characters.  Then if the
> function catches the error we are good.

Thats one approach, do you think it’s worth adding to ensure clipping during
truncation?

> +   *pid = slot->src_pid;
> +   slot->orig_length = 0;
> +   slot->message[0] = '\0';
> Message consumption should be the part using memset(0), no?  system
> calls are avoided within a spinlock section, but memset and strlcpy
> should be fast enough.  Those are used in WalReceiverMain() for
> example.

Good point.  IIRC I added it in the setting codepath to avoid memsetting in
case there were no more messages set.  That’s an incorrect optimization, so
fixed.

> The facility to store messages should be in its own file, and
> signalfuncs.c should depend on it, something like signal_message.c?

It was originally in backend_signal.c, and was moved into (what is now)
signalfuncs.c in the refactoring that Alvaro suggested.  Re-reading his email
I’m not sure he actually proposed merging this code with the signal code.

Moved the new functionality to signal_message.{ch}.

> +typedef struct
> +{
> +   pid_t   dest_pid;   /* The pid of the process being signalled */
> +   pid_t   src_pid;/* The pid of the processing signalling */
> +   slock_t mutex;  /* Per-slot protection */
> +   charmessage[MAX_CANCEL_MSG]; /* Message to send to signalled backend 
> */
> +   int orig_length;/* Length of the message as passed by the user,
> +* if this length exceeds MAX_CANCEL_MSG it will
> +* be truncated but we store the original length
> +* in order to be able to convey truncation
> */
> +   int sqlerrcode; /* errcode to use when signalling backend */
> +} BackendSignalFeedbackShmemStruct
> 
> A couple of thoughts here:
> - More information could make this facility more generic: an elevel to
> be able to fully manipulate the custom error message, and a breakpoint
> definition.  As far as I can see you have two of them in the patch which
> are the callers of ConsumeBackendSignalFeedback().  One event cancels
> the query, and another terminates it.  In both cases, the breakpoint
> implies the elevel.  So could we have more possible breakpoints possible
> and should we try to make this API more pluggable from the start?

I’m not sure I follow, can you elaborate on this?

> - Is orig_length really useful in the data stored?  Why not just
> warning/noticing the caller defining the message and just store the
> message.

The caller is being notified, but the original length is returned such that the
consumer can format the message with the truncation in mind.  In postgres.c we
currently do:

ereport(FATAL,
(errcode(sqlerrcode),
 errmsg("%s%s",
buffer, (len > sizeof(buffer) ? "..." : "")),
 errdetail("terminating connection due to administrator command by 
process %d",
   pid)));

If that’s not deemed of value to keep, then orig_length can be dropped.

> So, looking at your patch, I am wondering also about splitting it into
> a couple of pieces for clarity:
> - The base facility to be able to register and consume messages which
> can be attached to a backend slot, and then be accessed by other things.
> Let's think about it as a base facility which is useful for extensions
> and module developers.  If coupled with a hook, it would be actually
> possible to scan a backend's slot for some information which could be
> consumed afterwards for custom error messages...
> - The set of breakpoints which can then be used to define if a given
> code path should show up a custom error message.  I can think of three
> of them: cancel, termination and extension, which is a custom path that
> extensions can use.  The extension breakpoint would make sense as
> something included from the start, could be stored as an integer and I
> guess should not be an enum but a set of defined tables (see pgstat.h
> for wait event categories for example).
> - The set of SQL functions making use of it in-core, in your case these
> are the SQL functions for cancellation and termination.

This does not sound like a bad idea to me.  Is that something you are planning
to do or do you want me to cut the patch up into pieces?  Just want to avoid

Re: IDE setup and development features?

[Vladimir Sitnikov]

Mori>Was wondering if anyone has had luck getting these three set up for
any IDE or editor configuration?

Just a data point: CLion + CMake work just great.

Step by step (just checked in macOS):
1) "Check out from Version Control" -> Git ->
https://github.com/stalkerg/postgres_cmake.git -> clone
2) CLion asks if it should open CMakeLists.txt -> yes
3) Wait a bit
4) That's it

It results in quite good IDE support for PostgreSQL code: jump to
definition, find references, great autocomplete, parameter hints (including
the ones for macros), refactoring (e.g. function rename with update of the
references).
It imports run/debug configurations from CMake as well, so one can run and
debug tests/binaries right after project import.

Vladimir

Re: IDE setup and development features?

[Amit Kapila]

On Wed, Oct 10, 2018 at 3:10 AM Mori Bellamy  wrote:
>
> Hi all,
>
> I'd like a few features when developing postgres -- (1) jump to definition of 
> symbol (2) find references to symbol and (3) semantic autocompletion.
>

If you are using a Windows environment, then I think you can get all
these things without much hassle.  I use MSVC2017 for development
purpose and all these things work by default.

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

Re: Why we allow CHECK constraint contradiction?

[Amit Langote]

On 2018/10/10 16:28, Imai, Yoshikazu wrote:
> On Tue, Oct 9, 2018 at 6:01 PM, Amit Langote wrote:
>> I had wondered about it when developing the partitioning feature about
>> a couple of years ago and this is the response I'd gotten:
>>
>> https://www.postgresql.org/message-id/CA+TgmoaQABrsLQK4ms_4NiyavyJGS
>> -b6zfkzbbnc+-p5djj...@mail.gmail.com
> 
> Thanks for tell me one of a discussion about this.
> 
>> To summarize, the answer I got was that it's pointless to create defenses
>> against it inside the database.  It's on the users to create the
>> constraints (or specify bounds) that are non-contradicting.
> 
> I just thought it's kind to tell users whether users mistakenly specify
> bounds. 
> 
> 
>> Interesting quotes from the above email:
>>
>> "If we allow partitioning on expressions, then it quickly becomes
>> altogether impossible to deduce anything useful - unless you can solve
>> the halting problem."
>>
>> "... This patch is supposed to be implementing partitioning, not
>> artificial intelligence."
> 
> It takes little more time to completely understand this interesting quotes,
> but I guess I see that point.

The task of developing a contradiction proof module that takes an
arbitrary expression and returns whether it's self-contradictory seems
daunting to me.  You may know of predtest.c in the optimizer directory as
one example of such a module, but if you look closely it's scope is fairly
limited; it works only if the input expressions contain variables of
certain types and operators that btree operator classes can handle, and
gives up on producing a proof otherwise.

On the other hand, the syntax of CHECK constraints allows a fairly wide
range of expressions to be specified, with expressions/values of arbitrary
types and operators with arbitrary semantics (not limited to
btree/ordering semantics, for example).  It won't be a good enough
solution if it can provide the error for only a certain subset of
user-specified expressions, IMHO.

Thanks,
Amit

Re: [Proposal] Add accumulated statistics for wait event

[legrand legrand]

Bertrand DROUVOT wrote
> Hello Guys,
> 
> As you mentioned Oracle like active session history sampling in this
> thread, I just want to let you know that I am working on a brand new
> extension to provide this feature.
> 
> You can find the extension here: https://github.com/pgsentinel/pgsentinel
> 
> [...]
> 
> If you want to have a look, give your thoughts, you are welcome.
> 
> Bertrand


+1!

just a regret: ash sampling interval can not be smaller than a second ;o(

Cordialement
PAscal




--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Restore CurrentUserId only if 'prevUser' is valid when abort transaction

[Richard Guo]

Hi,

This is a follow-up to the issue described in thread
https://www.postgresql.org/message-id/CAMbWs4-Mys%3DhBQSevTA8Zpd-TYFnb%3DXuHhN2TnktXMsfMUbjiQ%40mail.gmail.com

In short, during the first transaction starting phase within a backend, if
there is an 'ereport' after setting transaction state but before saving
CurrentUserId into 'prevUser' in TransactionStateData, 'prevUser' will
remain
as InvalidOid. Then in AbortTransaction(), CurrentUserId is restored with
'prevUser'. As a result, CurrentUserId will be InvalidOid in the rest of the
session.

Attached is a patch that fixes this issue.

Thanks
Richard


0001-Restore-CurrentUserId-only-if-prevUser-is-valid.patch
Description: Binary data

RE: Why we allow CHECK constraint contradiction?

[Imai, Yoshikazu]

On Tue, Oct 9, 2018 at 6:01 PM, Amit Langote wrote:
> On 2018/10/10 14:25, Imai, Yoshikazu wrote:
> > Hi, all.
> >
> > I have a wonder about the behaviour of creating table which has a
> > constraint contradiction.
> >
> > I created below table.
> >
> > bugtest=# create table ct (a int, CHECK(a is not null and a >= 0 and
> a
> > < 100 and a >= 200 and a < 300)); bugtest=# \d+ ct
> > Table "public.ct"
> >  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> >
> +-+---+--+-+-+--
> +-
> >  a  | integer |   |  | | plain   |
> |
> > Check constraints:
> > "ct_a_check" CHECK (a IS NOT NULL AND a >= 0 AND a < 100 AND a >=
> > 200 AND a < 300)
> >
> >
> > Are there any rows which can satisfy the ct's CHECK constraint? If
> > not, why we allow creating table when check constraint itself is
> contradicted?
> >
> >
> > I originally noticed this while creating partitioned range table as
> below.
> >
> > bugtest=# create table rt (a int) partition by range (a); bugtest=#
> > create table rt_sub1 partition of rt for values from (0) to (100)
> > partition by range (a); bugtest=# create table rt_sub2 partition of
> rt
> > for values from (100) to (200) partition by range (a); bugtest=#
> > create table rt150 partition of rt_sub1 for values from (150) to (151);
> bugtest=# \d+ rt_sub1
> >   Table "public.rt_sub1"
> >  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> >
> +-+---+--+-+-+--
> +-
> >  a  | integer |   |  | | plain   |
> |
> > Partition of: rt FOR VALUES FROM (0) TO (100) Partition constraint:
> > ((a IS NOT NULL) AND (a >= 0) AND (a < 100)) Partition key: RANGE (a)
> > Partitions: rt150 FOR VALUES FROM (150) TO (151)
> >
> > bugtest=# \d+ rt150
> >Table "public.rt150"
> >  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> >
> +-+---+--+-+-+--
> +-
> >  a  | integer |   |  | | plain   |
> |
> > Partition of: rt_sub1 FOR VALUES FROM (150) TO (151) Partition
> > constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 100) AND (a IS NOT
> > NULL) AND (a >= 150) AND (a < 151))
> >
> >
> > Any rows are not routed to rt150 through rt nor we can't insert any
> > rows to
> > rt150 directly because of its constraints. If we add check whether
> > constraint is contradicted, it prevent us from accidentally creating
> > useless table like above rt150 which would not contain any rows.
> >
> > I thought there might be a discussion or documentation about this, but
> > I couldn't find it. If there is, please also tell me that.
> 
> I had wondered about it when developing the partitioning feature about
> a couple of years ago and this is the response I'd gotten:
> 
> https://www.postgresql.org/message-id/CA+TgmoaQABrsLQK4ms_4NiyavyJGS
> -b6zfkzbbnc+-p5djj...@mail.gmail.com

Thanks for tell me one of a discussion about this.

> To summarize, the answer I got was that it's pointless to create defenses
> against it inside the database.  It's on the users to create the
> constraints (or specify bounds) that are non-contradicting.

I just thought it's kind to tell users whether users mistakenly specify
bounds. 


> Interesting quotes from the above email:
>
> "If we allow partitioning on expressions, then it quickly becomes
> altogether impossible to deduce anything useful - unless you can solve
> the halting problem."
> 
> "... This patch is supposed to be implementing partitioning, not
> artificial intelligence."

It takes little more time to completely understand this interesting quotes,
but I guess I see that point.


Thanks again!

--
Yoshikazu Imai

Re: IDE setup and development features?

[Mateusz Starzycki]

I have also heard god things about rtags but havent used it yet.

I have used the YCM and I must say as much as sometimes it is pain to set
up right, it is well worth it.

On Wed, Oct 10, 2018 at 2:28 AM Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:

> From: Mori Bellamy [mailto:m...@invoked.net]
> > I'd like a few features when developing postgres -- (1) jump to
> definition
> > of symbol (2) find references to symbol and (3) semantic autocompletion.
>
> For 1), you can generate tags like:
>
> [for vi]
> $ src/tools/make_ctags
> [for Emacs]
> $ src/tools/make_etags
>
> Cscope works for 2).
>
>
> Regards
> Takayuki Tsunakawa
>
>

RE: Why we allow CHECK constraint contradiction?

[Imai, Yoshikazu]

Thanks for replying!

On Tue, Oct 9, 2018 at 5:58 PM, Corey Huinker wrote:
> On Wed, Oct 10, 2018 at 1:44 AM David G. Johnston
> mailto:david.g.johns...@gmail.com> >
> wrote:
> 
> 
>   On Tuesday, October 9, 2018, Imai, Yoshikazu
> mailto:imai.yoshik...@jp.fujitsu.com>
> > wrote:
> 
>   Are there any rows which can satisfy the ct's CHECK
> constraint? If not, why we
>   allow creating table when check constraint itself is
> contradicted?
> 
> 
> 
>   I'd bet on it being a combination of complexity and insufficient
> expected benefit.  Time is better spent elsewhere.  Mathmatically
> proving a contradiction in software is harder than reasoning about it
> mentally.
> 
> 
> I've actually used that as a feature, in postgresql and other databases,
> where assertions were unavailable, or procedural code was unavailable
> or against policy.
> 
> Consider the following:
> 
> 
>   CREATE TABLE wanted_values ( x integer );
> 
>   INSERT INTO wanted_values VALUES (1), (2), (3);
> 
> 
> 
> 
>   CREATE TABLE found_values ( x integer );
> 
>   INSERT INTO found_values VALUES (1), (3);
> 
> 
> 
> 
>   CREATE TABLE missing_values (
> 
>   x integer,
> 
>   CONSTRAINT contradiction CHECK (false)
> 
>   );
> 
> 
> 
> 
>   INSERT INTO missing_values
> 
>   SELECT x FROM wanted_values
> 
>   EXCEPT
> 
>   SELECT x FROM found_values;
> 
> 
> 
> 
> gives the error
> 
> 
>   ERROR:  new row for relation "missing_values" violates check
> constraint "contradiction"
> 
>   DETAIL:  Failing row contains (2).
> 
> 
> Which can be handy when you need to fail a transaction because of bad
> data and don't have branching logic available.

That's an interesting using! So, there are useful case of constraint
contradiction table not only for time shortage/difficulties of
implementing mathematically proving a contradiction.

--
Yoshikazu Imai

Re: Why we allow CHECK constraint contradiction?

[Amit Langote]

On 2018/10/10 14:25, Imai, Yoshikazu wrote:
> Hi, all.
> 
> I have a wonder about the behaviour of creating table which has a constraint
> contradiction.
> 
> I created below table.
> 
> bugtest=# create table ct (a int, CHECK(a is not null and a >= 0 and a < 100 
> and a >= 200 and a < 300));
> bugtest=# \d+ ct
> Table "public.ct"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats target | 
> Description 
> +-+---+--+-+-+--+-
>  a  | integer |   |  | | plain   |  | 
> Check constraints:
> "ct_a_check" CHECK (a IS NOT NULL AND a >= 0 AND a < 100 AND a >= 200 AND 
> a < 300)
> 
> 
> Are there any rows which can satisfy the ct's CHECK constraint? If not, why we
> allow creating table when check constraint itself is contradicted?
> 
> 
> I originally noticed this while creating partitioned range table as below.
> 
> bugtest=# create table rt (a int) partition by range (a);
> bugtest=# create table rt_sub1 partition of rt for values from (0) to (100) 
> partition by range (a);
> bugtest=# create table rt_sub2 partition of rt for values from (100) to (200) 
> partition by range (a);
> bugtest=# create table rt150 partition of rt_sub1 for values from (150) to 
> (151);
> bugtest=# \d+ rt_sub1
>   Table "public.rt_sub1"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats target | 
> Description 
> +-+---+--+-+-+--+-
>  a  | integer |   |  | | plain   |  | 
> Partition of: rt FOR VALUES FROM (0) TO (100)
> Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 100))
> Partition key: RANGE (a)
> Partitions: rt150 FOR VALUES FROM (150) TO (151)
> 
> bugtest=# \d+ rt150
>Table "public.rt150"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats target | 
> Description 
> +-+---+--+-+-+--+-
>  a  | integer |   |  | | plain   |  | 
> Partition of: rt_sub1 FOR VALUES FROM (150) TO (151)
> Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 100) AND (a IS 
> NOT NULL) AND (a >= 150) AND (a < 151))
> 
> 
> Any rows are not routed to rt150 through rt nor we can't insert any rows to 
> rt150 directly because of its constraints. If we add check whether constraint
> is contradicted, it prevent us from accidentally creating useless table like 
> above rt150 which would not contain any rows.
> 
> I thought there might be a discussion or documentation about this, but I 
> couldn't find it. If there is, please also tell me that.

I had wondered about it when developing the partitioning feature about a
couple of years ago and this is the response I'd gotten:

https://www.postgresql.org/message-id/ca+tgmoaqabrslqk4ms_4niyavyjgs-b6zfkzbbnc+-p5djj...@mail.gmail.com

To summarize, the answer I got was that it's pointless to create defenses
against it inside the database.  It's on the users to create the
constraints (or specify bounds) that are non-contradicting.  Interesting
quotes from the above email:

"If we allow partitioning on expressions, then it quickly becomes
altogether impossible to deduce anything useful - unless you can solve the
halting problem."

"... This patch is supposed to be implementing partitioning, not
artificial intelligence."

:)

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/ca+tgmoaqabrslqk4ms_4niyavyjgs-b6zfkzbbnc+-p5djj...@mail.gmail.com