Re: procedures and plpgsql PERFORM

[Pavel Stehule]

2017-12-14 8:21 GMT+01:00 Ashutosh Bapat :

> Hi,
> We allow a function to be invoked as part of PERFORM statement in plpgsql
> do $$
> begin perform pg_relation_size('t1'); end; $$ language plpgsql;
> DO
>
> But we do not allow a procedure to be invoked this way
>  create procedure dummy_proc(a int) as $$
> begin null; end;
> $$ language plpgsql;
> CREATE PROCEDURE
>
> do $$
> begin perform dummy_proc(1); end; $$ language plpgsql;
> ERROR:  dummy_proc(integer) is a procedure
> LINE 1: SELECT dummy_proc(1)
>^
> HINT:  To call a procedure, use CALL.
> QUERY:  SELECT dummy_proc(1)
> CONTEXT:  PL/pgSQL function inline_code_block line 2 at PERFORM
>
> The documentation of PERFORM [1] says
> "For any SQL command that does not return rows, for example INSERT
> without a RETURNING clause, you can execute the command within a
> PL/pgSQL function just by writing the command."
>
> Procedures fit that category and like functions, I think, we should
> allow them be invoked directly without any quoting and CALL
> decoration.
>

Why? The CALL is four chars more. It is keyword, and it reduce parser
complexity - we should not to different between routine name and variable
name.

So -1 from my for this proposal.

Regards

Pavel

>
> [1] https://www.postgresql.org/docs/devel/static/plpgsql-
> statements.html#PLPGSQL-STATEMENTS-SQL-NORESULT
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
>

Re: [HACKERS] Parallel Hash take II

[Thomas Munro]

On Thu, Dec 14, 2017 at 11:45 AM, Andres Freund  wrote:
> +   booloverflow;   /* Continuation of previous 
> chunk? */
> +   chardata[FLEXIBLE_ARRAY_MEMBER];
> +} SharedTuplestoreChunk;
>
> Ah. I was thinking we could have the 'overflow' variable be an int,
> indicating the remaining length of the oversized tuple. That'd allow us
> to skip ahead to the end of the oversized tuple in concurrent processes
> after hitting it.

Right, that is a bit better as it avoids extra read-skip cycles for
multi-overflow-chunk cases.  Done that way.

> +   if (accessor->write_pointer + 
> accessor->sts->meta_data_size >=
> +   accessor->write_end)
> +   elog(ERROR, "meta-data too long");
>
> That seems more like an Assert than a proper elog? Given that we're
> calculating size just a few lines above...

It's an error because the logic is not smart enough to split the
optional meta-data and tuple size over multiple chunks.  I have added
comments there to explain.  That error can be reached by CALL
test_sharedtuplestore(1, 1, 1, 32756, 1), but 32755 is OK.  My goal
here is to support arbitrarily large tuples, not arbitrarily large
per-tuple meta-data, since for my use case I only need 4 bytes (a hash
value).  This could be improved if required by later features
(probably anyone wanting more general meta-data would want variable
sized meta-data anyway, whereas this is fixed, and it would also be
nice if oversized tuples didn't have to start at the beginning of a
new chunk).

I fixed two nearby fencepost bugs: I made the limit that triggers that
error smaller by size(uint32) and fixed a problem when small tuples
appear after an oversize tuple in a final overflow chunk (found by
hacking the test module to create mixtures of different sized tuples).

> +   if (accessor->sts->meta_data_size > 0)
> +   memcpy(accessor->write_pointer, meta_data,
> +  accessor->sts->meta_data_size);
> +   written = accessor->write_end - 
> accessor->write_pointer -
> +   accessor->sts->meta_data_size;
> +   memcpy(accessor->write_pointer + 
> accessor->sts->meta_data_size,
> +  tuple, written);
>
> Also, shouldn't the same Assert() be here as well if you have it above?

No, when it comes to the tuple we just write as much of it as will
fit, and write the rest in the loop below.  Comments improved to make
that clear.

> +   ereport(ERROR,
> +   (errcode_for_file_access(),
> +errmsg("could not read from shared 
> tuplestore temporary file"),
> +errdetail("Short read while reading 
> meta-data")));
>
> The errdetail doesn't follow the style guide (not a sentence ending with
> .), and seems internal-ish. I'm ok with keeping it, but perhaps we
> should change all these to be errdetail_internal()? Seems pointless to
> translate all of them.

Done.

> +   LWLockAcquire(&p->lock, LW_EXCLUSIVE);
> +   eof = p->read_page >= p->npages;
> +   if (!eof)
> +   {
> +   read_page = p->read_page;
> +   p->read_page += STS_CHUNK_PAGES;
> +   }
> +   LWLockRelease(&p->lock);
>
> So if we went to the world I'm suggesting, with overflow containing the
> length till the end of the tuple, this'd probably would have to look a
> bit different.

Yeah.  I almost wanted to change it back to a spinlock but now it's
grown bigger again...

> +   if (!eof)
> +   {
> +   SharedTuplestoreChunk chunk_header;
> +
> +   /* Make sure we have the file open. */
> +   if (accessor->read_file == NULL)
> +   {
> +   charname[MAXPGPATH];
> +
> +   sts_filename(name, accessor, 
> accessor->read_participant);
> +   accessor->read_file =
> +   BufFileOpenShared(accessor->fileset, 
> name);
> +   if (accessor->read_file == NULL)
> +   elog(ERROR, "could not open temporary 
> file %s", name);
>
> Isn't this more an Assert or just not anything? There's now way
> BufFileOpenShared should ever return NULL, no?

Right.  As of commit 923e8dee this can no longer return NULL (instead
it would raise an error), so I removed this redundant check.

> +   /* If this is an overflow chunk, we skip it. */
> +   if (chunk_header.overflow)
> +   continue;
> +
> +   accessor->read_ntuples 

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Michael Paquier]

On Thu, Dec 14, 2017 at 2:49 PM, Justin Pryzby  wrote:
> On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
>> 2017-12-13 15:37 GMT+07:00 Amit Langote :
>> Patch for adding check in pg_upgrade. Going through git history, the check
>> for inconsistency in NOT NULL constraint has ben there since a long time
>> ago. In this patch the check will be applied for all old cluster version.
>> I'm not sure in which version was the release of table inheritance.
>
> Here are some spelling and grammar fixes to that patch:
>
> but nullabe in its children: nullable
> child column is not market: marked
> with adding: by adding
> and restart: and restarting
> the problem columns: the problematic columns
> 9.5, 9.6, 10: 9.5, 9.6, and 10
> restore, that will cause error.: restore phase of pg_upgrade, that would 
> cause an error.

I agree that we should do better in pg_upgrade for HEAD and
REL_10_STABLE as it is not cool to fail late in the upgrade process
especially if you are using the --link mode.

+ *  Currently pg_upgrade will fail if there are any inconsistencies in NOT NULL
+ *  constraint inheritance: In Postgres version 9.5, 9.6, 10 we can
have a column
+ *  that is NOT NULL in parent, but nullabe in its children. But during schema
+ *  restore, that will cause error.
On top of the multiple typos here, there is no need to mention
anything other than "PostgreSQL 9.6 and older versions did not add NOT
NULL constraints when a primary key was added to to a parent, so check
for inconsistent NOT NULL constraints in inheritance trees".

You seem to take a path similar to what is done with the jsonb check.
Why not. I would find cleaner to tell also the user about using ALTER
TABLE to add the proper constraints.

Note that I have not checked or tested the query in details, but
reading through the thing looks basically correct as it handles
inheritance chains with WITH RECURSIVE.

@@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
 check_for_prepared_transactions(&old_cluster);
 check_for_reg_data_type_usage(&old_cluster);
 check_for_isn_and_int8_passing_mismatch(&old_cluster);
+check_for_not_null_inheritance(&old_cluster);
The check needs indeed to happen in check_and_dump_old_cluster(), but
there is no point to check for it in servers with version 10 and
upwards, hence you should make it conditional with GET_MAJOR_VERSION()
<= 906.
-- 
Michael

Re: pgsql: Provide overflow safe integer math inline functions.

[Christoph Berg]

Re: Andres Freund 2017-12-13 <20171213173524.rjs7b3ahsong5...@alap3.anarazel.de>
> On 2017-12-13 09:37:25 -0500, Robert Haas wrote:
> > On Wed, Dec 13, 2017 at 5:10 AM, Christoph Berg  wrote:
> > > Re: Andres Freund 2017-12-13 
> > >> Provide overflow safe integer math inline functions.
> > >
> > > The new _overflow functions have buggy comments:
> > >
> > > /*
> > >  * If a - b overflows, return true, otherwise store the result of a + b 
> > > into
> > >  * *result. ^ 
> > > there
> > 
> > What's wrong with that?
> 
> After staring at it for a while, I seem to have partially mis-copied the
> note for addition to the subtraction operation...

I believe the attached is the correct version.

Christoph
diff --git a/src/include/common/int.h b/src/include/common/int.h
new file mode 100644
index e44d42f..e6907c6
*** a/src/include/common/int.h
--- b/src/include/common/int.h
*** pg_add_s16_overflow(int16 a, int16 b, in
*** 41,47 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 41,47 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a - b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_sub_s16_overflow(int16 a, int16 b, in
*** 61,67 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 61,67 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a * b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_add_s32_overflow(int32 a, int32 b, in
*** 101,107 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 101,107 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a - b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_sub_s32_overflow(int32 a, int32 b, in
*** 121,127 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 121,127 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a * b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_add_s64_overflow(int64 a, int64 b, in
*** 167,173 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 167,173 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a - b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_sub_s64_overflow(int64 a, int64 b, in
*** 193,199 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 193,199 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a * b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */

Re: [HACKERS] Parallel Hash take II

[Andres Freund]

Hi,

Looking at the main patch (v28).

First off: This looks pretty good, the code's quite readable now
(especially compared to earlier versions), the comments are good. Really
like the nodeHash split, and the always inline hackery in nodeHashjoin.
Think we're getting really really close.

  * ExecHashJoinImpl
  *
- * This function implements the Hybrid Hashjoin algorithm.
+ * This function implements the Hybrid Hashjoin algorithm.  By forcing it
+ * to be always inline many compilers are able to specialize it for
+ * parallel = true/false without repeating the code.
  *

what about adding the above explanation for the always inline?


+   /*
+* So far we have no idea whether there are any other 
participants,
+* and if so, what phase they are working on.  The only thing 
we care
+* about at this point is whether someone has already created 
the
+* SharedHashJoinBatch objects, the main hash table for batch 0 
and
+* (if necessary) the skew hash table yet.  One backend will be
+* elected to do that now if necessary.
+*/

The 'yet' sounds a bit weird in combination with the 'already'.


+ static void
+ ExecParallelHashIncreaseNumBatches(HashJoinTable hashtable)
+ ...
+   case PHJ_GROW_BATCHES_ELECTING:
+   /* Elect one participant to prepare the operation. */

That's a good chunk of code. I'm ever so slightly inclined to put that
into a separate function. But I'm not sure it'd look good. Feel entirely
free to disregard.


+ static HashJoinTuple
+ ExecParallelHashLoadTuple(HashJoinTable hashtable, MinimalTuple tuple,
+ dsa_pointer *shared)

Not really happy with the name. ExecParallelHashTableInsert() calling
ExecParallelHashLoadTuple() to insert a tuple into the hashtable doesn't
quite strike me as right; the naming similarity to
ExecParallelHashTableLoad seems problematic too.
ExecParallelHashAllocTuple() or such?

One could argue it'd not be a bad idea to keep a similar split as
dense_alloc() and memcpy() have, but I'm not really convinced by
myself. Hm.


+   case PHJ_GROW_BATCHES_ELECTING:
+   /* Elect one participant to prepare the operation. */

Imo that comment could use a one-line summary of what preparing means.


+   /*
+* We probably also need a smaller 
bucket array.  How many
+* tuples do we expect per batch, 
assuming we have only
+* half of them so far?

Makes sense, but did cost me a minute of thinking. Maybe add a short
explanation why.


+   case PHJ_GROW_BATCHES_ALLOCATING:
+   /* Wait for the above to be finished. */
+   BarrierArriveAndWait(&pstate->grow_batches_barrier,
+
WAIT_EVENT_HASH_GROW_BATCHES_ALLOCATING);
+   /* Fall through. */

Just to make sure I understand: The "empty" phase is to protect the
process of the electee doing the sizing calculations etc?  And the
reason that's not possible to do just by waiting for
PHJ_GROW_BATCHES_REPARTITIONING is that somebody could dynamically
arrive, i.e. it'd not be needed in a statically sized barrier?  Pretty
tired here, sorry ;)


+   /* reset temp memory each time to avoid leaks from qual 
expr */
+   ResetExprContext(econtext);
+
+   if (ExecQual(hjclauses, econtext))

I personally think it's better to avoid this pattern and store the
result of the ExecQual() in a variable, ResetExprContext() and then act
on the result.  No need to keep memory around for longer, and for bigger
contexts you're more likely to have all the metadata in cache.

I'd previously thought about introducing ExecQualAndReset() or such...


  * IDENTIFICATION
  *   src/backend/executor/nodeHashjoin.c
  *
+ * PARALLELISM
+ *

This is a pretty good explanation. How about adding a reference to it
from nodeHash.c's header?



+static TupleTableSlot */* return: a tuple or NULL */
+ExecHashJoin(PlanState *pstate)
+{
+   /*
+* On sufficiently smart compilers this should be inlined with the
+* parallel-aware branches removed.
+*/
+   return ExecHashJoinImpl(pstate, false);

Ah, the explanation I desired above is here. Still seems good to have a
comment at the somewhat suspicious use of always_inline.


+
+   /*
+* If we started up so late that the shared batches have been freed
+* already by ExecHashTableDetach(), then we are finished.
+*/
+   if (!DsaPointerIsValid(hashtable->parallel_state->batches))
+   return false;

This is really the only place that weird condition is 

Re: procedures and plpgsql PERFORM

[Merlin Moncure]

On Thursday, December 14, 2017, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> Hi,
> We allow a function to be invoked as part of PERFORM statement in plpgsql
> do $$
> begin perform pg_relation_size('t1'); end; $$ language plpgsql;
> DO
>
> But we do not allow a procedure to be invoked this way
>  create procedure dummy_proc(a int) as $$
> begin null; end;
> $$ language plpgsql;
> CREATE PROCEDURE
>
> do $$
> begin perform dummy_proc(1); end; $$ language plpgsql;
> ERROR:  dummy_proc(integer) is a procedure
> LINE 1: SELECT dummy_proc(1)
>^
> HINT:  To call a procedure, use CALL.
> QUERY:  SELECT dummy_proc(1)
> CONTEXT:  PL/pgSQL function inline_code_block line 2 at PERFORM
>
> The documentation of PERFORM [1] says
> "For any SQL command that does not return rows, for example INSERT
> without a RETURNING clause, you can execute the command within a
> PL/pgSQL function just by writing the command."
>
> Procedures fit that category and like functions, I think, we should
> allow them be invoked directly without any quoting and CALL
> decoration.
>

I disagree.  The SQL command is 'CALL'.  The documentation is really only
clarifying when PERFORM is explicitly required.

merlin

Re: pgbench's expression parsing & negative numbers

[Fabien COELHO]

Hello Andres,


There are some overflow checking with div and double to int cast, which were
added because of previous complaints, but which are not very useful to me.


I think handling it inconsistently is the worst of all worlds.


Hmmm... I cannot say that inconsistency is a good thing, that would not 
be consistent:-)


My 0.02€:

 - I do not think that updating pgbench arithmetic for managing integer
   overflows is worth Andres Freund time. My guess is that most
   script would not trigger client-side overflows, so the change would
   be a no-op in practice.

 - I think that pgbench has more important defects/missing features, to
   be fixed more urgently. Given the time patches spend in the cf queue,
   obviously committers disagree on this one:-)

Now ISTM that it does not harm anything to add such a feature, so fine 
with me. Maybe the global compiler option removal is worth the effort.


--
Fabien.

Re: [HACKERS] Partition-wise aggregation/grouping

[Ashutosh Bapat]

On Wed, Dec 13, 2017 at 6:37 PM, Jeevan Chalke
 wrote:
>
>
> On Tue, Dec 12, 2017 at 3:43 PM, Ashutosh Bapat
>  wrote:
>>
>> Here are review comments for 0009
>
>
> Thank you, Ashutosh for the detailed review so far.
>
> I am working on your reviews but since parallel Append is now committed,
> I need to re-base my changes over it and need to resolve the conflicts too.
>
> Once done, I will submit the new patch-set fixing these and earlier review
> comments.
>

Sure no problem. Take your time. Here's set of comments for 0008. That
ends the first read of all the patches (2nd reading for the core
changes)

+-- Also, disable parallel paths.
+SET max_parallel_workers_per_gather TO 0;

If you enable parallel aggregation for smaller data partition-wise aggregation
paths won't be chosen. I think this is the reason why you are disabling
parallel query. But we should probably explain that in a comment. Better if we
could come up testcases without disabling parallel query. Since parallel append
is now committed, may be it can help.

+
+-- Check with multiple columns in GROUP BY, order in target-list is reversed
+EXPLAIN (COSTS OFF)
+SELECT c, a, count(*) FROM pagg_tab GROUP BY a, c;
+   QUERY PLAN
+-
+ Append
+   ->  HashAggregate
+ Group Key: pagg_tab_p1.a, pagg_tab_p1.c
+ ->  Seq Scan on pagg_tab_p1
[ ... clipped ... ]
+(10 rows)

Why do we need this testcase?

+
+-- Test when input relation for grouping is dummy
+EXPLAIN (COSTS OFF)
+SELECT c, sum(a) FROM pagg_tab WHERE 1 = 2 GROUP BY c;
+   QUERY PLAN
+
+ HashAggregate
+   Group Key: pagg_tab.c
+   ->  Result
+ One-Time Filter: false
+(4 rows)

Not part of your patch, I am wondering if we can further optimize this plan by
converting HashAggregate to Result (One-time Filter: false) and the aggregate
target. Just an idea.

+
+SELECT c, sum(a) FROM pagg_tab WHERE 1 = 2 GROUP BY c;
+ c | sum
+---+-
+(0 rows)

I think we also need a case when the child input relations are marked dummy and
then the parent is marked dummy. Just use a condition with partkey = .

+
+-- Check with SORTED paths. Disable hashagg to get group aggregate

Suggest: "Test GroupAggregate paths by disabling hash aggregates."

+-- When GROUP BY clause matches with PARTITION KEY.

I don't think we need "with", and just extend the same sentence with "complete
aggregation is performed for each partition"

+-- Should choose full partition-wise aggregation path

suggest: "Should choose full partition-wise GroupAggregate plan", but I guess
with the above suggestion, this sentence is not needed.

+
+-- When GROUP BY clause not matches with PARTITION KEY.
+-- Should choose partial partition-wise aggregation path

Similar suggestions as above.

+-- No aggregates, but still able to perform partition-wise aggregates

That's a funny construction. May be "Test partition-wise grouping without any
aggregates".

We should try some output for this query.

+
+EXPLAIN (COSTS OFF)
+SELECT a FROM pagg_tab GROUP BY a ORDER BY 1;
+   QUERY PLAN
+-
+ Group
+   Group Key: pagg_tab_p1.a
+   ->  Merge Append
+ Sort Key: pagg_tab_p1.a
+ ->  Group
+   Group Key: pagg_tab_p1.a
+   ->  Sort
+ Sort Key: pagg_tab_p1.a
+ ->  Seq Scan on pagg_tab_p1
[ ... clipped ... ]
+(19 rows)

It's strange that we do not annotate partial grouping as Partial. Does not look
like a bug in your patch. Do we get similar output with parallel grouping?

+
+-- ORDERED SET within aggregate
+EXPLAIN (COSTS OFF)
+SELECT a, sum(b order by a) FROM pagg_tab GROUP BY a ORDER BY 1, 2;
+   QUERY PLAN
+
+ Sort
+   Sort Key: pagg_tab_p1.a, (sum(pagg_tab_p1.b ORDER BY pagg_tab_p1.a))
+   ->  GroupAggregate
+ Group Key: pagg_tab_p1.a
+ ->  Sort
+   Sort Key: pagg_tab_p1.a
+   ->  Append
+ ->  Seq Scan on pagg_tab_p1
+ ->  Seq Scan on pagg_tab_p2
+ ->  Seq Scan on pagg_tab_p3
+(10 rows)

pagg_tab is partitioned by column c. So, not having it in GROUP BY
itself might produce this plan if Partial parallel aggregation is expensive.
When testing negative tests like this GROUP BY should always have the partition
key.

In case of full aggregation, since all the rows that belong to the same group
come from the same partition, having an ORDER BY doesn't make any difference.
We should support such a case.

+INSERT INTO pagg_tab1 SELECT i%30, i%20 FROM generate_series(0, 299, 2) i;
+INSERT INTO pagg_tab2 SELECT i%20, i%30 FROM generate_series(0, 299, 3) i;

spaces around % operator?

+-- When GROUP BY clause matches with PARTITION KEY.
+-- Should choose full partition-wise aggregation path.

Probably we should

Re: CUBE seems a bit confused about ORDER BY

[Teodor Sigaev]

Yes.  I bet only few users have built indexes over ~> operator if any.
Ask them to reindex in the release notes seems OK for me.



Is there a good way to detect such cases? Either in pg_upgrade, so that
we can print warnings, or at least manually (which would be suitable for
release notes).


Hmm, suppose, fix should be backpatched (because now it's unusable) and 
pg_upgrade  should not do anything. Just add release note to 10.0 and 11.0


Oh, check expression is affected too, users will need to reinsert data.

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Masahiko Sawada]

On Wed, Dec 13, 2017 at 5:57 PM, Masahiko Sawada  wrote:
> On Wed, Dec 13, 2017 at 4:30 PM, Andres Freund  wrote:
>> On 2017-12-13 16:02:45 +0900, Masahiko Sawada wrote:
>>> When we add extra blocks on a relation do we access to the disk? I
>>> guess we just call lseek and write and don't access to the disk. If so
>>> the performance degradation regression might not be much.
>>
>> Usually changes in the file size require the filesystem to perform
>> metadata operations, which in turn requires journaling on most
>> FSs. Which'll often result in synchronous disk writes.
>>
>
> Thank you. I understood the reason why this measurement should use two
> different filesystems.
>

Here is the result.
I've measured the through-put with some cases on my virtual machine.
Each client loads 48k file to each different relations located on
either xfs filesystem or ext4 filesystem, for 30 sec.

Case 1: COPYs to relations on different filessystems(xfs and ext4) and
N_RELEXTLOCK_ENTS is 1024

clients = 2, avg = 296.2068
clients = 5, avg = 372.0707
clients = 10, avg = 389.8850
clients = 50, avg = 428.8050

Case 2: COPYs to relations on different filessystems(xfs and ext4) and
N_RELEXTLOCK_ENTS is 1

clients = 2, avg = 294.3633
clients = 5, avg = 358.9364
clients = 10, avg = 383.6945
clients = 50, avg = 424.3687

And the result of current HEAD is following.

clients = 2, avg = 284.9976
clients = 5, avg = 356.1726
clients = 10, avg = 375.9856
clients = 50, avg = 429.5745

In case2, the through-put got decreased compare to case 1 but it seems
to be almost same as current HEAD. Because the speed of acquiring and
releasing extension lock got x10 faster than current HEAD as I
mentioned before, the performance degradation may not have gotten
decreased than I expected even in case 2.
Since my machine doesn't have enough resources the result of clients =
50 might not be a valid result.

Regards,

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

Re: CUBE seems a bit confused about ORDER BY

[Alexander Korotkov]

On Thu, Dec 14, 2017 at 1:36 PM, Teodor Sigaev  wrote:

> Yes.  I bet only few users have built indexes over ~> operator if any.
>>> Ask them to reindex in the release notes seems OK for me.
>>>
>>>
>> Is there a good way to detect such cases? Either in pg_upgrade, so that
>> we can print warnings, or at least manually (which would be suitable for
>> release notes).
>>
>
> Hmm, suppose, fix should be backpatched (because now it's unusable) and
> pg_upgrade  should not do anything. Just add release note to 10.0 and 11.0
>
> Oh, check expression is affected too, users will need to reinsert data.


I wrote query to find both constraints and indexes depending on ~> cube
operator.

SELECT dep.classid::regclass AS class,
  CASE WHEN dep.classid = 'pg_catalog.pg_class'::regclass THEN
dep.objid::regclass::text
   WHEN dep.classid = 'pg_catalog.pg_constraint'::regclass THEN (SELECT
conname FROM pg_catalog.pg_constraint WHERE oid = dep.objid)
  ELSE NULL
  END AS name
FROM
  pg_catalog.pg_extension e
  JOIN pg_catalog.pg_depend edep ON edep.refclassid =
'pg_catalog.pg_extension'::regclass AND edep.refobjid = e.oid AND deptype =
'e' AND edep.classid = 'pg_catalog.pg_operator'::regclass
  JOIN pg_catalog.pg_operator o ON o.oid = edep.objid AND o.oprname = '~>'
  JOIN pg_catalog.pg_depend dep ON dep.refclassid =
'pg_catalog.pg_operator'::regclass AND dep.refobjid = o.oid
WHERE
  e.extname = 'cube' AND dep.classid IN
('pg_catalog.pg_constraint'::regclass, 'pg_catalog.pg_class'::regclass);

On the below data schema

create table tmp (c cube, check ((c ~> 0 > 0)));
create index tmp_idx on tmp ((c~>0));

it gives following result

 class |name
---+-
 pg_class  | tmp_idx
 pg_constraint | tmp_c_check
(2 rows)

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] [WIP] Zipfian distribution in pgbench

[Teodor Sigaev]

Thank you for all, pushed

Fabien COELHO wrote:


Note that the patch may interact with other patches which add functions to 
pgbench, so might need a rebase depending on the order in which the patch are 
applied.


Attached a minor rebase after 16827d4424.



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: CUBE seems a bit confused about ORDER BY

[Teodor Sigaev]

SELECT dep.classid::regclass AS class,
   CASE WHEN dep.classid = 'pg_catalog.pg_class'::regclass THEN 
dep.objid::regclass::text
        WHEN dep.classid = 'pg_catalog.pg_constraint'::regclass THEN (SELECT 
conname FROM pg_catalog.pg_constraint WHERE oid = dep.objid)

   ELSE NULL
   END AS name
FROM
   pg_catalog.pg_extension e
   JOIN pg_catalog.pg_depend edep ON edep.refclassid = 
'pg_catalog.pg_extension'::regclass AND edep.refobjid = e.oid AND deptype = 'e' 
AND edep.classid = 'pg_catalog.pg_operator'::regclass

   JOIN pg_catalog.pg_operator o ON o.oid = edep.objid AND o.oprname = '~>'
   JOIN pg_catalog.pg_depend dep ON dep.refclassid = 
'pg_catalog.pg_operator'::regclass AND dep.refobjid = o.oid

WHERE
   e.extname = 'cube' AND dep.classid IN ('pg_catalog.pg_constraint'::regclass, 
'pg_catalog.pg_class'::regclass);


On the below data schema

create table tmp (c cube, check ((c ~> 0 > 0)));
create index tmp_idx on tmp ((c~>0));

it gives following result

      class     |    name
---+-
  pg_class      | tmp_idx
  pg_constraint | tmp_c_check
(2 rows)


SQL-query seems too huge for release notes and isn't looking for materialized 
view (fixable) and functional indexes with function which contains this operator 
somewhere inside (not fixable by this query). I think, just words is enough.


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: [HACKERS] Surjective functional indexes

[Konstantin Knizhnik]

On 13.12.2017 14:29, Simon Riggs wrote:

On 4 December 2017 at 15:35, Konstantin Knizhnik
 wrote:

On 30.11.2017 05:02, Michael Paquier wrote:

On Wed, Sep 27, 2017 at 4:07 PM, Simon Riggs 
wrote:

On 15 September 2017 at 16:34, Konstantin Knizhnik
 wrote:


Attached please find yet another version of the patch.

Thanks. I'm reviewing it.

Two months later, this patch is still waiting for a review (you are
listed as well as a reviewer of this patch). The documentation of the
patch has conflicts, please provide a rebased version. I am moving
this patch to next CF with waiting on author as status.

Attached please find new patch with resolved documentation conflict.

I’ve not posted a review because it was my hope that I could fix up
the problems with this clever and useful patch and just commit it. I
spent 2 days trying to do that but some problems remain and I’ve run
out of time.

Patch 3 has got some additional features that on balance I don’t think
we need. Patch 1 didn’t actually work which was confusing when I tried
to go back to that.

Patch 3 Issues

* Auto tuning added 2 extra items to Stats for each relation.  That is
too high a price to pay, so we should remove that. If we can’t do
autotuning without that, please remove it.

* Patch needs a proper test case added. We shouldn’t just have a
DEBUG1 statement in there for testing - very ugly. Please rewrite
tests using functions that show how many changes have occurred during
the current  statement/transaction.

* Parameter coding was specific to that section of code. We need a
capability to parse that parameter from other locations, just as we do
with all other reloptions. It looks like it was coded that way because
of difficulties with code placement. Please solve this with generic
code, not just code that solves only the current issue. I’d personally
be happier without any option at all, but if y’all want it, then
please add the code in the right place.

* The new coding for heap_update() uses the phrase “warm”, which is
already taken by another patch. Please avoid confusing things by
re-using the same term for different things.
The use of these technical terms like projection and surjective
doesn’t seem to add anything useful to the patch

* Rename parameter to recheck_on_update

* Remove random whitespace changes in the patch

So at this stage the concept is good and works, but the patch is only
just at the prototype stage and nowhere near committable. I hope you
can correct these issues and if you do I will be happy to review and
perhaps commit.

Thanks



Attached please find new patch which fix all the reported issues except 
disabling autotune.
If you still thing that additional 16 bytes per relation in statistic is 
too high overhead, then I will also remove autotune.



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

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

 

diff --git a/src/backend/ac

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Fixed in the attached patch.


v7 needs a rebase.

Also, you might try to produce a version which is compatible with 
Robert's constraints.


--
Fabien.

Re: [HACKERS] pgbench more operators & functions

[Fabien COELHO]

Attached v16 fixes those two errors. I regenerated the documentation with the 
new xml toolchain, and made "check" overall and in pgbench.


Attached v17 is a rebase after the zipfian commit.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3..ea8f305 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -904,14 +904,32 @@ pgbench  options  d
  
   Sets variable varname to a value calculated
   from expression.
-  The expression may contain integer constants such as 5432,
+  The expression may contain the NULL constant,
+  boolean constants TRUE and FALSE,
+  integer constants such as 5432,
   double constants such as 3.14159,
   references to variables :variablename,
-  unary operators (+, -) and binary operators
-  (+, -, *, /,
-  %) with their usual precedence and associativity,
-  function calls, and
-  parentheses.
+  operators
+  with their usual SQL precedence and associativity,
+  function calls,
+  SQL CASE generic conditional
+  expressions and parentheses.
+ 
+
+ 
+  Functions and most operators return NULL on
+  NULL input.
+ 
+
+ 
+  For conditional purposes, non zero numerical values are
+  TRUE, zero numerical values and NULL
+  are FALSE.
+ 
+
+ 
+  When no final ELSE clause is provided to a
+  CASE, the default value is NULL.
  
 
  
@@ -920,6 +938,7 @@ pgbench  options  d
 \set ntellers 10 * :scale
 \set aid (1021 * random(1, 10 * :scale)) % \
(10 * :scale) + 1
+\set divx CASE WHEN :x <> 0 THEN :y/:x ELSE NULL END
 
 

@@ -996,6 +1015,177 @@ pgbench  options  d
   
  
 
+ 
+  Built-In Operators
+
+  
+   The arithmetic, bitwise, comparison and logical operators listed in
+are built into pgbench
+   and may be used in expressions appearing in
+   \set.
+  
+
+  
+   pgbench Operators by increasing precedence
+   
+
+ 
+  Operator
+  Description
+  Example
+  Result
+ 
+
+
+ 
+  OR
+  logical or
+  5 or 0
+  TRUE
+ 
+ 
+  AND
+  logical and
+  3 and 0
+  FALSE
+ 
+ 
+  NOT
+  logical not
+  not false
+  TRUE
+ 
+ 
+  IS [NOT] (NULL|TRUE|FALSE)
+  value tests
+  1 is null
+  FALSE
+ 
+ 
+  ISNULL|NOTNULL
+  null tests
+  1 notnull
+  TRUE
+ 
+ 
+  =
+  is equal
+  5 = 4
+  FALSE
+ 
+ 
+  <>
+  is not equal
+  5 <> 4
+  TRUE
+ 
+ 
+  !=
+  is not equal
+  5 != 5
+  FALSE
+ 
+ 
+  <
+  lower than
+  5 < 4
+  FALSE
+ 
+ 
+  <=
+  lower or equal
+  5 <= 4
+  FALSE
+ 
+ 
+  >
+  greater than
+  5 > 4
+  TRUE
+ 
+ 
+  >=
+  greater or equal
+  5 >= 4
+  TRUE
+ 
+ 
+  |
+  integer bitwise OR
+  1 | 2
+  3
+ 
+ 
+  #
+  integer bitwise XOR
+  1 # 3
+  2
+ 
+ 
+  &
+  integer bitwise AND
+  1 & 3
+  1
+ 
+ 
+  ~
+  integer bitwise NOT
+  ~ 1
+  -2
+ 
+ 
+  <<
+  integer bitwise shift left
+  1 << 2
+  4
+ 
+ 
+  >>
+  integer bitwise shift right
+  8 >> 2
+  2
+ 
+ 
+  +
+  addition
+  5 + 4
+  9
+ 
+ 
+  -
+  substraction
+  3 - 2.0
+  1.0
+ 
+ 
+  *
+  multiplication
+  5 * 4
+  20
+ 
+ 
+  /
+  division (integer truncates the results)
+  5 / 3
+  1
+ 
+ 
+  %
+  modulo
+  3 % 2
+  1
+ 
+ 
+  -
+  opposite
+  - 2.0
+  -2.0
+ 
+
+   
+  
+ 
+
  
   Built-In Functions
 
@@ -1042,6 +1232,13 @@ pgbench  options  d
5432.0
   
   
+   exp(x)
+   double
+   exponential
+   exp(1.0)
+   2.718281828459045
+  
+  
greatest(a [, ... ] )
double if any a is double, else integer
largest value among arguments
@@ -1063,6 +1260,20 @@ pgbench  options  d
2.1
   
   
+   ln(x)
+   double
+   natural logarithm
+   ln(2.718281828459045)
+   1.0
+  
+  
+   mod(i, bj)
+   integer
+   modulo
+   mod(54, 32)
+   22
+  
+  
pi()
double
value of the constant PI
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 25d5ad4..14b3726 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -19,13 +19,17 @@
 PgBenchExpr *expr_parse_result;
 
 static PgBenchExprList *make_elist(PgBenchExpr *exp, PgBenchExprList *list);
+static PgBenchExpr *make_null_constant(void);
+static PgBenchExpr *make_boolean_constant(bool bval);
 static PgBenchExpr *make_integer_constant(int64 ival);
 sta

Re: pgbench - add \if support

[Fabien COELHO]

Mostly a rebase after zipfian function commit.

This patch adds \if support to pgbench, similar to psql's version added in 
March.


This patch brings a consistent set of features especially when combined with 
two other patches already in the (slow) CF process:


- https://commitfest.postgresql.org/10/596/ .. /15/985/
  adds support for booleans expressions (comparisons, logical
  operators, ...). This enhanced expression engine would be useful
  to allow client-side expression in psql.

- https://commitfest.postgresql.org/10/669/ .. /15/669/
  adds support for \gset, so that pgbench can interact with a database
  and extract something into a variable, instead of discarding it.

This patch adds a \if construct so that an expression on variables, possibly 
with data coming from the database, can change the behavior of a script.


For instance, the following script, which uses features from the three 
patches, would implement TPC-B per spec (not "tpcb-like", but really as 
specified).


 \set tbid  random(1, :scale)
 \set tid  10 * (:tbid - 1) + random(1, 10)
 -- client in same branch as teller at 85%
 \if :scale = 1 OR random(0, 99) < 85
   -- same branch
   \set bid  :tbid
 \else
   -- different branch
   \set bid  1 + (:tbid + random(1, :scale - 1)) % :scale
 \endif
 \set aid  :bid * 10 + random(1, 10)
 \set delta  random(-99, 99)
 BEGIN;
 UPDATE pgbench_accounts
   SET abalance = abalance + :delta WHERE aid = :aid
   RETURNING abalance AS balance \gset
 UPDATE pgbench_tellers
   SET tbalance = tbalance + :delta WHERE tid = :tid;
 UPDATE pgbench_branches
   SET bbalance = bbalance + :delta WHERE bid = :bid;
 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime)
   VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
 END;

The patch moves the conditional stack infrastructure from psql to fe_utils, 
so that it is available to both psql & pgbench.


A partial evaluation is performed to detect structural errors (eg missing 
endif, else after else...) when the script is parsed, so that such errors 
cannot occur when a script is running.


A new automaton state is added to quickly step over false branches.

TAP tests ensure reasonable coverage of the feature.




--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3..bbc10b5 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -895,6 +895,21 @@ pgbench  options  d
   
 
   
+   
+\if expression
+\elif expression
+\else
+\endif
+
+  
+  This group of commands implements nestable conditional blocks,
+  similarly to psql's .
+  Conditional expressions are identical to those with \set,
+  with non-zero values interpreted as true.
+ 
+
+   
+

 
  \set varname expression
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fce7e3a..e88f782 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2148,7 +2148,7 @@ hello 10
   
 
 
-  
+  
 \if expression
 \elif expression
 \else
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 7ce6f60..7aa45f6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -32,6 +32,7 @@
 #endif			/* ! WIN32 */
 
 #include "postgres_fe.h"
+#include "fe_utils/conditional.h"
 
 #include "getopt_long.h"
 #include "libpq-fe.h"
@@ -273,6 +274,9 @@ typedef enum
 	 * and we enter the CSTATE_SLEEP state to wait for it to expire. Other
 	 * meta-commands are executed immediately.
 	 *
+	 * CSTATE_SKIP_COMMAND for conditional branches which are not executed,
+	 * quickly skip commands that do not need any evaluation.
+	 *
 	 * CSTATE_WAIT_RESULT waits until we get a result set back from the server
 	 * for the current command.
 	 *
@@ -282,6 +286,7 @@ typedef enum
 	 * command counter, and loops back to CSTATE_START_COMMAND state.
 	 */
 	CSTATE_START_COMMAND,
+	CSTATE_SKIP_COMMAND,
 	CSTATE_WAIT_RESULT,
 	CSTATE_SLEEP,
 	CSTATE_END_COMMAND,
@@ -311,6 +316,7 @@ typedef struct
 	PGconn	   *con;			/* connection handle to DB */
 	int			id;/* client No. */
 	ConnectionStateEnum state;	/* state machine's current state. */
+	ConditionalStack cstack;	/* enclosing conditionals state */
 
 	int			use_file;		/* index in sql_script for this client */
 	int			command;		/* command number in script */
@@ -399,7 +405,11 @@ typedef enum MetaCommand
 	META_SET,	/* \set */
 	META_SETSHELL,/* \setshell */
 	META_SHELL,	/* \shell */
-	META_SLEEP	/* \sleep */
+	META_SLEEP,	/* \sleep */
+	META_IF,	/* \if */
+	META_ELIF,	/* \elif */
+	META_ELSE,	/* \else */
+	META_ENDIF	/* \endif */
 } MetaCommand;
 
 typedef enum QueryMode
@@ -1468,6 +1478,27 @@ coerceToDouble(PgBenchValue *pval, double *dval)
 		return true;
 	}
 }
+
+/*
+ * Return true or false for conditional purposes.
+ * Non zero numerical values are true.
+ */
+static bool
+valueTruth

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Ali Akbar]

2017-12-14 15:08 GMT+07:00 Michael Paquier :
>
> On Thu, Dec 14, 2017 at 2:49 PM, Justin Pryzby  wrote:
> > On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
> >> 2017-12-13 15:37 GMT+07:00 Amit Langote :
> >> Patch for adding check in pg_upgrade. Going through git history, the check
> >> for inconsistency in NOT NULL constraint has ben there since a long time
> >> ago. In this patch the check will be applied for all old cluster version.
> >> I'm not sure in which version was the release of table inheritance.
> >
> > Here are some spelling and grammar fixes to that patch:
> >
> > but nullabe in its children: nullable
> > child column is not market: marked
> > with adding: by adding
> > and restart: and restarting
> > the problem columns: the problematic columns
> > 9.5, 9.6, 10: 9.5, 9.6, and 10
> > restore, that will cause error.: restore phase of pg_upgrade, that would 
> > cause an error.


Thanks. Typo fixed.

>
> I agree that we should do better in pg_upgrade for HEAD and
> REL_10_STABLE as it is not cool to fail late in the upgrade process
> especially if you are using the --link mode.
>
> + *  Currently pg_upgrade will fail if there are any inconsistencies in NOT 
> NULL
> + *  constraint inheritance: In Postgres version 9.5, 9.6, 10 we can
> have a column
> + *  that is NOT NULL in parent, but nullabe in its children. But during 
> schema
> + *  restore, that will cause error.
> On top of the multiple typos here, there is no need to mention
> anything other than "PostgreSQL 9.6 and older versions did not add NOT
> NULL constraints when a primary key was added to to a parent, so check
> for inconsistent NOT NULL constraints in inheritance trees".


In my case, my database becomes like that because accidental ALTER
COLUMN x DROP NOT NULL, and no, not the Primary Key.

Something like this:

CREATE TABLE parent (id SERIAL PRIMARY KEY, name VARCHAR(52) NOT NULL);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE child ALTER COLUMN name DROP NOT NULL;

And yes, in current version 10 (and HEAD), we can still do that.

Because both version currently accepts that inconsistency, ideally
pg_upgrade must be able to upgrade the cluster without problem. Hence
the comment: "Currently pg_upgrade will fail if there are any
inconsistencies in NOT NULL constraint inheritance".

> You seem to take a path similar to what is done with the jsonb check.
> Why not. I would find cleaner to tell also the user about using ALTER
> TABLE to add the proper constraints.
>
> Note that I have not checked or tested the query in details, but
> reading through the thing looks basically correct as it handles
> inheritance chains with WITH RECURSIVE.

Any suggestion to make it more readable? Maybe by separating column
query with schema & table name by using another CTE?

> @@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
>  check_for_prepared_transactions(&old_cluster);
>  check_for_reg_data_type_usage(&old_cluster);
>  check_for_isn_and_int8_passing_mismatch(&old_cluster);
> +check_for_not_null_inheritance(&old_cluster);
> The check needs indeed to happen in check_and_dump_old_cluster(), but
> there is no point to check for it in servers with version 10 and
> upwards, hence you should make it conditional with GET_MAJOR_VERSION()
> <= 906.

No, currently it can happen again in version 10 (and above) because of
the problem above.

By the way, should i add this patch to the current commitfest?

Thanks,
Ali Akbar
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1b9083597c..0bd2cd0ed1 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -26,6 +26,7 @@ static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
 static void check_for_reg_data_type_usage(ClusterInfo *cluster);
 static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
 static void check_for_pg_role_prefix(ClusterInfo *cluster);
+static void check_for_not_null_inheritance(ClusterInfo *cluster);
 static char *get_canonical_locale_name(int category, const char *locale);
 
 
@@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
 	check_for_prepared_transactions(&old_cluster);
 	check_for_reg_data_type_usage(&old_cluster);
 	check_for_isn_and_int8_passing_mismatch(&old_cluster);
+	check_for_not_null_inheritance(&old_cluster);
 
 	/*
 	 * Pre-PG 10 allowed tables with 'unknown' type columns and non WAL logged
@@ -1096,6 +1098,105 @@ check_for_pg_role_prefix(ClusterInfo *cluster)
 	check_ok();
 }
 
+/*
+ * check_for_not_null_inheritance()
+ *
+ *  Currently pg_upgrade will fail if there are any inconsistencies in NOT NULL
+ *  constraint inheritance: In PostgreSQL, we can have a column that is NOT NULL
+ *  in parent, but nullable in its children. So check for inconsistent NOT NULL
+ *  constraints in inheritance tree.
+ */
+static void
+check_for_not_null_inheritance(ClusterInfo *cluster)
+{
+	int			dbnum;
+	FILE	   *script = NULL;
+	bool		found = false;
+	char		out

Re: [HACKERS] pgbench more operators & functions

[Teodor Sigaev]

Huh, you are fast. Rebase patch during half an hour.

I haven't objection about patch idea, but I see some gotchas in coding.

1) /* Try to convert variable to numeric form; return false on failure */
static bool
makeVariableValue(Variable *var)

as now, makeVariableValue() convert value of eny type, not only numeric

2) In makeVariableValue():
if (pg_strcasecmp(var->svalue, "null") == 0)
...
else if (pg_strncasecmp(var->svalue, "true", slen)

mixing of pg_strcasecmp and pg_strNcasecmp. And, IMHO, result of
pg_strncasecmp("tru", "true", 1) will  be 0. It may be good for 't' of 'f' but 
it seems too free grammar to accept  'tr' or 'fa' or  even 'o' which actually 
not known to be on or off.


3) It seems to me that Variable.has_value could be eliminated and then new 
PGBT_NOT_SET is added to PgBenchValueType enum as a first (=0) value. This 
allows to shorten code and make it more readable, look

setBoolValue(&var->value, true);
var->has_value = true;
The second line actually doesn't needed. Although I don't insist to fix that.

I actually prefer PGBT_NOT_SET instead of kind of PGBT_UNDEFINED, because I 
remember a collision in JavaScript with undefined and null variable.



4) valueTypeName()
it checks all possible PgBenchValue.type but believes that field could contain 
some another value. In all other cases it treats as error or assert.





Fabien COELHO wrote:


Attached v16 fixes those two errors. I regenerated the documentation with the 
new xml toolchain, and made "check" overall and in pgbench.


Attached v17 is a rebase after the zipfian commit.



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: procedures and plpgsql PERFORM

[Tom Lane]

Ashutosh Bapat  writes:
> We allow a function to be invoked as part of PERFORM statement in plpgsql
> ...
> But we do not allow a procedure to be invoked this way

> Procedures fit that category and like functions, I think, we should
> allow them be invoked directly without any quoting and CALL
> decoration.

How is that going to work?  What if the procedure tries to commit the
current transaction?

IOW, this is not merely a syntactic-sugar question.

regards, tom lane

Re: [HACKERS] pgbench more operators & functions

[Fabien COELHO]

Hello Teodor,


Huh, you are fast. Rebase patch during half an hour.


Hmmm... just lucky, and other after lunch tasks were more demanding.


I haven't objection about patch idea, but I see some gotchas in coding.

1) /* Try to convert variable to numeric form; return false on failure */
static bool
makeVariableValue(Variable *var)

as now, makeVariableValue() convert value of eny type, not only numeric


Indeed, the comments need updating. I found a few instances.


2) In makeVariableValue():
if (pg_strcasecmp(var->svalue, "null") == 0)
...
else if (pg_strncasecmp(var->svalue, "true", slen)

mixing of pg_strcasecmp and pg_strNcasecmp. And, IMHO, result of
pg_strncasecmp("tru", "true", 1) will  be 0.


Yep, but it cannot be called like that because slen == 
strlen(var->svalue).


It may be good for 't' of 'f' but it seems too free grammar to accept 
'tr' or 'fa' or even 'o' which actually not known to be on or off.


Yes, it really works like that. I tried to make something clearer than 
"src/bin/psql/variable.c". Maybe I did not succeed.


I have added a comment and use some shortened versions in tests, with 
the -D option.


3) It seems to me that Variable.has_value could be eliminated and then new 
PGBT_NOT_SET is added to PgBenchValueType enum as a first (=0) value. This 
allows to shorten code and make it more readable, look

   setBoolValue(&var->value, true);
   var->has_value = true;
The second line actually doesn't needed. Although I don't insist to fix that.


I agree that the redundancy should be avoided. I tried to keep 
"is_numeric" under some form, but there is no added value doing that.


I actually prefer PGBT_NOT_SET instead of kind of PGBT_UNDEFINED, because I 
remember a collision in JavaScript with undefined and null variable.


I used "PGBT_NO_VALUE" which seemed clearer otherwise a variable may be 
set and its value would not "not set" which would look strange.



4) valueTypeName()
it checks all possible PgBenchValue.type but believes that field could 
contain some another value. In all other cases it treats as error or assert.


Ok. Treated as an internal error with an assert.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3..ea8f305 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -904,14 +904,32 @@ pgbench  options  d
  
   Sets variable varname to a value calculated
   from expression.
-  The expression may contain integer constants such as 5432,
+  The expression may contain the NULL constant,
+  boolean constants TRUE and FALSE,
+  integer constants such as 5432,
   double constants such as 3.14159,
   references to variables :variablename,
-  unary operators (+, -) and binary operators
-  (+, -, *, /,
-  %) with their usual precedence and associativity,
-  function calls, and
-  parentheses.
+  operators
+  with their usual SQL precedence and associativity,
+  function calls,
+  SQL CASE generic conditional
+  expressions and parentheses.
+ 
+
+ 
+  Functions and most operators return NULL on
+  NULL input.
+ 
+
+ 
+  For conditional purposes, non zero numerical values are
+  TRUE, zero numerical values and NULL
+  are FALSE.
+ 
+
+ 
+  When no final ELSE clause is provided to a
+  CASE, the default value is NULL.
  
 
  
@@ -920,6 +938,7 @@ pgbench  options  d
 \set ntellers 10 * :scale
 \set aid (1021 * random(1, 10 * :scale)) % \
(10 * :scale) + 1
+\set divx CASE WHEN :x <> 0 THEN :y/:x ELSE NULL END
 
 

@@ -996,6 +1015,177 @@ pgbench  options  d
   
  
 
+ 
+  Built-In Operators
+
+  
+   The arithmetic, bitwise, comparison and logical operators listed in
+are built into pgbench
+   and may be used in expressions appearing in
+   \set.
+  
+
+  
+   pgbench Operators by increasing precedence
+   
+
+ 
+  Operator
+  Description
+  Example
+  Result
+ 
+
+
+ 
+  OR
+  logical or
+  5 or 0
+  TRUE
+ 
+ 
+  AND
+  logical and
+  3 and 0
+  FALSE
+ 
+ 
+  NOT
+  logical not
+  not false
+  TRUE
+ 
+ 
+  IS [NOT] (NULL|TRUE|FALSE)
+  value tests
+  1 is null
+  FALSE
+ 
+ 
+  ISNULL|NOTNULL
+  null tests
+  1 notnull
+  TRUE
+ 
+ 
+  =
+  is equal
+  5 = 4
+  FALSE
+ 
+ 
+  <>
+  is not equal
+  5 <> 4
+  TRUE
+ 
+ 
+  !=
+  is not equal
+  5 != 5
+  FALSE
+ 
+ 
+  <
+  lower than
+  5 < 4
+  FALSE
+ 
+ 
+  <=
+  lower or equal
+  5 <= 4
+  FALSE
+ 
+ 
+  >
+  greater than
+  5 > 4
+  TRUE
+ 
+ 
+  >=
+  greater or equal
+  5 >= 4
+  TRUE
+ 
+ 
+  |
+  integer bitwise OR
+  1 | 2
+ 

Re: PATCH: Exclude unlogged tables from base backups

[Robert Haas]

On Tue, Dec 12, 2017 at 8:48 PM, Stephen Frost  wrote:
> If the persistence is changed then the table will be written into the
> WAL, no?  All of the WAL generated during a backup (which is what we're
> talking about here) has to be replayed after the restore is done and is
> before the database is considered consistent, so none of this matters,
> as far as I can see, because the drop table or alter table logged or
> anything else will be in the WAL that ends up getting replayed.

I can't see a hole in this argument.  If we copy the init fork and
skip copying the main fork, then either we skipped copying the right
file, or the file we skipped copying will be recreated with the
correct contents during WAL replay anyway.

We could have a problem if wal_level=minimal, because then the new
file might not have been WAL-logged; but taking an online backup with
wal_level=minimal isn't supported precisely because we won't have WAL
replay to fix things up.

We would also have a problem if the missing file caused something in
recovery to croak on the grounds that the file was expected to be
there, but I don't think anything works that way; I think we just
assume missing files are an expected failure mode and silently do
nothing if asked to remove them.

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

Sun Studio 12 vs. __builtin_constant_p()

[Tom Lane]

Well, that's annoying: buildfarm member castoroides just fell over
with symptoms indicating that its compiler thinks that
__builtin_constant_p("string literal") is false, thus breaking the
check I installed in commit 9fa6f00b1 that AllocSetContextCreate's
name argument is a literal.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=castoroides&dt=2017-12-14%2013%3A03%3A05

I think removing that check would be entirely impractical, and
anyway it's sufficient if it works on "most people's" compilers.
Fortunately, buildfarm results so far suggest that most compilers
do accept this usage of __builtin_constant_p, if they know the
function at all.

What I propose to do is adjust the configure check for
__builtin_constant_p so that it explicitly checks for
__builtin_constant_p("string literal") being true, in addition
to what it tests now.  That will result in slightly worse
code optimization around ereport/elog calls on compilers that
don't have this behavior, but that seems fine to me.

regards, tom lane

Re: access/parallel.h lacks PGDLLIMPORT

[Robert Haas]

On Wed, Dec 13, 2017 at 8:19 PM, Thomas Munro
 wrote:
> I suppose that extensions are supposed to be allowed to use the
> facilities in access/parallel.h.  I noticed in passing when I wrote a
> throwaway test harness that my Windows built drone complained:
>
> test_sharedtuplestore.obj : error LNK2001: unresolved external symbol
> ParallelWorkerNumber
> [C:\projects\postgres\test_sharedtuplestore.vcxproj]
> .\Release\test_sharedtuplestore\test_sharedtuplestore.dll : fatal
> error LNK1120: 1 unresolved externals
> [C:\projects\postgres\test_sharedtuplestore.vcxproj]
>
> I suppose that all three of these might need that, if they're part of
> the API for parallel worker management:
>
> extern volatile bool ParallelMessagePending;
> extern int  ParallelWorkerNumber;
> extern bool InitializingParallelWorker;
>
> I'm less sure about the other two but at least ParallelWorkerNumber is
> essential for anything that needs to coordinate access to input/output
> arrays or similar.

I can't really think of a reason for extensions to need to access
ParallelMessagePending.  InitializingParallelWorker could be useful if
the extension is doing something strange with custom GUCs.
ParallelWorkerNumber is useful for the reason you state.  It's not
absolutely essential, because an extension can do something like what
test_shm_mq does to assign worker numbers based on order of startup,
but it's certainly useful.

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

Re: incorrect error message, while dropping PROCEDURE

[Peter Eisentraut]

On 12/13/17 23:31, Rushabh Lathia wrote:
> Currently if some one try to drop the PROCEDURE and
> it don't have privilege or it's not an owner, than error message
> still indicate object as FUNCTION.

Yes, that is actually something that is fixed by the patches proposed in
the thread "replace GrantObjectType with ObjectType".

> PFA patch, where introduced new AclObjectKind (ACL_KIND_PROCEDURE),
> msg for the new AclObjectKind, and passed it through at
> appropriate places.

Yeah, that's a way to do it, but having both ACL_KIND_PROC and
ACL_KIND_PROCEDURE is clearly confusing.  The above-mentioned patch
cleans that up more thoroughly, I think.

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

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Dec 13, 2017 at 5:10 AM, Tomas Vondra
 wrote:
>> 2. If several data types can benefit from a similar approach, it has
>> to be separately implemented for each one.
>
> I don't think the current solution improves that, though. If you want to
> exploit internal features of individual data types, it pretty much
> requires code customized to every such data type.
>
> For example you can't take the tsvector compression and just slap it on
> tsquery, because it relies on knowledge of internal tsvector structure.
> So you need separate implementations anyway.

I don't think that's necessarily true.  Certainly, it's true that *if*
tsvector compression depends on knowledge of internal tsvector
structure, *then* that you can't use the implementation for anything
else (this, by the way, means that there needs to be some way for a
compression method to reject being applied to a column of a data type
it doesn't like).  However, it seems possible to imagine compression
algorithms that can work for a variety of data types, too.  There
might be a compression algorithm that is theoretically a
general-purpose algorithm but has features which are particularly
well-suited to, say, JSON or XML data, because it looks for word
boundaries to decide on what strings to insert into the compression
dictionary.

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

Re: procedures and plpgsql PERFORM

[Merlin Moncure]

On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>> We allow a function to be invoked as part of PERFORM statement in plpgsql
>> ...
>> But we do not allow a procedure to be invoked this way
>
>> Procedures fit that category and like functions, I think, we should
>> allow them be invoked directly without any quoting and CALL
>> decoration.
>
> How is that going to work?  What if the procedure tries to commit the
> current transaction?
>
> IOW, this is not merely a syntactic-sugar question.

I think OP is simply misunderstanding the docs. In pl/pgsql, a leading
keyword (SELECT/PERFORM/CALL/etc) is *required*. For example, you
can't do this:


BEGIN
  a := 1;
  f();  -- illegal
  PERFORM f(); -- ok
  b := f(); -- ok
...


What the documentation is trying to say is that you can do
INSERT/UPDATE/etc without any extra decoration and no special handling
as with PERFORM.  Another way of stating that is SQL commands are
'first class' in pl/pgsql, in that they can be inserted without any
pl/pgsql handling.

BTW, We've already come to (near-but good enough) consensus that
PERFORM syntax is really just unnecessary, and I submitted a patch to
make it optional (which I really need to dust off and complete).  If
so done, that entire section of language in the docs would be moot
since SELECT wouldn't really be any different in pl/pgsql than say,
INSERT from a syntax perspective.  All SQL commands, except for those
that we've reserve to be not usable in functions/procedures would
operate similarly 'in-procedure' vs 'not-in-'procedure', which is a
good thing IMO.  This thread is yet another example of why the
SELECT/PERFORM dichotomy just confuses people.

merlin

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Dec 13, 2017 at 1:34 PM, Tomas Vondra
 wrote:
> Wouldn't that require some universal compression level, shared by all
> supported compression algorithms? I don't think there is such thing.
>
> Defining it should not be extremely difficult, although I'm sure there
> will be some cumbersome cases. For example what if an algorithm "a"
> supports compression levels 0-10, and algorithm "b" only supports 0-3?
>
> You may define 11 "universal" compression levels, and map the four
> levels for "b" to that (how). But then everyone has to understand how
> that "universal" mapping is defined.

What we could do is use the "namespace" feature of reloptions to
distinguish options for the column itself from options for the
compression algorithm.  Currently namespaces are used only to allow
you to configure toast.whatever = somevalue, but we could let you say
pglz.something = somevalue or lz4.whatever = somevalue.  Or maybe, to
avoid confusion -- what happens if somebody invents a compression
method called toast? -- we should do it as compress.lz4.whatever =
somevalue.  I think this takes us a bit far afield from the core
purpose of this patch and should be a separate patch at a later time,
but I think it would be cool.

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

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Dec 13, 2017 at 7:18 AM, Ildus Kurbangaliev
 wrote:
> Since we agreed on ALTER syntax, i want to clear things about CREATE.
> Should it be CREATE ACCESS METHOD .. TYPE СOMPRESSION or CREATE
> COMPRESSION METHOD? I like the access method approach, and it
> simplifies the code, but I'm just not sure a compression is an access
> method or not.

+1 for ACCESS METHOD.

> Current implementation
> --
>
> To avoid extra patches I also want to clear things about current
> implementation. Right now there are two tables, "pg_compression" and
> "pg_compression_opt". When compression method is linked to a column it
> creates a record in pg_compression_opt. This record's Oid is stored in
> the varlena. These Oids kept in first column so I can move them in
> pg_upgrade but in all other aspects they behave like usual Oids. Also
> it's easy to restore them.

pg_compression_opt -> pg_attr_compression, maybe.

> Compression options linked to a specific column. When tuple is
> moved between relations it will be decompressed.

Can we do this only if the compression method isn't OK for the new
column?  For example, if the old column is COMPRESS foo PRESERVE bar
and the new column is COMPRESS bar PRESERVE foo, we don't need to
force decompression in any case.

> Also in current implementation SET COMPRESSION contains WITH syntax
> which is used to provide extra options to compression method.

Hmm, that's an alternative to use reloptions.  Maybe that's fine.

> What could be changed
> -
>
> As Alvaro mentioned COMPRESSION METHOD is practically an access method,
> so it could be created as CREATE ACCESS METHOD .. TYPE COMPRESSION.
> This approach simplifies the patch and "pg_compression" table could be
> removed. So compression method is created with something like:
>
> CREATE ACCESS METHOD .. TYPE COMPRESSION HANDLER
> awesome_compression_handler;
>
> Syntax of SET COMPRESSION changes to SET COMPRESSION .. PRESERVE which
> is useful to control rewrites and for pg_upgrade to make dependencies
> between moved compression options and compression methods from pg_am
> table.
>
> Default compression is always pglz and if users want to change they run:
>
> ALTER COLUMN  SET COMPRESSION awesome PRESERVE pglz;
>
> Without PRESERVE it will rewrite the whole relation using new
> compression. Also the rewrite removes all unlisted compression options
> so their compresssion methods could be safely dropped.

That all sounds good.

> "pg_compression_opt" table could be renamed to "pg_compression", and
> compression options will be stored there.

See notes above.

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

Re: BUG #14941: Vacuum crashes

[Bossart, Nathan]

On 12/10/17, 11:13 PM, "Michael Paquier"  wrote:
> Let's see what other folks think first about the ANALYZE grammar in
> VACUUM, as well as having ANALYZE use a parenthesized grammer. FWIW, I
> like the contents of this patch set, and the thing is non-intrusive. I
> think that NOWAIT gains more value now that multiple relations can be
> vacuumed or analyzed with a single manual command.

Thanks for taking a look.

Nathan

Re: BUG #14941: Vacuum crashes

[Bossart, Nathan]

On 12/11/17, 9:41 PM, "Masahiko Sawada"  wrote:
> I also reviewed the patches.

Thanks for taking a look.

> For 0003 patch, if we do VACUUM (NOWAIT) to whole database we don't
> get the above WARNING messages, but I think we should get them. The
> reported issue also did VACUUM FULL and REINDEX to whole database.
> Especially when VACUUM to whole database the information of skipped
> relations would be useful for users.

Perhaps we could attempt to construct the RangeVar just before
logging in this case.

Nathan

Re: procedures and plpgsql PERFORM

[David G. Johnston]

On Thu, Dec 14, 2017 at 8:22 AM, Merlin Moncure  wrote:

> On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
> > Ashutosh Bapat  writes:
> >> We allow a function to be invoked as part of PERFORM statement in
> plpgsql
> >> ...
> >> But we do not allow a procedure to be invoked this way
> >
> >> Procedures fit that category and like functions, I think, we should
> >> allow them be invoked directly without any quoting and CALL
> >> decoration.
> >
> > How is that going to work?  What if the procedure tries to commit the
> > current transaction?
> >
> > IOW, this is not merely a syntactic-sugar question.
>
> BTW, We've already come to (near-but good enough) consensus that
> PERFORM syntax is really just unnecessary, and I submitted a patch to
> make it optional (which I really need to dust off and complete).


​Except right now PERFORM doesn't exist in SQL and is a pl/pgsql keyword to
specify a specific limited form of the SQL SELECT command.  CALL is an SQL
command.  I don't see any real upside to allowing pl/pgsql to accept
omission of the command tag while SQL cannot - at least not without a
use-case describe why such syntax would be beneficial.  And likely those
use cases would revolve around some looping variant as opposed to a single
stand-alone, result-less, CALL.

If we do keep "PERFORM" in the pl/pgsql vocab I'd consider the following
enhancement:
PERFORM func() => SELECT func()
PERFORM proc() => CALL proc()

I prefer Merlin's suggestion to just documenting that PERFORM is deprecated
and works only with functions - and that to use procedures in pl/pgsql just
use the normal SQL CALL command.  And to write: "SELECT func()" to invoke
functions, again just like one would in an SQL script.

David J.

Re: [HACKERS] Walsender timeouts and large transactions

[Andrew Dunstan]

On 12/14/2017 01:46 AM, Craig Ringer wrote:
> On 7 December 2017 at 01:22, Petr Jelinek
> mailto:petr.jeli...@2ndquadrant.com>>
> wrote:
>
> On 05/12/17 21:07, Robert Haas wrote:
> > 
> > Generally we write if (a && b) { ... } not if (a) { if (b) .. }
> >
>
> It's rather ugly with && because one of the conditions is two
> line, but
> okay here you go. I am keeping the brackets even if normally don't for
> one-liners because it's completely unreadable without them IMHO.
>
>
>  
> Yeah, that's why I passed on that FWIW. Sometimes breaking up a
> condition is nice. Personally I intensely dislike the convention of 
>
>
> if (big_condition
>     && big_condition)
>    one_linerdo_something;
>
>
> as awfully unreadable, but I guess code convention means you live with
> things you don't like.
>  
>
> Anyway, I've just hit this bug in the wild for the umpteenth time this
> year, and I'd like to know what I can do to help progress it to
> commit+backport.
>
>


Ask and ye shall receive. I've just committed it.

cheers

andrew

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

Re: procedures and plpgsql PERFORM

[Pavel Stehule]

2017-12-14 17:10 GMT+01:00 David G. Johnston :

> On Thu, Dec 14, 2017 at 8:22 AM, Merlin Moncure 
> wrote:
>
>> On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
>> > Ashutosh Bapat  writes:
>> >> We allow a function to be invoked as part of PERFORM statement in
>> plpgsql
>> >> ...
>> >> But we do not allow a procedure to be invoked this way
>> >
>> >> Procedures fit that category and like functions, I think, we should
>> >> allow them be invoked directly without any quoting and CALL
>> >> decoration.
>> >
>> > How is that going to work?  What if the procedure tries to commit the
>> > current transaction?
>> >
>> > IOW, this is not merely a syntactic-sugar question.
>>
>> BTW, We've already come to (near-but good enough) consensus that
>> PERFORM syntax is really just unnecessary, and I submitted a patch to
>> make it optional (which I really need to dust off and complete).
>
>
> ​Except right now PERFORM doesn't exist in SQL and is a pl/pgsql keyword
> to specify a specific limited form of the SQL SELECT command.  CALL is an
> SQL command.  I don't see any real upside to allowing pl/pgsql to accept
> omission of the command tag while SQL cannot - at least not without a
> use-case describe why such syntax would be beneficial.  And likely those
> use cases would revolve around some looping variant as opposed to a single
> stand-alone, result-less, CALL.
>
> If we do keep "PERFORM" in the pl/pgsql vocab I'd consider the following
> enhancement:
> PERFORM func() => SELECT func()
> PERFORM proc() => CALL proc()
>

I don't like this idea - functions are not procedures - can be nice if it
will be visible.

Pavel


> I prefer Merlin's suggestion to just documenting that PERFORM is
> deprecated and works only with functions - and that to use procedures in
> pl/pgsql just use the normal SQL CALL command.  And to write: "SELECT
> func()" to invoke functions, again just like one would in an SQL script.
>
> David J.
>

Re: [HACKERS] Custom compression methods

[Tomas Vondra]

On 12/14/2017 04:21 PM, Robert Haas wrote:
> On Wed, Dec 13, 2017 at 5:10 AM, Tomas Vondra
>  wrote:
>>> 2. If several data types can benefit from a similar approach, it has
>>> to be separately implemented for each one.
>>
>> I don't think the current solution improves that, though. If you
>> want to exploit internal features of individual data types, it
>> pretty much requires code customized to every such data type.
>>
>> For example you can't take the tsvector compression and just slap
>> it on tsquery, because it relies on knowledge of internal tsvector
>> structure. So you need separate implementations anyway.
> 
> I don't think that's necessarily true. Certainly, it's true that
> *if* tsvector compression depends on knowledge of internal tsvector 
> structure, *then* that you can't use the implementation for anything 
> else (this, by the way, means that there needs to be some way for a 
> compression method to reject being applied to a column of a data
> type it doesn't like).

I believe such dependency (on implementation details) is pretty much the
main benefit of datatype-aware compression methods. If you don't rely on
such assumption, then I'd say it's a general-purpose compression method.

> However, it seems possible to imagine compression algorithms that can
> work for a variety of data types, too. There might be a compression
> algorithm that is theoretically a general-purpose algorithm but has
> features which are particularly well-suited to, say, JSON or XML
> data, because it looks for word boundaries to decide on what strings
> to insert into the compression dictionary.
> 

Can you give an example of such algorithm? Because I haven't seen such
example, and I find arguments based on hypothetical compression methods
somewhat suspicious.

FWIW I'm not against considering such compression methods, but OTOH it
may not be such a great primary use case to drive the overall design.

regards

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

Re: procedures and plpgsql PERFORM

[Merlin Moncure]

On Thu, Dec 14, 2017 at 10:46 AM, Pavel Stehule  wrote:
>
>
> 2017-12-14 17:10 GMT+01:00 David G. Johnston :
>>
>> On Thu, Dec 14, 2017 at 8:22 AM, Merlin Moncure 
>> wrote:
>>>
>>> On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
>>> > Ashutosh Bapat  writes:
>>> >> We allow a function to be invoked as part of PERFORM statement in
>>> >> plpgsql
>>> >> ...
>>> >> But we do not allow a procedure to be invoked this way
>>> >
>>> >> Procedures fit that category and like functions, I think, we should
>>> >> allow them be invoked directly without any quoting and CALL
>>> >> decoration.
>>> >
>>> > How is that going to work?  What if the procedure tries to commit the
>>> > current transaction?
>>> >
>>> > IOW, this is not merely a syntactic-sugar question.
>>>
>>> BTW, We've already come to (near-but good enough) consensus that
>>> PERFORM syntax is really just unnecessary, and I submitted a patch to
>>> make it optional (which I really need to dust off and complete).
>>
>>
>> Except right now PERFORM doesn't exist in SQL and is a pl/pgsql keyword to
>> specify a specific limited form of the SQL SELECT command.  CALL is an SQL
>> command.  I don't see any real upside to allowing pl/pgsql to accept
>> omission of the command tag while SQL cannot - at least not without a
>> use-case describe why such syntax would be beneficial.  And likely those use
>> cases would revolve around some looping variant as opposed to a single
>> stand-alone, result-less, CALL.
>>
>> If we do keep "PERFORM" in the pl/pgsql vocab I'd consider the following
>> enhancement:
>> PERFORM func() => SELECT func()
>> PERFORM proc() => CALL proc()
>
>
> I don't like this idea - functions are not procedures - can be nice if it
> will be visible.

We need to get rid of PERFORM ASAP.  Agree that we need to not obfuscate CALL.

merlin

Re: procedures and plpgsql PERFORM

[Pavel Stehule]

2017-12-14 18:33 GMT+01:00 Merlin Moncure :

> On Thu, Dec 14, 2017 at 10:46 AM, Pavel Stehule 
> wrote:
> >
> >
> > 2017-12-14 17:10 GMT+01:00 David G. Johnston  >:
> >>
> >> On Thu, Dec 14, 2017 at 8:22 AM, Merlin Moncure 
> >> wrote:
> >>>
> >>> On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
> >>> > Ashutosh Bapat  writes:
> >>> >> We allow a function to be invoked as part of PERFORM statement in
> >>> >> plpgsql
> >>> >> ...
> >>> >> But we do not allow a procedure to be invoked this way
> >>> >
> >>> >> Procedures fit that category and like functions, I think, we should
> >>> >> allow them be invoked directly without any quoting and CALL
> >>> >> decoration.
> >>> >
> >>> > How is that going to work?  What if the procedure tries to commit the
> >>> > current transaction?
> >>> >
> >>> > IOW, this is not merely a syntactic-sugar question.
> >>>
> >>> BTW, We've already come to (near-but good enough) consensus that
> >>> PERFORM syntax is really just unnecessary, and I submitted a patch to
> >>> make it optional (which I really need to dust off and complete).
> >>
> >>
> >> Except right now PERFORM doesn't exist in SQL and is a pl/pgsql keyword
> to
> >> specify a specific limited form of the SQL SELECT command.  CALL is an
> SQL
> >> command.  I don't see any real upside to allowing pl/pgsql to accept
> >> omission of the command tag while SQL cannot - at least not without a
> >> use-case describe why such syntax would be beneficial.  And likely
> those use
> >> cases would revolve around some looping variant as opposed to a single
> >> stand-alone, result-less, CALL.
> >>
> >> If we do keep "PERFORM" in the pl/pgsql vocab I'd consider the following
> >> enhancement:
> >> PERFORM func() => SELECT func()
> >> PERFORM proc() => CALL proc()
> >
> >
> > I don't like this idea - functions are not procedures - can be nice if it
> > will be visible.
>
> We need to get rid of PERFORM ASAP.  Agree that we need to not obfuscate
> CALL.
>

If we have a procedures, then functions without returned values lost a
sense - and I don't see any changes with PERFORM necessary.

Regards

Pavel



> merlin
>

Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

[Fujii Masao]

On Mon, Dec 11, 2017 at 2:16 PM, Michael Paquier
 wrote:
> On Mon, Dec 11, 2017 at 2:03 PM, Masahiko Sawada  
> wrote:
>> On Sat, Dec 9, 2017 at 2:24 AM, Robert Haas  wrote:
>>> On Fri, Dec 8, 2017 at 4:13 AM, Michael Paquier
>>>  wrote:
 I would just write "To
 avoid calling CHECK_FOR_INTERRUPTS which can happen when releasing a
 LWLock" and be done with it. There is no point to list a full function
 dependency list, which could change in the future with static routines
 of lwlock.c.
>>
>> Agreed. Updated the comment.
>
> Robert actually liked adding the complete routine list. Let's see what
> Fujii-san thinks at the end, there is still some time until the next
> round of minor releases.

What I think is the patch I attached. Thought?

Regards,

-- 
Fujii Masao


fix_do_pg_abort_backup_v12_fujii.patch
Description: Binary data

Re: procedures and plpgsql PERFORM

[Merlin Moncure]

On Thu, Dec 14, 2017 at 11:56 AM, Pavel Stehule  wrote:
>
>
> 2017-12-14 18:33 GMT+01:00 Merlin Moncure :
>>
>> On Thu, Dec 14, 2017 at 10:46 AM, Pavel Stehule 
>> wrote:
>> >
>> >
>> > 2017-12-14 17:10 GMT+01:00 David G. Johnston
>> > :
>> >>
>> >> On Thu, Dec 14, 2017 at 8:22 AM, Merlin Moncure 
>> >> wrote:
>> >>>
>> >>> On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
>> >>> > Ashutosh Bapat  writes:
>> >>> >> We allow a function to be invoked as part of PERFORM statement in
>> >>> >> plpgsql
>> >>> >> ...
>> >>> >> But we do not allow a procedure to be invoked this way
>> >>> >
>> >>> >> Procedures fit that category and like functions, I think, we should
>> >>> >> allow them be invoked directly without any quoting and CALL
>> >>> >> decoration.
>> >>> >
>> >>> > How is that going to work?  What if the procedure tries to commit
>> >>> > the
>> >>> > current transaction?
>> >>> >
>> >>> > IOW, this is not merely a syntactic-sugar question.
>> >>>
>> >>> BTW, We've already come to (near-but good enough) consensus that
>> >>> PERFORM syntax is really just unnecessary, and I submitted a patch to
>> >>> make it optional (which I really need to dust off and complete).
>> >>
>> >>
>> >> Except right now PERFORM doesn't exist in SQL and is a pl/pgsql keyword
>> >> to
>> >> specify a specific limited form of the SQL SELECT command.  CALL is an
>> >> SQL
>> >> command.  I don't see any real upside to allowing pl/pgsql to accept
>> >> omission of the command tag while SQL cannot - at least not without a
>> >> use-case describe why such syntax would be beneficial.  And likely
>> >> those use
>> >> cases would revolve around some looping variant as opposed to a single
>> >> stand-alone, result-less, CALL.
>> >>
>> >> If we do keep "PERFORM" in the pl/pgsql vocab I'd consider the
>> >> following
>> >> enhancement:
>> >> PERFORM func() => SELECT func()
>> >> PERFORM proc() => CALL proc()
>> >
>> >
>> > I don't like this idea - functions are not procedures - can be nice if
>> > it
>> > will be visible.
>>
>> We need to get rid of PERFORM ASAP.  Agree that we need to not obfuscate
>> CALL.
>
> If we have a procedures, then functions without returned values lost a sense
> - and I don't see any changes with PERFORM necessary.

I don't think the presence of procedures really changes the thinking
here.  Having to simulate procedures with void returning functions
wasn't really the point; it's an annoying syntax departure from SQL
for little benefit other than assuming the users are wrong when they
are not explicitly capturing the result..

the topic was heavily discussed:
https://www.postgresql.org/message-id/CAHyXU0zYbeT-FzuonaaycbS9Wd8d5JO%2B_niAygzYtv5FMdx4rg%40mail.gmail.com


merlin

Re: [HACKERS] Proposal for CSN based snapshots

[Alexander Kuzmenkov]

El 08/12/17 a las 14:59, Alexander Korotkov escribió:
These results look promising for me.  Could you try benchmarking using 
more workloads including read-only and mixed mostly-read workloads?
You can try same benchmarks I used in my talk about CSN in pgconf.eu 
 [1] slides 19-25 (and you're welcome to invent more 
benchmakrs yourself)




Sure, here are some more benchmarks.

I've already had measured the "skip some updates" and "select-only" 
pgbench variants, and also a simple "select 1" query. These are for 
scale 1500 and for 20 to 1000 connections. The graphs are attached.
"select 1", which basically benchmarks snapshot taking, shows an 
impressive twofold increase in TPS over master, but this is to be 
expected. "select-only" stabilizes at 20% higher than master. 
Interesting to note is that these select-only scenarios almost do not 
degrade with growing client count.
For the "skip some updates" scenario, CSN is slightly slower than 
master, but this is improved by the LWLock patch I mentioned upthread.


I also replicated the setup from your slides 23 and 25. I used scale 500 
and client counts 20-300, and probably the same 72-core Xeon.
Slide 23 shows 22% write and 78% read queries, that is "-b select-only@9 
-b tpcb-like@1". The corresponding picture is called "random.png". The 
absolute numbers are somewhat lower for my run, but CSN is about 40% 
faster than master, like the CSN-rewrite variant.
Slide 25 is a custom script called "rrw" with extra 20 read queries. We 
can see that since your run the master has improved much, and the 
current CSN shows the same general behaviour as CSN-rewrite, although 
being slower in absolute numbers.


Also, I wonder how current version of CSN patch behaves in worst case 
when we have to scan the table with a lot of unique xid (and 
correspondingly have to do a lot of csnlog lookups)?  See [1] slide 
18.  This worst case was significant part of my motivation to try 
"rewrite xid with csn" approach.  Please, find simple extension I used 
to fill table with random xmins in the attachment.




OK, let's summarize how the worst case in question works. It happens 
when the tuples have a wide range of xmins and no hint bits. All 
visibility checks have to go to clog then. Fortunately, on master we 
also set hint bits during these checks, and the subsequent scans can 
determine visibility using just the hint bits and the current snapshot. 
This makes the subsequent scans run much faster. With CSN, this is not 
enough, and the visibility checks always go to CSN log for all the 
transactions newer than global xmin. This can become very slow if there 
is a long-running transaction that holds back the global xmin.


I made a simple test to see these effects. The procedure is as follows. 
I start a transaction in psql; that will be our long-running 
transaction. Next, I run pgbench for a minute, and randomize the tuple 
xmins in "pgbench_accounts" using your extension. Then I run "select 
sum(abalance) from pgbench_accounts" twice, and record the durations.
Here is a table with the results (50M tuples, 1M transactions for master 
and 400k for CSN):


Branch    scan 1, s   scan 2, s

CSN  80  80
master   13  3.5

So, we are indeed seeing the expected results. Significant slowdown with 
long-running transaction is an important problem for this patch. Even 
the first scan is much slower, because a CSN log page contains 16 times 
less transactions than a clog page, and we have the same number of 
buffers (max 128) for them. When the range of xids is wide, we spend 
most of the time loading and unloading the pages. I believe it can be 
improved by using more buffers for CSN log. I did some work in this 
direction, namely, made SLRUs use a dynahash table instead of linear 
search for page->buffer lookups. This is included in the v8 I posted 
earlier, but it should probably be done as a separate patch.


--
Alexander Kuzmenkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Surjective functional indexes

[Robert Haas]

On Wed, Dec 13, 2017 at 12:32 PM, Konstantin Knizhnik
 wrote:
> I can not believe that there can be more than thousand non-temporary
> relations in any database.

I ran across a cluster with more than 5 million non-temporary
relations just this week.  That's extreme, but having more than a
thousand non-temporary tables is not particularly uncommon.  I would
guess that the percentage of EnterpriseDB's customers who have more
than a thousand non-temporary tables in a database is in the double
digits.  That number is only going to go up as our partitioning
capabilities improve.

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

Re: pgbench's expression parsing & negative numbers

[Andres Freund]

Hi,

On 2017-12-14 10:41:04 +0100, Fabien COELHO wrote:
>  - I do not think that updating pgbench arithmetic for managing integer
>overflows is worth Andres Freund time. My guess is that most
>script would not trigger client-side overflows, so the change would
>be a no-op in practice.

It might not be if you view it in isolation (although I'm not
convinced). The problem is that it has cost beyond pgbench. Due to
pgbench's overflow handling I can't run make check on a build that has
-ftrapv, which found several bugs already.

I'd be happy if somebody else would tackle the issue, but I don't quite
see it happening...

Greetings,

Andres Freund

Re: pgsql: Provide overflow safe integer math inline functions.

[Andres Freund]

Hi,

On 2017-12-14 09:28:08 +0100, Christoph Berg wrote:
> Re: Andres Freund 2017-12-13 
> <20171213173524.rjs7b3ahsong5...@alap3.anarazel.de>
> > After staring at it for a while, I seem to have partially mis-copied the
> > note for addition to the subtraction operation...
> 
> I believe the attached is the correct version.

Thanks! Pushed.

Schönen Abend,

Andres

Re: Top-N sorts verses parallelism

[Jeff Janes]

On Tue, Dec 12, 2017 at 10:46 PM, Thomas Munro <
thomas.mu...@enterprisedb.com> wrote:

> Hi hackers,
>
> The start-up cost of bounded (top-N) sorts is sensitive at the small
> end of N, and the
> comparison_cost * tuples * LOG2(2.0 * output_tuples) curve doesn't
> seem to correspond to reality.  Here's a contrived example that comes
> from a benchmark:
>
>   create table foo as
> select generate_series(1, 100)::int a, (random() * 1000)::int b;
>   analyze foo;
>   select * from foo order by b limit N;
>


This looks like a costing bug.  The estimated cost of sorting 416,667
estimated tuples in one parallel worker is almost identical to the
estimated cost of sorting 1,000,000 tuples when parallelism is turned off.
Adding some logging to the cost_sort function, it looks like the limit does
not get sent down for the parallel estimate:

NOTICE:  JJ cost_sort tuples 100.00, limit 61.00, sort_mem 65536
NOTICE:  JJ cost_sort tuples 416667.00, limit -1.00, sort_mem 65536

So the LIMIT gets passed down for the execution step, but not for the
planning step.

(On my system, LIMIT 61 is the point at which it switches to parallel)

In any case, if we "fixed" the top-n estimate to use the random-case rather
than the worst-case, that would make the LIMIT 133 look more like the LIMIT
1, so would be the opposite direction of what you want.

Cheers,

Jeff

Re: [HACKERS] Surjective functional indexes

[Alvaro Herrera]

Konstantin Knizhnik wrote:

> If you still thing that additional 16 bytes per relation in statistic is too
> high overhead, then I will also remove autotune.

I think it's pretty clear that these additional bytes are excessive.

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

Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

[Michael Paquier]

On Fri, Dec 15, 2017 at 3:52 AM, Fujii Masao  wrote:
> On Mon, Dec 11, 2017 at 2:16 PM, Michael Paquier
>  wrote:
>> On Mon, Dec 11, 2017 at 2:03 PM, Masahiko Sawada  
>> wrote:
>>> On Sat, Dec 9, 2017 at 2:24 AM, Robert Haas  wrote:
 On Fri, Dec 8, 2017 at 4:13 AM, Michael Paquier
  wrote:
> I would just write "To
> avoid calling CHECK_FOR_INTERRUPTS which can happen when releasing a
> LWLock" and be done with it. There is no point to list a full function
> dependency list, which could change in the future with static routines
> of lwlock.c.
>>>
>>> Agreed. Updated the comment.
>>
>> Robert actually liked adding the complete routine list. Let's see what
>> Fujii-san thinks at the end, there is still some time until the next
>> round of minor releases.
>
> What I think is the patch I attached. Thought?

That's OK for me. Thanks.
-- 
Michael

Re: [HACKERS] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

[Andres Freund]

On 2017-12-07 18:32:51 +0900, Michael Paquier wrote:
> On Thu, Dec 7, 2017 at 5:23 AM, Alvaro Herrera  
> wrote:
> > Looking at 0002: I agree with the stuff being done here.
> 
> The level of details you are providing with a proper error code is an
> improvement over the first version proposed in my opinion.
> 
> > I think a
> > couple of these checks could be moved one block outerwards in term of
> > scope; I don't see any reason why the check should not apply in that
> > case. I didn't catch any place missing additional checks.
> 
> In FreezeMultiXactId() wouldn't it be better to issue an error as well
> for this assertion?
> Assert(!TransactionIdPrecedes(members[i].xid, cutoff_xid));

I'm not really concerned that much about pure lockers, they don't cause
permanent data corruption...


> > Despite these being "shouldn't happen" conditions, I think we should
> > turn these up all the way to ereports with an errcode and all, and also
> > report the XIDs being complained about.  No translation required,
> > though.  Other than those changes and minor copy editing a commit
> > (attached), 0002 looks good to me.

If you want to go around doing that in some more places we can do so in
master only...


> +   if (!(tuple->t_infomask & HEAP_XMAX_LOCK_ONLY) &&
> +   TransactionIdDidCommit(xid))
> +   ereport(ERROR,
> +   (errcode(ERRCODE_DATA_CORRUPTED),
> +errmsg("can't freeze committed xmax %u", xid)));
> The usual wording used in errmsg is not the "can't" but "cannot".
> 
> +   ereport(ERROR,
> +   (errcode(ERRCODE_DATA_CORRUPTED),
> +errmsg_internal("uncommitted Xmin %u from
> before xid cutoff %u needs to be frozen",
> +xid, cutoff_xid)));
> "Xmin" I have never seen, but "xmin" I did.

Changed...

Greetings,

Andres Freund

Re: incorrect error message, while dropping PROCEDURE

[Michael Paquier]

On Fri, Dec 15, 2017 at 12:18 AM, Peter Eisentraut
 wrote:
> On 12/13/17 23:31, Rushabh Lathia wrote:
>> PFA patch, where introduced new AclObjectKind (ACL_KIND_PROCEDURE),
>> msg for the new AclObjectKind, and passed it through at
>> appropriate places.
>
> Yeah, that's a way to do it, but having both ACL_KIND_PROC and
> ACL_KIND_PROCEDURE is clearly confusing.  The above-mentioned patch
> cleans that up more thoroughly, I think.

+1. I looked at the patch of Peter, as well as this one, and I prefer
Peter's approach of reducing duplications in concepts.
-- 
Michael

Re: [HACKERS] Surjective functional indexes

[Michael Paquier]

On Fri, Dec 15, 2017 at 6:15 AM, Alvaro Herrera  wrote:
> Konstantin Knizhnik wrote:
>> If you still thing that additional 16 bytes per relation in statistic is too
>> high overhead, then I will also remove autotune.
>
> I think it's pretty clear that these additional bytes are excessive.

The bar to add new fields in PgStat_TableCounts in very high, and one
attempt to tackle its scaling problems with many relations is here by
Horiguchi-san:
https://www.postgresql.org/message-id/20171211.201523.24172046.horiguchi.kyot...@lab.ntt.co.jp
His patch may be worth a look if you need more fields for your
feature. So it seems to me that the patch as currently presented has
close to zero chance to be committed as long as you keep your changes
to pgstat.c.
-- 
Michael

worker_spi example BGW code GUC tweak

[Chapman Flack]

Would this sample code make an even better teaching example if it
used the existing GUC way to declare that worker_spi.naptime is
in units of seconds?

Or does it not do that for some reason I've overlooked?

-Chap
>From 50fd326e5c1ff033c701f90ce09ab9b15d600593 Mon Sep 17 00:00:00 2001
From: Chapman Flack 
Date: Thu, 14 Dec 2017 17:24:46 -0500
Subject: [PATCH] Give worker_spi.naptime an explicit time unit.

---
 src/test/modules/worker_spi/worker_spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 4c6ab6d..d8a7ef5 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -316,14 +316,14 @@ _PG_init(void)
 
 	/* get the configuration */
 	DefineCustomIntVariable("worker_spi.naptime",
-			"Duration between each check (in seconds).",
+			"Duration between each check.",
 			NULL,
 			&worker_spi_naptime,
 			10,
 			1,
 			INT_MAX,
 			PGC_SIGHUP,
-			0,
+			GUC_UNIT_S,
 			NULL,
 			NULL,
 			NULL);
-- 
1.8.3.1

Re: pgbench's expression parsing & negative numbers

[Fabien COELHO]

Hello,


 - I do not think that updating pgbench arithmetic for managing integer
   overflows is worth Andres Freund time. My guess is that most
   script would not trigger client-side overflows, so the change would
   be a no-op in practice.


It might not be if you view it in isolation (although I'm not
convinced). The problem is that it has cost beyond pgbench. Due to
pgbench's overflow handling


Lack of?

I can't run make check on a build that has -ftrapv, which found several 
bugs already.


Hmmm. You suggest that integer overflows do occur when running pgbench.

Indeed, this tap test case: "\set maxint debug(:minint - 1)"

Otherwise, some stat counters may overflow on very long runs? Although
overflowing a int64 takes some time...

I'd be happy if somebody else would tackle the issue, but I don't quite 
see it happening...


I must admit that it is not very high on my may-do list. I'll review it if 
it appears, though.


--
Fabien.

Re: Would a BGW need shmem_access or database_connection to enumerate databases?

[Chapman Flack]

On 12/04/2017 09:13 AM, Craig Ringer wrote:
> On 1 December 2017 at 23:04, Chapman Flack  wrote:
>> Can I call RegisterDynamicBackgroundWorker when not in the postmaster,
>> but also not in a "regular backend", but rather another BGW?
>>
> Yes. BDR does it a lot.

Would this doc patch be acceptable to clarify that, in case
I'm not the last person who might wonder?

-Chap
>From 3308ef5647e8ce4a84855b4d0ca09ba6aeb7 Mon Sep 17 00:00:00 2001
From: Chapman Flack 
Date: Thu, 14 Dec 2017 18:09:14 -0500
Subject: [PATCH] Clarify that a BGW can register a dynamic BGW.

---
 doc/src/sgml/bgworker.sgml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index 4bc2b69..e490bb8 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -41,7 +41,7 @@
   *worker, BackgroundWorkerHandle **handle).  Unlike
   RegisterBackgroundWorker, which can only be called from within
   the postmaster, RegisterDynamicBackgroundWorker must be
-  called from a regular backend.
+  called from a regular backend, possibly another background worker.
  
 
  
-- 
1.8.3.1

Re: Would a BGW need shmem_access or database_connection to enumerate databases?

[Michael Paquier]

On Fri, Dec 15, 2017 at 8:12 AM, Chapman Flack  wrote:
> On 12/04/2017 09:13 AM, Craig Ringer wrote:
>> On 1 December 2017 at 23:04, Chapman Flack  wrote:
>>> Can I call RegisterDynamicBackgroundWorker when not in the postmaster,
>>> but also not in a "regular backend", but rather another BGW?
>>>
>> Yes. BDR does it a lot.
>
> Would this doc patch be acceptable to clarify that, in case
> I'm not the last person who might wonder?

This doc addition looks like a good idea to me.
-- 
Michael

Re: [HACKERS] Walsender timeouts and large transactions

[Craig Ringer]

On 15 December 2017 at 00:36, Andrew Dunstan  wrote:

>
>
> On 12/14/2017 01:46 AM, Craig Ringer wrote:
> > On 7 December 2017 at 01:22, Petr Jelinek
> > mailto:petr.jeli...@2ndquadrant.com>>
> > wrote:
> >
> > On 05/12/17 21:07, Robert Haas wrote:
> > >
> > > Generally we write if (a && b) { ... } not if (a) { if (b) .. }
> > >
> >
> > It's rather ugly with && because one of the conditions is two
> > line, but
> > okay here you go. I am keeping the brackets even if normally don't
> for
> > one-liners because it's completely unreadable without them IMHO.
> >
> >
> >
> > Yeah, that's why I passed on that FWIW. Sometimes breaking up a
> > condition is nice. Personally I intensely dislike the convention of
> >
> >
> > if (big_condition
> > && big_condition)
> >one_linerdo_something;
> >
> >
> > as awfully unreadable, but I guess code convention means you live with
> > things you don't like.
> >
> >
> > Anyway, I've just hit this bug in the wild for the umpteenth time this
> > year, and I'd like to know what I can do to help progress it to
> > commit+backport.
> >
> >
>
>
> Ask and ye shall receive. I've just committed it.
>
>
Brilliant, thanks. Backpatched too, great.

Now I'm going to shamelessly point you at the other nasty recurring logical
decoding bug while you're fresh from thinking about replication. I can
hope, right ;)

https://commitfest.postgresql.org/16/1397/ causes errors or bad data to be
decoded, so it's a serious bug.

I'll see if I can rustle up some review attention first though.


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

Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

[Masahiko Sawada]

On Fri, Dec 15, 2017 at 6:46 AM, Michael Paquier
 wrote:
> On Fri, Dec 15, 2017 at 3:52 AM, Fujii Masao  wrote:
>> On Mon, Dec 11, 2017 at 2:16 PM, Michael Paquier
>>  wrote:
>>> On Mon, Dec 11, 2017 at 2:03 PM, Masahiko Sawada  
>>> wrote:
 On Sat, Dec 9, 2017 at 2:24 AM, Robert Haas  wrote:
> On Fri, Dec 8, 2017 at 4:13 AM, Michael Paquier
>  wrote:
>> I would just write "To
>> avoid calling CHECK_FOR_INTERRUPTS which can happen when releasing a
>> LWLock" and be done with it. There is no point to list a full function
>> dependency list, which could change in the future with static routines
>> of lwlock.c.

 Agreed. Updated the comment.
>>>
>>> Robert actually liked adding the complete routine list. Let's see what
>>> Fujii-san thinks at the end, there is still some time until the next
>>> round of minor releases.
>>
>> What I think is the patch I attached. Thought?
>
> That's OK for me. Thanks.

+1 from me.

Regards,

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

Re: [HACKERS] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

[Andres Freund]

Hi,

On 2017-11-13 19:03:41 -0800, Andres Freund wrote:
> diff --git a/src/backend/access/heap/rewriteheap.c 
> b/src/backend/access/heap/rewriteheap.c
> index f93c194e182..7d163c91379 100644
> --- a/src/backend/access/heap/rewriteheap.c
> +++ b/src/backend/access/heap/rewriteheap.c
> @@ -407,7 +407,10 @@ rewrite_heap_tuple(RewriteState state,
>* While we have our hands on the tuple, we may as well freeze any
>* eligible xmin or xmax, so that future VACUUM effort can be saved.
>*/
> - heap_freeze_tuple(new_tuple->t_data, state->rs_freeze_xid,
> + heap_freeze_tuple(new_tuple->t_data,
> +   
> state->rs_old_rel->rd_rel->relfrozenxid,
> +   state->rs_old_rel->rd_rel->relminmxid,
> +   state->rs_freeze_xid,
> state->rs_cutoff_multi);

Hm. So this requires backpatching the introduction of
RewriteStateData->rs_old_rel into 9.3, which in turn requires a new
argument to begin_heap_rewrite().  It originally was added in the
logical decoding commit (i.e. 9.4).

I'm fine with that, but it could theoretically cause issues for somebody
with an extension that calls begin_heap_rewrite() - which seems unlikely
and I couldn't find any that does so.

Does anybody have a problem with that?

Regards,

Andres

Re: Top-N sorts verses parallelism

[Thomas Munro]

On Fri, Dec 15, 2017 at 10:05 AM, Jeff Janes  wrote:
> On Tue, Dec 12, 2017 at 10:46 PM, Thomas Munro
>  wrote:
>>
>> Hi hackers,
>>
>> The start-up cost of bounded (top-N) sorts is sensitive at the small
>> end of N, and the
>> comparison_cost * tuples * LOG2(2.0 * output_tuples) curve doesn't
>> seem to correspond to reality.  Here's a contrived example that comes
>> from a benchmark:
>>
>>   create table foo as
>> select generate_series(1, 100)::int a, (random() * 1000)::int b;
>>   analyze foo;
>>   select * from foo order by b limit N;
>
> This looks like a costing bug.  The estimated cost of sorting 416,667
> estimated tuples in one parallel worker is almost identical to the estimated
> cost of sorting 1,000,000 tuples when parallelism is turned off.  Adding
> some logging to the cost_sort function, it looks like the limit does not get
> sent down for the parallel estimate:
>
> NOTICE:  JJ cost_sort tuples 100.00, limit 61.00, sort_mem 65536
> NOTICE:  JJ cost_sort tuples 416667.00, limit -1.00, sort_mem 65536
>
> So the LIMIT gets passed down for the execution step, but not for the
> planning step.

Oh, well spotted.  I was looking in the wrong place.  Maybe the fix is
as simple as the attached?  It certainly helps in the cases I tested,
including with wider tables.

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


gather-merge-limit.patch
Description: Binary data

Re: Using ProcSignal to get memory context stats from a running backend

[Greg Stark]

Another simpler option would be to open up a new file in the log
directory something like "debug_dump..txt" and print whatever you
want there. Then print out a reasonable size log entry saying "Debug
dump output to file 'debug_dump..txt'". You could provide a
function that reads such files out of the log directory or just
document how to access them using the pg_read_file().

It's not perfect but it's got most of the advantages of communicating
with the requesting process without the complexities of a DSM segment
and it's a bit more flexible too. Users can have automated monitoring
tools watch for dumps for example. And regular tools can be used to
back up and clean out old files.

pearltidy source code has been removed (pgindent)

[Jordan Deitch]

Hi hackers,

I am unable to build pgindent as it appears the pearltidy source has been
removed from sourceforge:

see prerequisite 2:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/pgindent/README;hb=HEAD

Thanks,
Jordan Deitch

Re: pearltidy source code has been removed (pgindent)

[Andrew Dunstan]

On 12/14/2017 08:37 PM, Jordan Deitch wrote:
> Hi hackers,
>
> I am unable to build pgindent as it appears the pearltidy source has
> been removed from sourceforge:
>
> see prerequisite 2:
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/pgindent/README;hb=HEAD
>
>



I just downloaded it from there.

cheers

andrew

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

Re: pearltidy source code has been removed (pgindent)

[Tom Lane]

Andrew Dunstan  writes:
> On 12/14/2017 08:37 PM, Jordan Deitch wrote:
>> I am unable to build pgindent as it appears the pearltidy source has
>> been removed from sourceforge:
>> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/pgindent/README;hb=HEAD

> I just downloaded it from there.

The 20090616 release we recommend?  I don't see it there either:

https://sourceforge.net/projects/perltidy/files/

Maybe it's time to update to a less hoary version?

In any case, you don't need perltidy unless you intend to reindent
the Perl code ...

regards, tom lane

Re: [HACKERS] [COMMITTERS] pgsql: Fix freezing of a dead HOT-updated tuple

[Andres Freund]

On 2017-12-14 17:00:29 -0800, Andres Freund wrote:
> Hi,
> 
> On 2017-11-13 19:03:41 -0800, Andres Freund wrote:
> > diff --git a/src/backend/access/heap/rewriteheap.c 
> > b/src/backend/access/heap/rewriteheap.c
> > index f93c194e182..7d163c91379 100644
> > --- a/src/backend/access/heap/rewriteheap.c
> > +++ b/src/backend/access/heap/rewriteheap.c
> > @@ -407,7 +407,10 @@ rewrite_heap_tuple(RewriteState state,
> >  * While we have our hands on the tuple, we may as well freeze any
> >  * eligible xmin or xmax, so that future VACUUM effort can be saved.
> >  */
> > -   heap_freeze_tuple(new_tuple->t_data, state->rs_freeze_xid,
> > +   heap_freeze_tuple(new_tuple->t_data,
> > + 
> > state->rs_old_rel->rd_rel->relfrozenxid,
> > + state->rs_old_rel->rd_rel->relminmxid,
> > + state->rs_freeze_xid,
> >   state->rs_cutoff_multi);
> 
> Hm. So this requires backpatching the introduction of
> RewriteStateData->rs_old_rel into 9.3, which in turn requires a new
> argument to begin_heap_rewrite().  It originally was added in the
> logical decoding commit (i.e. 9.4).
> 
> I'm fine with that, but it could theoretically cause issues for somebody
> with an extension that calls begin_heap_rewrite() - which seems unlikely
> and I couldn't find any that does so.
> 
> Does anybody have a problem with that?

Pushed this way.  Moved some more relfrozenxid/relminmxid tests outside
of the cutoff changes, polished some error messages.


Alvaro, Michael, Peter, and everyone else I'd greatly appreciate if you
could have a look at the backported version, just about everything but
v10 had conflicts, some of them not insubstantial.

Greetings,

Andres Freund

Re: Using ProcSignal to get memory context stats from a running backend

[Craig Ringer]

On 15 December 2017 at 09:24, Greg Stark  wrote:

> Another simpler option would be to open up a new file in the log
> directory


... if we have one.

We might be logging to syslog, or whatever else.

I'd rather keep it simple(ish).

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

Re: pearltidy source code has been removed (pgindent)

[John Naylor]

On 12/15/17, Jordan Deitch  wrote:
> Hi hackers,
>
> I am unable to build pgindent as it appears the pearltidy source has been
> removed from sourceforge:

I found it here:

https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20090616.tar.gz

Re: incorrect error message, while dropping PROCEDURE

[Rushabh Lathia]

On Fri, Dec 15, 2017 at 3:32 AM, Michael Paquier 
wrote:

> On Fri, Dec 15, 2017 at 12:18 AM, Peter Eisentraut
>  wrote:
> > On 12/13/17 23:31, Rushabh Lathia wrote:
> >> PFA patch, where introduced new AclObjectKind (ACL_KIND_PROCEDURE),
> >> msg for the new AclObjectKind, and passed it through at
> >> appropriate places.
> >
> > Yeah, that's a way to do it, but having both ACL_KIND_PROC and
> > ACL_KIND_PROCEDURE is clearly confusing.  The above-mentioned patch
> > cleans that up more thoroughly, I think.
>
> +1. I looked at the patch of Peter, as well as this one, and I prefer
> Peter's approach of reducing duplications in concepts.
>

Yes, I also agree.

Thanks,


-- 
Rushabh Lathia

Re: pearltidy source code has been removed (pgindent)

[David Fetter]

On Thu, Dec 14, 2017 at 08:55:31PM -0500, Tom Lane wrote:
> Andrew Dunstan  writes:
> > On 12/14/2017 08:37 PM, Jordan Deitch wrote:
> >> I am unable to build pgindent as it appears the pearltidy source has
> >> been removed from sourceforge:
> >> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/pgindent/README;hb=HEAD
> 
> > I just downloaded it from there.
> 
> The 20090616 release we recommend?  I don't see it there either:
> 
> https://sourceforge.net/projects/perltidy/files/
> 
> Maybe it's time to update to a less hoary version?

If this one were any younger, it would be violating causality.

http://search.cpan.org/~shancock/Perl-Tidy-20171214/lib/Perl/Tidy.pod

We can probably require one somewhere in between the current one and
that one without stressing anyone unduly.

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

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

GSoC 2018

[Stephen Frost]

Greetings -hackers,

Google Summer of Code 2018 was announced back in September and they've
changed up the timeline a bit [1].  Specifically, they moved up the
dates for things like the mentoring organization application deadline,
so it's time to start working on our Idea's page for 2018 in earnest.

The deadline for Mentoring organizations to apply is: January 23.

The list of accepted organization will be published around February 12.

Unsurprisingly, we'll need to have an Ideas page again, so I've gone
ahead and created one (copying last year's):

https://wiki.postgresql.org/wiki/GSoC_2018

Google discusses what makes a good "Ideas" list here:

https://google.github.io/gsocguides/mentor/defining-a-project-ideas-list.html

I marked all the entries with '2017' to indicate they were pulled from
last year.  If the project from last year is still relevant, please
update it to be '2018' and make sure to update all of the information
(in particular, make sure to list yourself as a mentor and remove the
other mentors, as appropriate).

New entries are certainly welcome and encouraged, just be sure to note
them as '2018' when you add it.

Projects from last year which were worked on but have significant
follow-on work to be completed are absolutely welcome as well- simply
update the description appropriately and mark it as being for '2018'.

When we get closer to actually submitting our application, I'll clean
out the '2017' entries that didn't get any updates.

As a reminder, each idea on the page should be in the format that the
other entries are in and should include:

- Project title/one-line description
- Brief, 2-5 sentence, description of the project (remember, these are
  12-week projects)
- Description of programming skills needed and estimation of the
  difficulty level
- List of potential mentors
- Expected Outcomes

As with last year, please consider PostgreSQL to be an "Umbrella"
project and that anything which would be considered "PostgreSQL Family"
per the News/Announce policy [2] is likely to be acceptable as a
PostgreSQL GSoC project.

In other words, if you're a contributor or developer on barman,
pgBackRest, the PostgreSQL website (pgweb), the PgEU/PgUS website code
(pgeu-website), pgAdmin4, PostgresXL, pgbouncer, Citus, pldebugger, the
PG RPMs (pgrpms), the JDBC driver, the ODBC driver, or any of the many
other PG Family projects, please feel free to add a project for
consideration!  If we get quite a few, we can organize the page further
based on which project or maybe what skills are needed or similar.

Let's have another great year of GSoC with PostgreSQL!

Thanks!

Stephen

[1]: https://developers.google.com/open-source/gsoc/timeline
[2]: https://wiki.postgresql.org/wiki/NewsEventsApproval


signature.asc
Description: Digital signature

Re: procedures and plpgsql PERFORM

[Ashutosh Bapat]

On Thu, Dec 14, 2017 at 8:08 PM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>> We allow a function to be invoked as part of PERFORM statement in plpgsql
>> ...
>> But we do not allow a procedure to be invoked this way
>
>> Procedures fit that category and like functions, I think, we should
>> allow them be invoked directly without any quoting and CALL
>> decoration.
>
> How is that going to work?  What if the procedure tries to commit the
> current transaction?

That can happen even today if somebody uses PERFORM 'call procedure()'
and procedure tries to commit the transaction. I don't think we have
any mechanism to prevent that. If we device one, it will be equally
applicable to PERFORM procedure().

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

Re: procedures and plpgsql PERFORM

[Ashutosh Bapat]

On Thu, Dec 14, 2017 at 9:40 PM, David G. Johnston
 wrote:
> On Thu, Dec 14, 2017 at 8:22 AM, Merlin Moncure  wrote:
>>
>> On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
>> > Ashutosh Bapat  writes:
>> >> We allow a function to be invoked as part of PERFORM statement in
>> >> plpgsql
>> >> ...
>> >> But we do not allow a procedure to be invoked this way
>> >
>> >> Procedures fit that category and like functions, I think, we should
>> >> allow them be invoked directly without any quoting and CALL
>> >> decoration.
>> >
>> > How is that going to work?  What if the procedure tries to commit the
>> > current transaction?
>> >
>> > IOW, this is not merely a syntactic-sugar question.
>>
>> BTW, We've already come to (near-but good enough) consensus that
>> PERFORM syntax is really just unnecessary, and I submitted a patch to
>> make it optional (which I really need to dust off and complete).
>
>
> Except right now PERFORM doesn't exist in SQL and is a pl/pgsql keyword to
> specify a specific limited form of the SQL SELECT command.  CALL is an SQL
> command.  I don't see any real upside to allowing pl/pgsql to accept
> omission of the command tag while SQL cannot - at least not without a
> use-case describe why such syntax would be beneficial.  And likely those use
> cases would revolve around some looping variant as opposed to a single
> stand-alone, result-less, CALL.
>
> If we do keep "PERFORM" in the pl/pgsql vocab I'd consider the following
> enhancement:
> PERFORM func() => SELECT func()
> PERFORM proc() => CALL proc()

Right, that's what I am suggesting.

Furthermore the current error message is misleading:

do $$
begin perform dummy_proc(1); end; $$ language plpgsql;
ERROR:  dummy_proc(integer) is a procedure
LINE 1: SELECT dummy_proc(1)
   ^
HINT:  To call a procedure, use CALL.
QUERY:  SELECT dummy_proc(1)
CONTEXT:  PL/pgSQL function inline_code_block line 2 at PERFORM

The user never wrote SELECT dummy_proc(), it was injected by plpgsql.
Let's assume for a moment, that user infers that s/he has to use CALL
instead. Even then plpgsql doesn't support PERFORM CALL dummy_proc()
or CALL dummy_proc().

>
> I prefer Merlin's suggestion to just documenting that PERFORM is deprecated
> and works only with functions - and that to use procedures in pl/pgsql just
> use the normal SQL CALL command.  And to write: "SELECT func()" to invoke
> functions, again just like one would in an SQL script.

That would simplify it, but I don't have any opinion as to whether we
should remove PERFORM or not.

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

Re: procedures and plpgsql PERFORM

[Ashutosh Bapat]

On Thu, Dec 14, 2017 at 10:16 PM, Pavel Stehule  wrote:
>
>
> 2017-12-14 17:10 GMT+01:00 David G. Johnston :
>>
>> On Thu, Dec 14, 2017 at 8:22 AM, Merlin Moncure 
>> wrote:
>>>
>>> On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
>>> > Ashutosh Bapat  writes:
>>> >> We allow a function to be invoked as part of PERFORM statement in
>>> >> plpgsql
>>> >> ...
>>> >> But we do not allow a procedure to be invoked this way
>>> >
>>> >> Procedures fit that category and like functions, I think, we should
>>> >> allow them be invoked directly without any quoting and CALL
>>> >> decoration.
>>> >
>>> > How is that going to work?  What if the procedure tries to commit the
>>> > current transaction?
>>> >
>>> > IOW, this is not merely a syntactic-sugar question.
>>>
>>> BTW, We've already come to (near-but good enough) consensus that
>>> PERFORM syntax is really just unnecessary, and I submitted a patch to
>>> make it optional (which I really need to dust off and complete).
>>
>>
>> Except right now PERFORM doesn't exist in SQL and is a pl/pgsql keyword to
>> specify a specific limited form of the SQL SELECT command.  CALL is an SQL
>> command.  I don't see any real upside to allowing pl/pgsql to accept
>> omission of the command tag while SQL cannot - at least not without a
>> use-case describe why such syntax would be beneficial.  And likely those use
>> cases would revolve around some looping variant as opposed to a single
>> stand-alone, result-less, CALL.
>>
>> If we do keep "PERFORM" in the pl/pgsql vocab I'd consider the following
>> enhancement:
>> PERFORM func() => SELECT func()
>> PERFORM proc() => CALL proc()
>
>
> I don't like this idea - functions are not procedures - can be nice if it
> will be visible.
>

There is a certain similarly between functions and procedures which
can not be denied, both take IN/OUT arguments and except SELECT/CALL
syntax decoration they are invoked similarly. Just to note: users have
been using function with void return value till now.

If we allow SELECT to be dropped while invoking a function through
PERFORM, why not to drop CALL for procedures similarly?

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

Re: [HACKERS] replace GrantObjectType with ObjectType

[Rushabh Lathia]

On Tue, Dec 12, 2017 at 10:16 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 10/12/17 22:18, Stephen Frost wrote:
> > I'm generally supportive of this, but I'm not entirely thrilled with how
> > this ends up conflating TABLEs and RELATIONs.  From the GRANT
> > perspective, there's no distinction, and that was clear from the
> > language used and how things were handled, but the OBJECT enum has that
> > distinction.  This change just makes VIEWs be OBJECT_TABLE even though
> > they actually aren't tables and there even is an OBJECT_VIEW value.
> > This commit may be able to grok that and manage it properly, but later
> > hackers might miss that.
> >
> > I would also suggest that the naming be consistent with the other bits
> > of the GRANT system (eg: ACL_ALL_RIGHTS_NAMESPACE would be changed to
> > ACL_ALL_RIGHTS_SCHEMA, to match OBJECT_SCHEMA).
>
> OK, here is a bigger patch set that addresses these issues.  I have
> added OBJECT_RELATION to reflect the difference between TABLE and
> RELATION.  I have also renamed NAMESPACE to SCHEMA.  And then I got rid
> of AclObjectKind as well, because it's just another enum for the same
> thing.
>
> This is now a bit bigger, so I'll put it in the commit fest for detailed
> review.
>
>
I quickly look the patch and I liked the
v2-0001-Replace-GrantObjectType-with-ObjectType.patch, it's very clean
and making things much better.

I looked at another patch

About v2-0002-Replace-AclObjectKind-with-ObjectType.patch:

I noted that no_priv_msg and not_owner_msg array been removed
and code fitted the code into aclcheck_error().  Actually that
makes the code more complex then what it used to be.  I would
prefer the array rather then code been fitted into the function.



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



-- 
Rushabh Lathia

Re: procedures and plpgsql PERFORM

[Pavel Stehule]

2017-12-15 4:43 GMT+01:00 Ashutosh Bapat :

> On Thu, Dec 14, 2017 at 10:16 PM, Pavel Stehule 
> wrote:
> >
> >
> > 2017-12-14 17:10 GMT+01:00 David G. Johnston  >:
> >>
> >> On Thu, Dec 14, 2017 at 8:22 AM, Merlin Moncure 
> >> wrote:
> >>>
> >>> On Thu, Dec 14, 2017 at 8:38 AM, Tom Lane  wrote:
> >>> > Ashutosh Bapat  writes:
> >>> >> We allow a function to be invoked as part of PERFORM statement in
> >>> >> plpgsql
> >>> >> ...
> >>> >> But we do not allow a procedure to be invoked this way
> >>> >
> >>> >> Procedures fit that category and like functions, I think, we should
> >>> >> allow them be invoked directly without any quoting and CALL
> >>> >> decoration.
> >>> >
> >>> > How is that going to work?  What if the procedure tries to commit the
> >>> > current transaction?
> >>> >
> >>> > IOW, this is not merely a syntactic-sugar question.
> >>>
> >>> BTW, We've already come to (near-but good enough) consensus that
> >>> PERFORM syntax is really just unnecessary, and I submitted a patch to
> >>> make it optional (which I really need to dust off and complete).
> >>
> >>
> >> Except right now PERFORM doesn't exist in SQL and is a pl/pgsql keyword
> to
> >> specify a specific limited form of the SQL SELECT command.  CALL is an
> SQL
> >> command.  I don't see any real upside to allowing pl/pgsql to accept
> >> omission of the command tag while SQL cannot - at least not without a
> >> use-case describe why such syntax would be beneficial.  And likely
> those use
> >> cases would revolve around some looping variant as opposed to a single
> >> stand-alone, result-less, CALL.
> >>
> >> If we do keep "PERFORM" in the pl/pgsql vocab I'd consider the following
> >> enhancement:
> >> PERFORM func() => SELECT func()
> >> PERFORM proc() => CALL proc()
> >
> >
> > I don't like this idea - functions are not procedures - can be nice if it
> > will be visible.
> >
>
> There is a certain similarly between functions and procedures which
> can not be denied, both take IN/OUT arguments and except SELECT/CALL
> syntax decoration they are invoked similarly. Just to note: users have
> been using function with void return value till now.
>

No, there are significant difference between SELECT and CALL - procedure is
not a void function.


> If we allow SELECT to be dropped while invoking a function through
> PERFORM, why not to drop CALL for procedures similarly?
>

>From my perspective a PERFORM is not bad idea, because it is consistent in
PLpgSQL.

Again - I don't see more issues related to PERFORM - usually much more
terrible is different system for OUT variables. This is a problem.

Regards

Pavel

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

Re: [HACKERS] Partition-wise aggregation/grouping

[Ashutosh Bapat]

On Thu, Dec 14, 2017 at 4:01 PM, Ashutosh Bapat
 wrote:
>
> +
> +EXPLAIN (COSTS OFF)
> +SELECT a FROM pagg_tab GROUP BY a ORDER BY 1;
> +   QUERY PLAN
> +-
> + Group
> +   Group Key: pagg_tab_p1.a
> +   ->  Merge Append
> + Sort Key: pagg_tab_p1.a
> + ->  Group
> +   Group Key: pagg_tab_p1.a
> +   ->  Sort
> + Sort Key: pagg_tab_p1.a
> + ->  Seq Scan on pagg_tab_p1
> [ ... clipped ... ]
> +(19 rows)
>
> It's strange that we do not annotate partial grouping as Partial. Does not 
> look
> like a bug in your patch. Do we get similar output with parallel grouping?

I am wrong here. It's not partial grouping. It's two level grouping. I
think annotating Group as Partial would be misleading. Sorry.


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

Re: [HACKERS] Partition-wise aggregation/grouping

[Ashutosh Bapat]

On Thu, Dec 14, 2017 at 4:01 PM, Ashutosh Bapat
 wrote:
> +
> +-- Test when input relation for grouping is dummy
> +EXPLAIN (COSTS OFF)
> +SELECT c, sum(a) FROM pagg_tab WHERE 1 = 2 GROUP BY c;
> +   QUERY PLAN
> +
> + HashAggregate
> +   Group Key: pagg_tab.c
> +   ->  Result
> + One-Time Filter: false
> +(4 rows)
>
> Not part of your patch, I am wondering if we can further optimize this plan by
> converting HashAggregate to Result (One-time Filter: false) and the aggregate
> target. Just an idea.
>
This comment is also wrong. The finalization step of aggregates needs
to be executed irrespective of whether or not the underlying scan
produces any rows. It may, for example, add a constant value to the
transition result. We may apply this optimization only when none of
aggregations have finalization functions, but it may not be worth the
code.

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

Re: procedures and plpgsql PERFORM

[Pavel Stehule]

2017-12-14 8:21 GMT+01:00 Ashutosh Bapat :

> Hi,
> We allow a function to be invoked as part of PERFORM statement in plpgsql
> do $$
> begin perform pg_relation_size('t1'); end; $$ language plpgsql;
> DO
>
> But we do not allow a procedure to be invoked this way
>  create procedure dummy_proc(a int) as $$
> begin null; end;
> $$ language plpgsql;
> CREATE PROCEDURE
>
> do $$
> begin perform dummy_proc(1); end; $$ language plpgsql;
> ERROR:  dummy_proc(integer) is a procedure
> LINE 1: SELECT dummy_proc(1)
>^
> HINT:  To call a procedure, use CALL.
> QUERY:  SELECT dummy_proc(1)
> CONTEXT:  PL/pgSQL function inline_code_block line 2 at PERFORM
>
> The documentation of PERFORM [1] says
> "For any SQL command that does not return rows, for example INSERT
> without a RETURNING clause, you can execute the command within a
> PL/pgSQL function just by writing the command."
>
> Procedures fit that category and like functions, I think, we should
> allow them be invoked directly without any quoting and CALL
> decoration.
>

Why? The CALL is four chars more. It is keyword, and it reduce parser
complexity - we should not to different between routine name and variable
name.

So -1 from my for this proposal.

Regards

Pavel

>
> [1] https://www.postgresql.org/docs/devel/static/plpgsql-
> statements.html#PLPGSQL-STATEMENTS-SQL-NORESULT
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
>

Re: [HACKERS] Parallel Hash take II

[Thomas Munro]

On Thu, Dec 14, 2017 at 11:45 AM, Andres Freund  wrote:
> +   booloverflow;   /* Continuation of previous 
> chunk? */
> +   chardata[FLEXIBLE_ARRAY_MEMBER];
> +} SharedTuplestoreChunk;
>
> Ah. I was thinking we could have the 'overflow' variable be an int,
> indicating the remaining length of the oversized tuple. That'd allow us
> to skip ahead to the end of the oversized tuple in concurrent processes
> after hitting it.

Right, that is a bit better as it avoids extra read-skip cycles for
multi-overflow-chunk cases.  Done that way.

> +   if (accessor->write_pointer + 
> accessor->sts->meta_data_size >=
> +   accessor->write_end)
> +   elog(ERROR, "meta-data too long");
>
> That seems more like an Assert than a proper elog? Given that we're
> calculating size just a few lines above...

It's an error because the logic is not smart enough to split the
optional meta-data and tuple size over multiple chunks.  I have added
comments there to explain.  That error can be reached by CALL
test_sharedtuplestore(1, 1, 1, 32756, 1), but 32755 is OK.  My goal
here is to support arbitrarily large tuples, not arbitrarily large
per-tuple meta-data, since for my use case I only need 4 bytes (a hash
value).  This could be improved if required by later features
(probably anyone wanting more general meta-data would want variable
sized meta-data anyway, whereas this is fixed, and it would also be
nice if oversized tuples didn't have to start at the beginning of a
new chunk).

I fixed two nearby fencepost bugs: I made the limit that triggers that
error smaller by size(uint32) and fixed a problem when small tuples
appear after an oversize tuple in a final overflow chunk (found by
hacking the test module to create mixtures of different sized tuples).

> +   if (accessor->sts->meta_data_size > 0)
> +   memcpy(accessor->write_pointer, meta_data,
> +  accessor->sts->meta_data_size);
> +   written = accessor->write_end - 
> accessor->write_pointer -
> +   accessor->sts->meta_data_size;
> +   memcpy(accessor->write_pointer + 
> accessor->sts->meta_data_size,
> +  tuple, written);
>
> Also, shouldn't the same Assert() be here as well if you have it above?

No, when it comes to the tuple we just write as much of it as will
fit, and write the rest in the loop below.  Comments improved to make
that clear.

> +   ereport(ERROR,
> +   (errcode_for_file_access(),
> +errmsg("could not read from shared 
> tuplestore temporary file"),
> +errdetail("Short read while reading 
> meta-data")));
>
> The errdetail doesn't follow the style guide (not a sentence ending with
> .), and seems internal-ish. I'm ok with keeping it, but perhaps we
> should change all these to be errdetail_internal()? Seems pointless to
> translate all of them.

Done.

> +   LWLockAcquire(&p->lock, LW_EXCLUSIVE);
> +   eof = p->read_page >= p->npages;
> +   if (!eof)
> +   {
> +   read_page = p->read_page;
> +   p->read_page += STS_CHUNK_PAGES;
> +   }
> +   LWLockRelease(&p->lock);
>
> So if we went to the world I'm suggesting, with overflow containing the
> length till the end of the tuple, this'd probably would have to look a
> bit different.

Yeah.  I almost wanted to change it back to a spinlock but now it's
grown bigger again...

> +   if (!eof)
> +   {
> +   SharedTuplestoreChunk chunk_header;
> +
> +   /* Make sure we have the file open. */
> +   if (accessor->read_file == NULL)
> +   {
> +   charname[MAXPGPATH];
> +
> +   sts_filename(name, accessor, 
> accessor->read_participant);
> +   accessor->read_file =
> +   BufFileOpenShared(accessor->fileset, 
> name);
> +   if (accessor->read_file == NULL)
> +   elog(ERROR, "could not open temporary 
> file %s", name);
>
> Isn't this more an Assert or just not anything? There's now way
> BufFileOpenShared should ever return NULL, no?

Right.  As of commit 923e8dee this can no longer return NULL (instead
it would raise an error), so I removed this redundant check.

> +   /* If this is an overflow chunk, we skip it. */
> +   if (chunk_header.overflow)
> +   continue;
> +
> +   accessor->read_ntuples 

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Michael Paquier]

On Thu, Dec 14, 2017 at 2:49 PM, Justin Pryzby  wrote:
> On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
>> 2017-12-13 15:37 GMT+07:00 Amit Langote :
>> Patch for adding check in pg_upgrade. Going through git history, the check
>> for inconsistency in NOT NULL constraint has ben there since a long time
>> ago. In this patch the check will be applied for all old cluster version.
>> I'm not sure in which version was the release of table inheritance.
>
> Here are some spelling and grammar fixes to that patch:
>
> but nullabe in its children: nullable
> child column is not market: marked
> with adding: by adding
> and restart: and restarting
> the problem columns: the problematic columns
> 9.5, 9.6, 10: 9.5, 9.6, and 10
> restore, that will cause error.: restore phase of pg_upgrade, that would 
> cause an error.

I agree that we should do better in pg_upgrade for HEAD and
REL_10_STABLE as it is not cool to fail late in the upgrade process
especially if you are using the --link mode.

+ *  Currently pg_upgrade will fail if there are any inconsistencies in NOT NULL
+ *  constraint inheritance: In Postgres version 9.5, 9.6, 10 we can
have a column
+ *  that is NOT NULL in parent, but nullabe in its children. But during schema
+ *  restore, that will cause error.
On top of the multiple typos here, there is no need to mention
anything other than "PostgreSQL 9.6 and older versions did not add NOT
NULL constraints when a primary key was added to to a parent, so check
for inconsistent NOT NULL constraints in inheritance trees".

You seem to take a path similar to what is done with the jsonb check.
Why not. I would find cleaner to tell also the user about using ALTER
TABLE to add the proper constraints.

Note that I have not checked or tested the query in details, but
reading through the thing looks basically correct as it handles
inheritance chains with WITH RECURSIVE.

@@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
 check_for_prepared_transactions(&old_cluster);
 check_for_reg_data_type_usage(&old_cluster);
 check_for_isn_and_int8_passing_mismatch(&old_cluster);
+check_for_not_null_inheritance(&old_cluster);
The check needs indeed to happen in check_and_dump_old_cluster(), but
there is no point to check for it in servers with version 10 and
upwards, hence you should make it conditional with GET_MAJOR_VERSION()
<= 906.
-- 
Michael

Re: pgsql: Provide overflow safe integer math inline functions.

[Christoph Berg]

Re: Andres Freund 2017-12-13 <20171213173524.rjs7b3ahsong5...@alap3.anarazel.de>
> On 2017-12-13 09:37:25 -0500, Robert Haas wrote:
> > On Wed, Dec 13, 2017 at 5:10 AM, Christoph Berg  wrote:
> > > Re: Andres Freund 2017-12-13 
> > >> Provide overflow safe integer math inline functions.
> > >
> > > The new _overflow functions have buggy comments:
> > >
> > > /*
> > >  * If a - b overflows, return true, otherwise store the result of a + b 
> > > into
> > >  * *result. ^ 
> > > there
> > 
> > What's wrong with that?
> 
> After staring at it for a while, I seem to have partially mis-copied the
> note for addition to the subtraction operation...

I believe the attached is the correct version.

Christoph
diff --git a/src/include/common/int.h b/src/include/common/int.h
new file mode 100644
index e44d42f..e6907c6
*** a/src/include/common/int.h
--- b/src/include/common/int.h
*** pg_add_s16_overflow(int16 a, int16 b, in
*** 41,47 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 41,47 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a - b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_sub_s16_overflow(int16 a, int16 b, in
*** 61,67 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 61,67 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a * b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_add_s32_overflow(int32 a, int32 b, in
*** 101,107 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 101,107 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a - b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_sub_s32_overflow(int32 a, int32 b, in
*** 121,127 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 121,127 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a * b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_add_s64_overflow(int64 a, int64 b, in
*** 167,173 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 167,173 
  }
  
  /*
!  * If a - b overflows, return true, otherwise store the result of a - b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
*** pg_sub_s64_overflow(int64 a, int64 b, in
*** 193,199 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a + b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */
--- 193,199 
  }
  
  /*
!  * If a * b overflows, return true, otherwise store the result of a * b into
   * *result. The content of *result is implementation defined in case of
   * overflow.
   */

Re: [HACKERS] Parallel Hash take II

[Andres Freund]

Hi,

Looking at the main patch (v28).

First off: This looks pretty good, the code's quite readable now
(especially compared to earlier versions), the comments are good. Really
like the nodeHash split, and the always inline hackery in nodeHashjoin.
Think we're getting really really close.

  * ExecHashJoinImpl
  *
- * This function implements the Hybrid Hashjoin algorithm.
+ * This function implements the Hybrid Hashjoin algorithm.  By forcing it
+ * to be always inline many compilers are able to specialize it for
+ * parallel = true/false without repeating the code.
  *

what about adding the above explanation for the always inline?


+   /*
+* So far we have no idea whether there are any other 
participants,
+* and if so, what phase they are working on.  The only thing 
we care
+* about at this point is whether someone has already created 
the
+* SharedHashJoinBatch objects, the main hash table for batch 0 
and
+* (if necessary) the skew hash table yet.  One backend will be
+* elected to do that now if necessary.
+*/

The 'yet' sounds a bit weird in combination with the 'already'.


+ static void
+ ExecParallelHashIncreaseNumBatches(HashJoinTable hashtable)
+ ...
+   case PHJ_GROW_BATCHES_ELECTING:
+   /* Elect one participant to prepare the operation. */

That's a good chunk of code. I'm ever so slightly inclined to put that
into a separate function. But I'm not sure it'd look good. Feel entirely
free to disregard.


+ static HashJoinTuple
+ ExecParallelHashLoadTuple(HashJoinTable hashtable, MinimalTuple tuple,
+ dsa_pointer *shared)

Not really happy with the name. ExecParallelHashTableInsert() calling
ExecParallelHashLoadTuple() to insert a tuple into the hashtable doesn't
quite strike me as right; the naming similarity to
ExecParallelHashTableLoad seems problematic too.
ExecParallelHashAllocTuple() or such?

One could argue it'd not be a bad idea to keep a similar split as
dense_alloc() and memcpy() have, but I'm not really convinced by
myself. Hm.


+   case PHJ_GROW_BATCHES_ELECTING:
+   /* Elect one participant to prepare the operation. */

Imo that comment could use a one-line summary of what preparing means.


+   /*
+* We probably also need a smaller 
bucket array.  How many
+* tuples do we expect per batch, 
assuming we have only
+* half of them so far?

Makes sense, but did cost me a minute of thinking. Maybe add a short
explanation why.


+   case PHJ_GROW_BATCHES_ALLOCATING:
+   /* Wait for the above to be finished. */
+   BarrierArriveAndWait(&pstate->grow_batches_barrier,
+
WAIT_EVENT_HASH_GROW_BATCHES_ALLOCATING);
+   /* Fall through. */

Just to make sure I understand: The "empty" phase is to protect the
process of the electee doing the sizing calculations etc?  And the
reason that's not possible to do just by waiting for
PHJ_GROW_BATCHES_REPARTITIONING is that somebody could dynamically
arrive, i.e. it'd not be needed in a statically sized barrier?  Pretty
tired here, sorry ;)


+   /* reset temp memory each time to avoid leaks from qual 
expr */
+   ResetExprContext(econtext);
+
+   if (ExecQual(hjclauses, econtext))

I personally think it's better to avoid this pattern and store the
result of the ExecQual() in a variable, ResetExprContext() and then act
on the result.  No need to keep memory around for longer, and for bigger
contexts you're more likely to have all the metadata in cache.

I'd previously thought about introducing ExecQualAndReset() or such...


  * IDENTIFICATION
  *   src/backend/executor/nodeHashjoin.c
  *
+ * PARALLELISM
+ *

This is a pretty good explanation. How about adding a reference to it
from nodeHash.c's header?



+static TupleTableSlot */* return: a tuple or NULL */
+ExecHashJoin(PlanState *pstate)
+{
+   /*
+* On sufficiently smart compilers this should be inlined with the
+* parallel-aware branches removed.
+*/
+   return ExecHashJoinImpl(pstate, false);

Ah, the explanation I desired above is here. Still seems good to have a
comment at the somewhat suspicious use of always_inline.


+
+   /*
+* If we started up so late that the shared batches have been freed
+* already by ExecHashTableDetach(), then we are finished.
+*/
+   if (!DsaPointerIsValid(hashtable->parallel_state->batches))
+   return false;

This is really the only place that weird condition is 

Re: procedures and plpgsql PERFORM

[Merlin Moncure]

On Thursday, December 14, 2017, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> Hi,
> We allow a function to be invoked as part of PERFORM statement in plpgsql
> do $$
> begin perform pg_relation_size('t1'); end; $$ language plpgsql;
> DO
>
> But we do not allow a procedure to be invoked this way
>  create procedure dummy_proc(a int) as $$
> begin null; end;
> $$ language plpgsql;
> CREATE PROCEDURE
>
> do $$
> begin perform dummy_proc(1); end; $$ language plpgsql;
> ERROR:  dummy_proc(integer) is a procedure
> LINE 1: SELECT dummy_proc(1)
>^
> HINT:  To call a procedure, use CALL.
> QUERY:  SELECT dummy_proc(1)
> CONTEXT:  PL/pgSQL function inline_code_block line 2 at PERFORM
>
> The documentation of PERFORM [1] says
> "For any SQL command that does not return rows, for example INSERT
> without a RETURNING clause, you can execute the command within a
> PL/pgSQL function just by writing the command."
>
> Procedures fit that category and like functions, I think, we should
> allow them be invoked directly without any quoting and CALL
> decoration.
>

I disagree.  The SQL command is 'CALL'.  The documentation is really only
clarifying when PERFORM is explicitly required.

merlin

Re: pgbench's expression parsing & negative numbers

[Fabien COELHO]

Hello Andres,


There are some overflow checking with div and double to int cast, which were
added because of previous complaints, but which are not very useful to me.


I think handling it inconsistently is the worst of all worlds.


Hmmm... I cannot say that inconsistency is a good thing, that would not 
be consistent:-)


My 0.02€:

 - I do not think that updating pgbench arithmetic for managing integer
   overflows is worth Andres Freund time. My guess is that most
   script would not trigger client-side overflows, so the change would
   be a no-op in practice.

 - I think that pgbench has more important defects/missing features, to
   be fixed more urgently. Given the time patches spend in the cf queue,
   obviously committers disagree on this one:-)

Now ISTM that it does not harm anything to add such a feature, so fine 
with me. Maybe the global compiler option removal is worth the effort.


--
Fabien.

Re: [HACKERS] Partition-wise aggregation/grouping

[Ashutosh Bapat]

On Wed, Dec 13, 2017 at 6:37 PM, Jeevan Chalke
 wrote:
>
>
> On Tue, Dec 12, 2017 at 3:43 PM, Ashutosh Bapat
>  wrote:
>>
>> Here are review comments for 0009
>
>
> Thank you, Ashutosh for the detailed review so far.
>
> I am working on your reviews but since parallel Append is now committed,
> I need to re-base my changes over it and need to resolve the conflicts too.
>
> Once done, I will submit the new patch-set fixing these and earlier review
> comments.
>

Sure no problem. Take your time. Here's set of comments for 0008. That
ends the first read of all the patches (2nd reading for the core
changes)

+-- Also, disable parallel paths.
+SET max_parallel_workers_per_gather TO 0;

If you enable parallel aggregation for smaller data partition-wise aggregation
paths won't be chosen. I think this is the reason why you are disabling
parallel query. But we should probably explain that in a comment. Better if we
could come up testcases without disabling parallel query. Since parallel append
is now committed, may be it can help.

+
+-- Check with multiple columns in GROUP BY, order in target-list is reversed
+EXPLAIN (COSTS OFF)
+SELECT c, a, count(*) FROM pagg_tab GROUP BY a, c;
+   QUERY PLAN
+-
+ Append
+   ->  HashAggregate
+ Group Key: pagg_tab_p1.a, pagg_tab_p1.c
+ ->  Seq Scan on pagg_tab_p1
[ ... clipped ... ]
+(10 rows)

Why do we need this testcase?

+
+-- Test when input relation for grouping is dummy
+EXPLAIN (COSTS OFF)
+SELECT c, sum(a) FROM pagg_tab WHERE 1 = 2 GROUP BY c;
+   QUERY PLAN
+
+ HashAggregate
+   Group Key: pagg_tab.c
+   ->  Result
+ One-Time Filter: false
+(4 rows)

Not part of your patch, I am wondering if we can further optimize this plan by
converting HashAggregate to Result (One-time Filter: false) and the aggregate
target. Just an idea.

+
+SELECT c, sum(a) FROM pagg_tab WHERE 1 = 2 GROUP BY c;
+ c | sum
+---+-
+(0 rows)

I think we also need a case when the child input relations are marked dummy and
then the parent is marked dummy. Just use a condition with partkey = .

+
+-- Check with SORTED paths. Disable hashagg to get group aggregate

Suggest: "Test GroupAggregate paths by disabling hash aggregates."

+-- When GROUP BY clause matches with PARTITION KEY.

I don't think we need "with", and just extend the same sentence with "complete
aggregation is performed for each partition"

+-- Should choose full partition-wise aggregation path

suggest: "Should choose full partition-wise GroupAggregate plan", but I guess
with the above suggestion, this sentence is not needed.

+
+-- When GROUP BY clause not matches with PARTITION KEY.
+-- Should choose partial partition-wise aggregation path

Similar suggestions as above.

+-- No aggregates, but still able to perform partition-wise aggregates

That's a funny construction. May be "Test partition-wise grouping without any
aggregates".

We should try some output for this query.

+
+EXPLAIN (COSTS OFF)
+SELECT a FROM pagg_tab GROUP BY a ORDER BY 1;
+   QUERY PLAN
+-
+ Group
+   Group Key: pagg_tab_p1.a
+   ->  Merge Append
+ Sort Key: pagg_tab_p1.a
+ ->  Group
+   Group Key: pagg_tab_p1.a
+   ->  Sort
+ Sort Key: pagg_tab_p1.a
+ ->  Seq Scan on pagg_tab_p1
[ ... clipped ... ]
+(19 rows)

It's strange that we do not annotate partial grouping as Partial. Does not look
like a bug in your patch. Do we get similar output with parallel grouping?

+
+-- ORDERED SET within aggregate
+EXPLAIN (COSTS OFF)
+SELECT a, sum(b order by a) FROM pagg_tab GROUP BY a ORDER BY 1, 2;
+   QUERY PLAN
+
+ Sort
+   Sort Key: pagg_tab_p1.a, (sum(pagg_tab_p1.b ORDER BY pagg_tab_p1.a))
+   ->  GroupAggregate
+ Group Key: pagg_tab_p1.a
+ ->  Sort
+   Sort Key: pagg_tab_p1.a
+   ->  Append
+ ->  Seq Scan on pagg_tab_p1
+ ->  Seq Scan on pagg_tab_p2
+ ->  Seq Scan on pagg_tab_p3
+(10 rows)

pagg_tab is partitioned by column c. So, not having it in GROUP BY
itself might produce this plan if Partial parallel aggregation is expensive.
When testing negative tests like this GROUP BY should always have the partition
key.

In case of full aggregation, since all the rows that belong to the same group
come from the same partition, having an ORDER BY doesn't make any difference.
We should support such a case.

+INSERT INTO pagg_tab1 SELECT i%30, i%20 FROM generate_series(0, 299, 2) i;
+INSERT INTO pagg_tab2 SELECT i%20, i%30 FROM generate_series(0, 299, 3) i;

spaces around % operator?

+-- When GROUP BY clause matches with PARTITION KEY.
+-- Should choose full partition-wise aggregation path.

Probably we should

Re: CUBE seems a bit confused about ORDER BY

[Teodor Sigaev]

Yes.  I bet only few users have built indexes over ~> operator if any.
Ask them to reindex in the release notes seems OK for me.



Is there a good way to detect such cases? Either in pg_upgrade, so that
we can print warnings, or at least manually (which would be suitable for
release notes).


Hmm, suppose, fix should be backpatched (because now it's unusable) and 
pg_upgrade  should not do anything. Just add release note to 10.0 and 11.0


Oh, check expression is affected too, users will need to reinsert data.

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Masahiko Sawada]

On Wed, Dec 13, 2017 at 5:57 PM, Masahiko Sawada  wrote:
> On Wed, Dec 13, 2017 at 4:30 PM, Andres Freund  wrote:
>> On 2017-12-13 16:02:45 +0900, Masahiko Sawada wrote:
>>> When we add extra blocks on a relation do we access to the disk? I
>>> guess we just call lseek and write and don't access to the disk. If so
>>> the performance degradation regression might not be much.
>>
>> Usually changes in the file size require the filesystem to perform
>> metadata operations, which in turn requires journaling on most
>> FSs. Which'll often result in synchronous disk writes.
>>
>
> Thank you. I understood the reason why this measurement should use two
> different filesystems.
>

Here is the result.
I've measured the through-put with some cases on my virtual machine.
Each client loads 48k file to each different relations located on
either xfs filesystem or ext4 filesystem, for 30 sec.

Case 1: COPYs to relations on different filessystems(xfs and ext4) and
N_RELEXTLOCK_ENTS is 1024

clients = 2, avg = 296.2068
clients = 5, avg = 372.0707
clients = 10, avg = 389.8850
clients = 50, avg = 428.8050

Case 2: COPYs to relations on different filessystems(xfs and ext4) and
N_RELEXTLOCK_ENTS is 1

clients = 2, avg = 294.3633
clients = 5, avg = 358.9364
clients = 10, avg = 383.6945
clients = 50, avg = 424.3687

And the result of current HEAD is following.

clients = 2, avg = 284.9976
clients = 5, avg = 356.1726
clients = 10, avg = 375.9856
clients = 50, avg = 429.5745

In case2, the through-put got decreased compare to case 1 but it seems
to be almost same as current HEAD. Because the speed of acquiring and
releasing extension lock got x10 faster than current HEAD as I
mentioned before, the performance degradation may not have gotten
decreased than I expected even in case 2.
Since my machine doesn't have enough resources the result of clients =
50 might not be a valid result.

Regards,

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

Re: CUBE seems a bit confused about ORDER BY

[Alexander Korotkov]

On Thu, Dec 14, 2017 at 1:36 PM, Teodor Sigaev  wrote:

> Yes.  I bet only few users have built indexes over ~> operator if any.
>>> Ask them to reindex in the release notes seems OK for me.
>>>
>>>
>> Is there a good way to detect such cases? Either in pg_upgrade, so that
>> we can print warnings, or at least manually (which would be suitable for
>> release notes).
>>
>
> Hmm, suppose, fix should be backpatched (because now it's unusable) and
> pg_upgrade  should not do anything. Just add release note to 10.0 and 11.0
>
> Oh, check expression is affected too, users will need to reinsert data.


I wrote query to find both constraints and indexes depending on ~> cube
operator.

SELECT dep.classid::regclass AS class,
  CASE WHEN dep.classid = 'pg_catalog.pg_class'::regclass THEN
dep.objid::regclass::text
   WHEN dep.classid = 'pg_catalog.pg_constraint'::regclass THEN (SELECT
conname FROM pg_catalog.pg_constraint WHERE oid = dep.objid)
  ELSE NULL
  END AS name
FROM
  pg_catalog.pg_extension e
  JOIN pg_catalog.pg_depend edep ON edep.refclassid =
'pg_catalog.pg_extension'::regclass AND edep.refobjid = e.oid AND deptype =
'e' AND edep.classid = 'pg_catalog.pg_operator'::regclass
  JOIN pg_catalog.pg_operator o ON o.oid = edep.objid AND o.oprname = '~>'
  JOIN pg_catalog.pg_depend dep ON dep.refclassid =
'pg_catalog.pg_operator'::regclass AND dep.refobjid = o.oid
WHERE
  e.extname = 'cube' AND dep.classid IN
('pg_catalog.pg_constraint'::regclass, 'pg_catalog.pg_class'::regclass);

On the below data schema

create table tmp (c cube, check ((c ~> 0 > 0)));
create index tmp_idx on tmp ((c~>0));

it gives following result

 class |name
---+-
 pg_class  | tmp_idx
 pg_constraint | tmp_c_check
(2 rows)

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] [WIP] Zipfian distribution in pgbench

[Teodor Sigaev]

Thank you for all, pushed

Fabien COELHO wrote:


Note that the patch may interact with other patches which add functions to 
pgbench, so might need a rebase depending on the order in which the patch are 
applied.


Attached a minor rebase after 16827d4424.



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: CUBE seems a bit confused about ORDER BY

[Teodor Sigaev]

SELECT dep.classid::regclass AS class,
   CASE WHEN dep.classid = 'pg_catalog.pg_class'::regclass THEN 
dep.objid::regclass::text
        WHEN dep.classid = 'pg_catalog.pg_constraint'::regclass THEN (SELECT 
conname FROM pg_catalog.pg_constraint WHERE oid = dep.objid)

   ELSE NULL
   END AS name
FROM
   pg_catalog.pg_extension e
   JOIN pg_catalog.pg_depend edep ON edep.refclassid = 
'pg_catalog.pg_extension'::regclass AND edep.refobjid = e.oid AND deptype = 'e' 
AND edep.classid = 'pg_catalog.pg_operator'::regclass

   JOIN pg_catalog.pg_operator o ON o.oid = edep.objid AND o.oprname = '~>'
   JOIN pg_catalog.pg_depend dep ON dep.refclassid = 
'pg_catalog.pg_operator'::regclass AND dep.refobjid = o.oid

WHERE
   e.extname = 'cube' AND dep.classid IN ('pg_catalog.pg_constraint'::regclass, 
'pg_catalog.pg_class'::regclass);


On the below data schema

create table tmp (c cube, check ((c ~> 0 > 0)));
create index tmp_idx on tmp ((c~>0));

it gives following result

      class     |    name
---+-
  pg_class      | tmp_idx
  pg_constraint | tmp_c_check
(2 rows)


SQL-query seems too huge for release notes and isn't looking for materialized 
view (fixable) and functional indexes with function which contains this operator 
somewhere inside (not fixable by this query). I think, just words is enough.


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: [HACKERS] Surjective functional indexes

[Konstantin Knizhnik]

On 13.12.2017 14:29, Simon Riggs wrote:

On 4 December 2017 at 15:35, Konstantin Knizhnik
 wrote:

On 30.11.2017 05:02, Michael Paquier wrote:

On Wed, Sep 27, 2017 at 4:07 PM, Simon Riggs 
wrote:

On 15 September 2017 at 16:34, Konstantin Knizhnik
 wrote:


Attached please find yet another version of the patch.

Thanks. I'm reviewing it.

Two months later, this patch is still waiting for a review (you are
listed as well as a reviewer of this patch). The documentation of the
patch has conflicts, please provide a rebased version. I am moving
this patch to next CF with waiting on author as status.

Attached please find new patch with resolved documentation conflict.

I’ve not posted a review because it was my hope that I could fix up
the problems with this clever and useful patch and just commit it. I
spent 2 days trying to do that but some problems remain and I’ve run
out of time.

Patch 3 has got some additional features that on balance I don’t think
we need. Patch 1 didn’t actually work which was confusing when I tried
to go back to that.

Patch 3 Issues

* Auto tuning added 2 extra items to Stats for each relation.  That is
too high a price to pay, so we should remove that. If we can’t do
autotuning without that, please remove it.

* Patch needs a proper test case added. We shouldn’t just have a
DEBUG1 statement in there for testing - very ugly. Please rewrite
tests using functions that show how many changes have occurred during
the current  statement/transaction.

* Parameter coding was specific to that section of code. We need a
capability to parse that parameter from other locations, just as we do
with all other reloptions. It looks like it was coded that way because
of difficulties with code placement. Please solve this with generic
code, not just code that solves only the current issue. I’d personally
be happier without any option at all, but if y’all want it, then
please add the code in the right place.

* The new coding for heap_update() uses the phrase “warm”, which is
already taken by another patch. Please avoid confusing things by
re-using the same term for different things.
The use of these technical terms like projection and surjective
doesn’t seem to add anything useful to the patch

* Rename parameter to recheck_on_update

* Remove random whitespace changes in the patch

So at this stage the concept is good and works, but the patch is only
just at the prototype stage and nowhere near committable. I hope you
can correct these issues and if you do I will be happy to review and
perhaps commit.

Thanks



Attached please find new patch which fix all the reported issues except 
disabling autotune.
If you still thing that additional 16 bytes per relation in statistic is 
too high overhead, then I will also remove autotune.



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

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

 

diff --git a/src/backend/ac

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Fixed in the attached patch.


v7 needs a rebase.

Also, you might try to produce a version which is compatible with 
Robert's constraints.


--
Fabien.

Re: [HACKERS] pgbench more operators & functions

[Fabien COELHO]

Attached v16 fixes those two errors. I regenerated the documentation with the 
new xml toolchain, and made "check" overall and in pgbench.


Attached v17 is a rebase after the zipfian commit.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3..ea8f305 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -904,14 +904,32 @@ pgbench  options  d
  
   Sets variable varname to a value calculated
   from expression.
-  The expression may contain integer constants such as 5432,
+  The expression may contain the NULL constant,
+  boolean constants TRUE and FALSE,
+  integer constants such as 5432,
   double constants such as 3.14159,
   references to variables :variablename,
-  unary operators (+, -) and binary operators
-  (+, -, *, /,
-  %) with their usual precedence and associativity,
-  function calls, and
-  parentheses.
+  operators
+  with their usual SQL precedence and associativity,
+  function calls,
+  SQL CASE generic conditional
+  expressions and parentheses.
+ 
+
+ 
+  Functions and most operators return NULL on
+  NULL input.
+ 
+
+ 
+  For conditional purposes, non zero numerical values are
+  TRUE, zero numerical values and NULL
+  are FALSE.
+ 
+
+ 
+  When no final ELSE clause is provided to a
+  CASE, the default value is NULL.
  
 
  
@@ -920,6 +938,7 @@ pgbench  options  d
 \set ntellers 10 * :scale
 \set aid (1021 * random(1, 10 * :scale)) % \
(10 * :scale) + 1
+\set divx CASE WHEN :x <> 0 THEN :y/:x ELSE NULL END
 
 

@@ -996,6 +1015,177 @@ pgbench  options  d
   
  
 
+ 
+  Built-In Operators
+
+  
+   The arithmetic, bitwise, comparison and logical operators listed in
+are built into pgbench
+   and may be used in expressions appearing in
+   \set.
+  
+
+  
+   pgbench Operators by increasing precedence
+   
+
+ 
+  Operator
+  Description
+  Example
+  Result
+ 
+
+
+ 
+  OR
+  logical or
+  5 or 0
+  TRUE
+ 
+ 
+  AND
+  logical and
+  3 and 0
+  FALSE
+ 
+ 
+  NOT
+  logical not
+  not false
+  TRUE
+ 
+ 
+  IS [NOT] (NULL|TRUE|FALSE)
+  value tests
+  1 is null
+  FALSE
+ 
+ 
+  ISNULL|NOTNULL
+  null tests
+  1 notnull
+  TRUE
+ 
+ 
+  =
+  is equal
+  5 = 4
+  FALSE
+ 
+ 
+  <>
+  is not equal
+  5 <> 4
+  TRUE
+ 
+ 
+  !=
+  is not equal
+  5 != 5
+  FALSE
+ 
+ 
+  <
+  lower than
+  5 < 4
+  FALSE
+ 
+ 
+  <=
+  lower or equal
+  5 <= 4
+  FALSE
+ 
+ 
+  >
+  greater than
+  5 > 4
+  TRUE
+ 
+ 
+  >=
+  greater or equal
+  5 >= 4
+  TRUE
+ 
+ 
+  |
+  integer bitwise OR
+  1 | 2
+  3
+ 
+ 
+  #
+  integer bitwise XOR
+  1 # 3
+  2
+ 
+ 
+  &
+  integer bitwise AND
+  1 & 3
+  1
+ 
+ 
+  ~
+  integer bitwise NOT
+  ~ 1
+  -2
+ 
+ 
+  <<
+  integer bitwise shift left
+  1 << 2
+  4
+ 
+ 
+  >>
+  integer bitwise shift right
+  8 >> 2
+  2
+ 
+ 
+  +
+  addition
+  5 + 4
+  9
+ 
+ 
+  -
+  substraction
+  3 - 2.0
+  1.0
+ 
+ 
+  *
+  multiplication
+  5 * 4
+  20
+ 
+ 
+  /
+  division (integer truncates the results)
+  5 / 3
+  1
+ 
+ 
+  %
+  modulo
+  3 % 2
+  1
+ 
+ 
+  -
+  opposite
+  - 2.0
+  -2.0
+ 
+
+   
+  
+ 
+
  
   Built-In Functions
 
@@ -1042,6 +1232,13 @@ pgbench  options  d
5432.0
   
   
+   exp(x)
+   double
+   exponential
+   exp(1.0)
+   2.718281828459045
+  
+  
greatest(a [, ... ] )
double if any a is double, else integer
largest value among arguments
@@ -1063,6 +1260,20 @@ pgbench  options  d
2.1
   
   
+   ln(x)
+   double
+   natural logarithm
+   ln(2.718281828459045)
+   1.0
+  
+  
+   mod(i, bj)
+   integer
+   modulo
+   mod(54, 32)
+   22
+  
+  
pi()
double
value of the constant PI
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 25d5ad4..14b3726 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -19,13 +19,17 @@
 PgBenchExpr *expr_parse_result;
 
 static PgBenchExprList *make_elist(PgBenchExpr *exp, PgBenchExprList *list);
+static PgBenchExpr *make_null_constant(void);
+static PgBenchExpr *make_boolean_constant(bool bval);
 static PgBenchExpr *make_integer_constant(int64 ival);
 sta

Re: pgbench - add \if support

[Fabien COELHO]

Mostly a rebase after zipfian function commit.

This patch adds \if support to pgbench, similar to psql's version added in 
March.


This patch brings a consistent set of features especially when combined with 
two other patches already in the (slow) CF process:


- https://commitfest.postgresql.org/10/596/ .. /15/985/
  adds support for booleans expressions (comparisons, logical
  operators, ...). This enhanced expression engine would be useful
  to allow client-side expression in psql.

- https://commitfest.postgresql.org/10/669/ .. /15/669/
  adds support for \gset, so that pgbench can interact with a database
  and extract something into a variable, instead of discarding it.

This patch adds a \if construct so that an expression on variables, possibly 
with data coming from the database, can change the behavior of a script.


For instance, the following script, which uses features from the three 
patches, would implement TPC-B per spec (not "tpcb-like", but really as 
specified).


 \set tbid  random(1, :scale)
 \set tid  10 * (:tbid - 1) + random(1, 10)
 -- client in same branch as teller at 85%
 \if :scale = 1 OR random(0, 99) < 85
   -- same branch
   \set bid  :tbid
 \else
   -- different branch
   \set bid  1 + (:tbid + random(1, :scale - 1)) % :scale
 \endif
 \set aid  :bid * 10 + random(1, 10)
 \set delta  random(-99, 99)
 BEGIN;
 UPDATE pgbench_accounts
   SET abalance = abalance + :delta WHERE aid = :aid
   RETURNING abalance AS balance \gset
 UPDATE pgbench_tellers
   SET tbalance = tbalance + :delta WHERE tid = :tid;
 UPDATE pgbench_branches
   SET bbalance = bbalance + :delta WHERE bid = :bid;
 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime)
   VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
 END;

The patch moves the conditional stack infrastructure from psql to fe_utils, 
so that it is available to both psql & pgbench.


A partial evaluation is performed to detect structural errors (eg missing 
endif, else after else...) when the script is parsed, so that such errors 
cannot occur when a script is running.


A new automaton state is added to quickly step over false branches.

TAP tests ensure reasonable coverage of the feature.




--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3..bbc10b5 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -895,6 +895,21 @@ pgbench  options  d
   
 
   
+   
+\if expression
+\elif expression
+\else
+\endif
+
+  
+  This group of commands implements nestable conditional blocks,
+  similarly to psql's .
+  Conditional expressions are identical to those with \set,
+  with non-zero values interpreted as true.
+ 
+
+   
+

 
  \set varname expression
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fce7e3a..e88f782 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2148,7 +2148,7 @@ hello 10
   
 
 
-  
+  
 \if expression
 \elif expression
 \else
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 7ce6f60..7aa45f6 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -32,6 +32,7 @@
 #endif			/* ! WIN32 */
 
 #include "postgres_fe.h"
+#include "fe_utils/conditional.h"
 
 #include "getopt_long.h"
 #include "libpq-fe.h"
@@ -273,6 +274,9 @@ typedef enum
 	 * and we enter the CSTATE_SLEEP state to wait for it to expire. Other
 	 * meta-commands are executed immediately.
 	 *
+	 * CSTATE_SKIP_COMMAND for conditional branches which are not executed,
+	 * quickly skip commands that do not need any evaluation.
+	 *
 	 * CSTATE_WAIT_RESULT waits until we get a result set back from the server
 	 * for the current command.
 	 *
@@ -282,6 +286,7 @@ typedef enum
 	 * command counter, and loops back to CSTATE_START_COMMAND state.
 	 */
 	CSTATE_START_COMMAND,
+	CSTATE_SKIP_COMMAND,
 	CSTATE_WAIT_RESULT,
 	CSTATE_SLEEP,
 	CSTATE_END_COMMAND,
@@ -311,6 +316,7 @@ typedef struct
 	PGconn	   *con;			/* connection handle to DB */
 	int			id;/* client No. */
 	ConnectionStateEnum state;	/* state machine's current state. */
+	ConditionalStack cstack;	/* enclosing conditionals state */
 
 	int			use_file;		/* index in sql_script for this client */
 	int			command;		/* command number in script */
@@ -399,7 +405,11 @@ typedef enum MetaCommand
 	META_SET,	/* \set */
 	META_SETSHELL,/* \setshell */
 	META_SHELL,	/* \shell */
-	META_SLEEP	/* \sleep */
+	META_SLEEP,	/* \sleep */
+	META_IF,	/* \if */
+	META_ELIF,	/* \elif */
+	META_ELSE,	/* \else */
+	META_ENDIF	/* \endif */
 } MetaCommand;
 
 typedef enum QueryMode
@@ -1468,6 +1478,27 @@ coerceToDouble(PgBenchValue *pval, double *dval)
 		return true;
 	}
 }
+
+/*
+ * Return true or false for conditional purposes.
+ * Non zero numerical values are true.
+ */
+static bool
+valueTruth

Re: [HACKERS] pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL

[Ali Akbar]

2017-12-14 15:08 GMT+07:00 Michael Paquier :
>
> On Thu, Dec 14, 2017 at 2:49 PM, Justin Pryzby  wrote:
> > On Thu, Dec 14, 2017 at 08:51:06AM +0700, Ali Akbar wrote:
> >> 2017-12-13 15:37 GMT+07:00 Amit Langote :
> >> Patch for adding check in pg_upgrade. Going through git history, the check
> >> for inconsistency in NOT NULL constraint has ben there since a long time
> >> ago. In this patch the check will be applied for all old cluster version.
> >> I'm not sure in which version was the release of table inheritance.
> >
> > Here are some spelling and grammar fixes to that patch:
> >
> > but nullabe in its children: nullable
> > child column is not market: marked
> > with adding: by adding
> > and restart: and restarting
> > the problem columns: the problematic columns
> > 9.5, 9.6, 10: 9.5, 9.6, and 10
> > restore, that will cause error.: restore phase of pg_upgrade, that would 
> > cause an error.


Thanks. Typo fixed.

>
> I agree that we should do better in pg_upgrade for HEAD and
> REL_10_STABLE as it is not cool to fail late in the upgrade process
> especially if you are using the --link mode.
>
> + *  Currently pg_upgrade will fail if there are any inconsistencies in NOT 
> NULL
> + *  constraint inheritance: In Postgres version 9.5, 9.6, 10 we can
> have a column
> + *  that is NOT NULL in parent, but nullabe in its children. But during 
> schema
> + *  restore, that will cause error.
> On top of the multiple typos here, there is no need to mention
> anything other than "PostgreSQL 9.6 and older versions did not add NOT
> NULL constraints when a primary key was added to to a parent, so check
> for inconsistent NOT NULL constraints in inheritance trees".


In my case, my database becomes like that because accidental ALTER
COLUMN x DROP NOT NULL, and no, not the Primary Key.

Something like this:

CREATE TABLE parent (id SERIAL PRIMARY KEY, name VARCHAR(52) NOT NULL);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE child ALTER COLUMN name DROP NOT NULL;

And yes, in current version 10 (and HEAD), we can still do that.

Because both version currently accepts that inconsistency, ideally
pg_upgrade must be able to upgrade the cluster without problem. Hence
the comment: "Currently pg_upgrade will fail if there are any
inconsistencies in NOT NULL constraint inheritance".

> You seem to take a path similar to what is done with the jsonb check.
> Why not. I would find cleaner to tell also the user about using ALTER
> TABLE to add the proper constraints.
>
> Note that I have not checked or tested the query in details, but
> reading through the thing looks basically correct as it handles
> inheritance chains with WITH RECURSIVE.

Any suggestion to make it more readable? Maybe by separating column
query with schema & table name by using another CTE?

> @@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
>  check_for_prepared_transactions(&old_cluster);
>  check_for_reg_data_type_usage(&old_cluster);
>  check_for_isn_and_int8_passing_mismatch(&old_cluster);
> +check_for_not_null_inheritance(&old_cluster);
> The check needs indeed to happen in check_and_dump_old_cluster(), but
> there is no point to check for it in servers with version 10 and
> upwards, hence you should make it conditional with GET_MAJOR_VERSION()
> <= 906.

No, currently it can happen again in version 10 (and above) because of
the problem above.

By the way, should i add this patch to the current commitfest?

Thanks,
Ali Akbar
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1b9083597c..0bd2cd0ed1 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -26,6 +26,7 @@ static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
 static void check_for_reg_data_type_usage(ClusterInfo *cluster);
 static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
 static void check_for_pg_role_prefix(ClusterInfo *cluster);
+static void check_for_not_null_inheritance(ClusterInfo *cluster);
 static char *get_canonical_locale_name(int category, const char *locale);
 
 
@@ -99,6 +100,7 @@ check_and_dump_old_cluster(bool live_check)
 	check_for_prepared_transactions(&old_cluster);
 	check_for_reg_data_type_usage(&old_cluster);
 	check_for_isn_and_int8_passing_mismatch(&old_cluster);
+	check_for_not_null_inheritance(&old_cluster);
 
 	/*
 	 * Pre-PG 10 allowed tables with 'unknown' type columns and non WAL logged
@@ -1096,6 +1098,105 @@ check_for_pg_role_prefix(ClusterInfo *cluster)
 	check_ok();
 }
 
+/*
+ * check_for_not_null_inheritance()
+ *
+ *  Currently pg_upgrade will fail if there are any inconsistencies in NOT NULL
+ *  constraint inheritance: In PostgreSQL, we can have a column that is NOT NULL
+ *  in parent, but nullable in its children. So check for inconsistent NOT NULL
+ *  constraints in inheritance tree.
+ */
+static void
+check_for_not_null_inheritance(ClusterInfo *cluster)
+{
+	int			dbnum;
+	FILE	   *script = NULL;
+	bool		found = false;
+	char		out

Re: [HACKERS] pgbench more operators & functions

[Teodor Sigaev]

Huh, you are fast. Rebase patch during half an hour.

I haven't objection about patch idea, but I see some gotchas in coding.

1) /* Try to convert variable to numeric form; return false on failure */
static bool
makeVariableValue(Variable *var)

as now, makeVariableValue() convert value of eny type, not only numeric

2) In makeVariableValue():
if (pg_strcasecmp(var->svalue, "null") == 0)
...
else if (pg_strncasecmp(var->svalue, "true", slen)

mixing of pg_strcasecmp and pg_strNcasecmp. And, IMHO, result of
pg_strncasecmp("tru", "true", 1) will  be 0. It may be good for 't' of 'f' but 
it seems too free grammar to accept  'tr' or 'fa' or  even 'o' which actually 
not known to be on or off.


3) It seems to me that Variable.has_value could be eliminated and then new 
PGBT_NOT_SET is added to PgBenchValueType enum as a first (=0) value. This 
allows to shorten code and make it more readable, look

setBoolValue(&var->value, true);
var->has_value = true;
The second line actually doesn't needed. Although I don't insist to fix that.

I actually prefer PGBT_NOT_SET instead of kind of PGBT_UNDEFINED, because I 
remember a collision in JavaScript with undefined and null variable.



4) valueTypeName()
it checks all possible PgBenchValue.type but believes that field could contain 
some another value. In all other cases it treats as error or assert.





Fabien COELHO wrote:


Attached v16 fixes those two errors. I regenerated the documentation with the 
new xml toolchain, and made "check" overall and in pgbench.


Attached v17 is a rebase after the zipfian commit.



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: procedures and plpgsql PERFORM

[Tom Lane]

Ashutosh Bapat  writes:
> We allow a function to be invoked as part of PERFORM statement in plpgsql
> ...
> But we do not allow a procedure to be invoked this way

> Procedures fit that category and like functions, I think, we should
> allow them be invoked directly without any quoting and CALL
> decoration.

How is that going to work?  What if the procedure tries to commit the
current transaction?

IOW, this is not merely a syntactic-sugar question.

regards, tom lane

Re: [HACKERS] pgbench more operators & functions

[Fabien COELHO]

Hello Teodor,


Huh, you are fast. Rebase patch during half an hour.


Hmmm... just lucky, and other after lunch tasks were more demanding.


I haven't objection about patch idea, but I see some gotchas in coding.

1) /* Try to convert variable to numeric form; return false on failure */
static bool
makeVariableValue(Variable *var)

as now, makeVariableValue() convert value of eny type, not only numeric


Indeed, the comments need updating. I found a few instances.


2) In makeVariableValue():
if (pg_strcasecmp(var->svalue, "null") == 0)
...
else if (pg_strncasecmp(var->svalue, "true", slen)

mixing of pg_strcasecmp and pg_strNcasecmp. And, IMHO, result of
pg_strncasecmp("tru", "true", 1) will  be 0.


Yep, but it cannot be called like that because slen == 
strlen(var->svalue).


It may be good for 't' of 'f' but it seems too free grammar to accept 
'tr' or 'fa' or even 'o' which actually not known to be on or off.


Yes, it really works like that. I tried to make something clearer than 
"src/bin/psql/variable.c". Maybe I did not succeed.


I have added a comment and use some shortened versions in tests, with 
the -D option.


3) It seems to me that Variable.has_value could be eliminated and then new 
PGBT_NOT_SET is added to PgBenchValueType enum as a first (=0) value. This 
allows to shorten code and make it more readable, look

   setBoolValue(&var->value, true);
   var->has_value = true;
The second line actually doesn't needed. Although I don't insist to fix that.


I agree that the redundancy should be avoided. I tried to keep 
"is_numeric" under some form, but there is no added value doing that.


I actually prefer PGBT_NOT_SET instead of kind of PGBT_UNDEFINED, because I 
remember a collision in JavaScript with undefined and null variable.


I used "PGBT_NO_VALUE" which seemed clearer otherwise a variable may be 
set and its value would not "not set" which would look strange.



4) valueTypeName()
it checks all possible PgBenchValue.type but believes that field could 
contain some another value. In all other cases it treats as error or assert.


Ok. Treated as an internal error with an assert.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4431fc3..ea8f305 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -904,14 +904,32 @@ pgbench  options  d
  
   Sets variable varname to a value calculated
   from expression.
-  The expression may contain integer constants such as 5432,
+  The expression may contain the NULL constant,
+  boolean constants TRUE and FALSE,
+  integer constants such as 5432,
   double constants such as 3.14159,
   references to variables :variablename,
-  unary operators (+, -) and binary operators
-  (+, -, *, /,
-  %) with their usual precedence and associativity,
-  function calls, and
-  parentheses.
+  operators
+  with their usual SQL precedence and associativity,
+  function calls,
+  SQL CASE generic conditional
+  expressions and parentheses.
+ 
+
+ 
+  Functions and most operators return NULL on
+  NULL input.
+ 
+
+ 
+  For conditional purposes, non zero numerical values are
+  TRUE, zero numerical values and NULL
+  are FALSE.
+ 
+
+ 
+  When no final ELSE clause is provided to a
+  CASE, the default value is NULL.
  
 
  
@@ -920,6 +938,7 @@ pgbench  options  d
 \set ntellers 10 * :scale
 \set aid (1021 * random(1, 10 * :scale)) % \
(10 * :scale) + 1
+\set divx CASE WHEN :x <> 0 THEN :y/:x ELSE NULL END
 
 

@@ -996,6 +1015,177 @@ pgbench  options  d
   
  
 
+ 
+  Built-In Operators
+
+  
+   The arithmetic, bitwise, comparison and logical operators listed in
+are built into pgbench
+   and may be used in expressions appearing in
+   \set.
+  
+
+  
+   pgbench Operators by increasing precedence
+   
+
+ 
+  Operator
+  Description
+  Example
+  Result
+ 
+
+
+ 
+  OR
+  logical or
+  5 or 0
+  TRUE
+ 
+ 
+  AND
+  logical and
+  3 and 0
+  FALSE
+ 
+ 
+  NOT
+  logical not
+  not false
+  TRUE
+ 
+ 
+  IS [NOT] (NULL|TRUE|FALSE)
+  value tests
+  1 is null
+  FALSE
+ 
+ 
+  ISNULL|NOTNULL
+  null tests
+  1 notnull
+  TRUE
+ 
+ 
+  =
+  is equal
+  5 = 4
+  FALSE
+ 
+ 
+  <>
+  is not equal
+  5 <> 4
+  TRUE
+ 
+ 
+  !=
+  is not equal
+  5 != 5
+  FALSE
+ 
+ 
+  <
+  lower than
+  5 < 4
+  FALSE
+ 
+ 
+  <=
+  lower or equal
+  5 <= 4
+  FALSE
+ 
+ 
+  >
+  greater than
+  5 > 4
+  TRUE
+ 
+ 
+  >=
+  greater or equal
+  5 >= 4
+  TRUE
+ 
+ 
+  |
+  integer bitwise OR
+  1 | 2
+ 

Re: PATCH: Exclude unlogged tables from base backups

[Robert Haas]

On Tue, Dec 12, 2017 at 8:48 PM, Stephen Frost  wrote:
> If the persistence is changed then the table will be written into the
> WAL, no?  All of the WAL generated during a backup (which is what we're
> talking about here) has to be replayed after the restore is done and is
> before the database is considered consistent, so none of this matters,
> as far as I can see, because the drop table or alter table logged or
> anything else will be in the WAL that ends up getting replayed.

I can't see a hole in this argument.  If we copy the init fork and
skip copying the main fork, then either we skipped copying the right
file, or the file we skipped copying will be recreated with the
correct contents during WAL replay anyway.

We could have a problem if wal_level=minimal, because then the new
file might not have been WAL-logged; but taking an online backup with
wal_level=minimal isn't supported precisely because we won't have WAL
replay to fix things up.

We would also have a problem if the missing file caused something in
recovery to croak on the grounds that the file was expected to be
there, but I don't think anything works that way; I think we just
assume missing files are an expected failure mode and silently do
nothing if asked to remove them.

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

Sun Studio 12 vs. __builtin_constant_p()

[Tom Lane]

Well, that's annoying: buildfarm member castoroides just fell over
with symptoms indicating that its compiler thinks that
__builtin_constant_p("string literal") is false, thus breaking the
check I installed in commit 9fa6f00b1 that AllocSetContextCreate's
name argument is a literal.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=castoroides&dt=2017-12-14%2013%3A03%3A05

I think removing that check would be entirely impractical, and
anyway it's sufficient if it works on "most people's" compilers.
Fortunately, buildfarm results so far suggest that most compilers
do accept this usage of __builtin_constant_p, if they know the
function at all.

What I propose to do is adjust the configure check for
__builtin_constant_p so that it explicitly checks for
__builtin_constant_p("string literal") being true, in addition
to what it tests now.  That will result in slightly worse
code optimization around ereport/elog calls on compilers that
don't have this behavior, but that seems fine to me.

regards, tom lane

Re: access/parallel.h lacks PGDLLIMPORT

[Robert Haas]

On Wed, Dec 13, 2017 at 8:19 PM, Thomas Munro
 wrote:
> I suppose that extensions are supposed to be allowed to use the
> facilities in access/parallel.h.  I noticed in passing when I wrote a
> throwaway test harness that my Windows built drone complained:
>
> test_sharedtuplestore.obj : error LNK2001: unresolved external symbol
> ParallelWorkerNumber
> [C:\projects\postgres\test_sharedtuplestore.vcxproj]
> .\Release\test_sharedtuplestore\test_sharedtuplestore.dll : fatal
> error LNK1120: 1 unresolved externals
> [C:\projects\postgres\test_sharedtuplestore.vcxproj]
>
> I suppose that all three of these might need that, if they're part of
> the API for parallel worker management:
>
> extern volatile bool ParallelMessagePending;
> extern int  ParallelWorkerNumber;
> extern bool InitializingParallelWorker;
>
> I'm less sure about the other two but at least ParallelWorkerNumber is
> essential for anything that needs to coordinate access to input/output
> arrays or similar.

I can't really think of a reason for extensions to need to access
ParallelMessagePending.  InitializingParallelWorker could be useful if
the extension is doing something strange with custom GUCs.
ParallelWorkerNumber is useful for the reason you state.  It's not
absolutely essential, because an extension can do something like what
test_shm_mq does to assign worker numbers based on order of startup,
but it's certainly useful.

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

Re: incorrect error message, while dropping PROCEDURE

[Peter Eisentraut]

On 12/13/17 23:31, Rushabh Lathia wrote:
> Currently if some one try to drop the PROCEDURE and
> it don't have privilege or it's not an owner, than error message
> still indicate object as FUNCTION.

Yes, that is actually something that is fixed by the patches proposed in
the thread "replace GrantObjectType with ObjectType".

> PFA patch, where introduced new AclObjectKind (ACL_KIND_PROCEDURE),
> msg for the new AclObjectKind, and passed it through at
> appropriate places.

Yeah, that's a way to do it, but having both ACL_KIND_PROC and
ACL_KIND_PROCEDURE is clearly confusing.  The above-mentioned patch
cleans that up more thoroughly, I think.

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

Re: [HACKERS] Custom compression methods

[Robert Haas]

On Wed, Dec 13, 2017 at 5:10 AM, Tomas Vondra
 wrote:
>> 2. If several data types can benefit from a similar approach, it has
>> to be separately implemented for each one.
>
> I don't think the current solution improves that, though. If you want to
> exploit internal features of individual data types, it pretty much
> requires code customized to every such data type.
>
> For example you can't take the tsvector compression and just slap it on
> tsquery, because it relies on knowledge of internal tsvector structure.
> So you need separate implementations anyway.

I don't think that's necessarily true.  Certainly, it's true that *if*
tsvector compression depends on knowledge of internal tsvector
structure, *then* that you can't use the implementation for anything
else (this, by the way, means that there needs to be some way for a
compression method to reject being applied to a column of a data type
it doesn't like).  However, it seems possible to imagine compression
algorithms that can work for a variety of data types, too.  There
might be a compression algorithm that is theoretically a
general-purpose algorithm but has features which are particularly
well-suited to, say, JSON or XML data, because it looks for word
boundaries to decide on what strings to insert into the compression
dictionary.

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