Re: constraint exclusion and nulls in IN (..) clause

[Ashutosh Bapat]

On Thu, Feb 1, 2018 at 12:26 PM, Amit Langote
 wrote:
> Hi.
>
> When addressing a review comment on the fast partition pruning thread [1],
> I noticed that specifying null in the IN-list will cause constraint
> exclusion to wrongly fail to refute a table's check predicate.
>
> create table foo (a int check (a = 1));
> insert into foo values (null), (1);
>
> -- ExecEvalScalarArrayOp() won't return true for any record in foo
> select * from foo where a in (null, 2);
>  a
> ---
> (0 rows)

AFAIU, that's true only when = operator is strict. For a non-strict
operator which treats two NULL values as equivalent it would return
row with NULL value.

>
> -- The null in the IN-list prevents constraint exclusion to exclude foo
> -- from being scanned in the first place
> explain (costs off) select * from foo where a in (null, 2);
>  QUERY PLAN
> -
>  Seq Scan on foo
>Filter: (a = ANY ('{NULL,2}'::integer[]))
> (2 rows)
>
> I propose a patch that teaches predtest.c to disregard any null values in
> a SAOP (i.e., the IN (..) expression) when performing constraint exclusion
> using that SAOP, because they cause predicate_refuted_by_recurse()'s logic
> to fail to conclude the refutation.  There is a rule that all items of an
> OR clause (SAOP is treated as one) must refute the predicate.  But the
> OpExpr constructed with null as its constant argument won't refute
> anything and thus will cause the whole OR clause to fail to refute the
> predicate.

A cursory look through constraint exclusion code esp.
operator_predicate_proof() doesn't show any special handling for
strict/non-strict operators. Probably that's why that function refuses
to refute/imply anything when it encounters NULLs.
1593  * We have two identical subexpressions, and two other
subexpressions that
1594  * are not identical but are both Consts; and we have commuted the
1595  * operators if necessary so that the Consts are on the
right.  We'll need
1596  * to compare the Consts' values.  If either is NULL, fail.
1597  */
1598 if (pred_const->constisnull)
1599 return false;
1600 if (clause_const->constisnull)
1601 return false;

>
> -- With the patch
> explain (costs off) select * from foo where a in (null, 2);
> QUERY PLAN
> --
>  Result
>One-Time Filter: false
> (2 rows)
>
> explain (costs off) select * from foo where a in (null, 2, 1);
>   QUERY PLAN
> ---
>  Seq Scan on foo
>Filter: (a = ANY ('{NULL,2,1}'::integer[]))
> (2 rows)
>
> Thoughts?

AFAIU, this doesn't look to be the right way to fix the problem; it
assumes that the operators are strict. Sorry, if I have misunderstood
the patch and your thoughts behind it. May be constraint exclusion
code should be taught to treat strict/non-strict operators separately.
I am not sure.

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

constraint exclusion and nulls in IN (..) clause

[Amit Langote]

Hi.

When addressing a review comment on the fast partition pruning thread [1],
I noticed that specifying null in the IN-list will cause constraint
exclusion to wrongly fail to refute a table's check predicate.

create table foo (a int check (a = 1));
insert into foo values (null), (1);

-- ExecEvalScalarArrayOp() won't return true for any record in foo
select * from foo where a in (null, 2);
 a
---
(0 rows)

-- The null in the IN-list prevents constraint exclusion to exclude foo
-- from being scanned in the first place
explain (costs off) select * from foo where a in (null, 2);
 QUERY PLAN
-
 Seq Scan on foo
   Filter: (a = ANY ('{NULL,2}'::integer[]))
(2 rows)

I propose a patch that teaches predtest.c to disregard any null values in
a SAOP (i.e., the IN (..) expression) when performing constraint exclusion
using that SAOP, because they cause predicate_refuted_by_recurse()'s logic
to fail to conclude the refutation.  There is a rule that all items of an
OR clause (SAOP is treated as one) must refute the predicate.  But the
OpExpr constructed with null as its constant argument won't refute
anything and thus will cause the whole OR clause to fail to refute the
predicate.

-- With the patch
explain (costs off) select * from foo where a in (null, 2);
QUERY PLAN
--
 Result
   One-Time Filter: false
(2 rows)

explain (costs off) select * from foo where a in (null, 2, 1);
  QUERY PLAN
---
 Seq Scan on foo
   Filter: (a = ANY ('{NULL,2,1}'::integer[]))
(2 rows)

Thoughts?

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/64241fee-09af-fe4b-a0ab-7cd739965041%40lab.ntt.co.jp
From 4e3408541da4b67c37ee5dc733c807c0708e1ba7 Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 1 Feb 2018 11:32:23 +0900
Subject: [PATCH v1] Disregard nulls in SAOP rightarg array/list during CE

---
 src/backend/optimizer/util/predtest.c | 6 ++
 src/test/regress/expected/inherit.out | 6 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/backend/optimizer/util/predtest.c 
b/src/backend/optimizer/util/predtest.c
index 8106010329..0928306c62 100644
--- a/src/backend/optimizer/util/predtest.c
+++ b/src/backend/optimizer/util/predtest.c
@@ -968,6 +968,9 @@ arrayconst_next_fn(PredIterInfo info)
 
if (state->next_elem >= state->num_elems)
return NULL;
+   /* skip nulls */
+   while (state->elem_nulls[state->next_elem])
+   state->next_elem++;
state->constexpr.constvalue = state->elem_values[state->next_elem];
state->constexpr.constisnull = state->elem_nulls[state->next_elem];
state->next_elem++;
@@ -1028,6 +1031,9 @@ arrayexpr_next_fn(PredIterInfo info)
 
if (state->next == NULL)
return NULL;
+   /* skip nulls */
+   while (IsA(state->next, Const) && ((Const *) state->next)->constisnull)
+   state->next = lnext(state->next);
lsecond(state->opexpr.args) = lfirst(state->next);
state->next = lnext(state->next);
return (Node *) &(state->opexpr);
diff --git a/src/test/regress/expected/inherit.out 
b/src/test/regress/expected/inherit.out
index a79f891da7..d56e5d25d9 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1715,11 +1715,7 @@ explain (costs off) select * from list_parted where a = 
'ab' or a in (null, 'cd'
  Append
->  Seq Scan on part_ab_cd
  Filter: (((a)::text = 'ab'::text) OR ((a)::text = ANY 
('{NULL,cd}'::text[])))
-   ->  Seq Scan on part_ef_gh
- Filter: (((a)::text = 'ab'::text) OR ((a)::text = ANY 
('{NULL,cd}'::text[])))
-   ->  Seq Scan on part_null_xy
- Filter: (((a)::text = 'ab'::text) OR ((a)::text = ANY 
('{NULL,cd}'::text[])))
-(7 rows)
+(3 rows)
 
 explain (costs off) select * from list_parted where a = 'ab';
 QUERY PLAN
-- 
2.11.0

Re: Re: BUG #15039: some question about hash index code

[Amit Kapila]

On Wed, Jan 31, 2018 at 6:14 PM, Amit Kapila  wrote:
> On Wed, Jan 31, 2018 at 5:57 PM, 自己  wrote:
>> thank you for your quick reply.
>> and i have another question, for the following code, whether exist such
>> scene : page_found is false and
>> newmapbuf is invalid, if so, may be the statement MarkBufferDirty(metabuf);
>> should be placed outside the if statement ?
>>
>
> On a quick look, your observation seems to be right and I think in
> this function we might call markbufferdirty twice for meta page which
> doesn't seem to be required.
>

Attached patch fix_markbufdirty_hash_index_v1.patch fixes the problem
by calling MarkBufferDirty at the appropriate place in the code.
However, I noticed that we might end up calling MarkBufferDirty twice
for metapage in _hash_addovflpage.  I think we can easily avoid that
as is done in patch fix_markbufdirty_hash_index_v1.1.patch.

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


fix_markbufdirty_hash_index_v1.patch
Description: Binary data


fix_markbufdirty_hash_index_v1.1.patch
Description: Binary data

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

On Wed, Jan 24, 2018 at 03:44:04PM +0900, Michael Paquier wrote:
> Good catch. Coverage reports mark those areas as empty! Similarly the
> functions for record_* are mostly not used. Some tests could be added
> for them at the same time. The four error paths of those functions are
> tested as well, which is cool to see. Even if the buildfarm explodes
> after 0002 and 0003, 0001 is still a good addition. The set looks good
> to me by the way.

OK, so those have been committed as a61116d, 0b5e33f6 and a6ef00b5 for
the record.  The last steps would be to:
- Introduce bool8 for catalogs.
- Address GinTernaryValue.
- Integrate stdbool.h.
Peter, are you planning to work on that for the next CF?
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] WIP: Restricting pg_rewind to data/wal dirs

[Michael Paquier]

On Mon, Jan 22, 2018 at 03:12:58PM -0500, Stephen Frost wrote:
> I would think the next step here, as Michael suggested very early on in
> this thread, would be to bring the exclude list and perhaps logic for
> pg_basebackup into the common code and have pg_rewind leverage that
> instead of having its own code that largely does the same and then
> adding an option to exclude additional items to that.  There's no sense
> having pg_rewind operate on files that are going to end up getting wiped
> out when recovery starts anyway.  Perhaps there's a use-case for
> overriding the exclude list with a 'include' option too, but I'm not
> convinced there is.

Me neither.  I'll look into getting something for the next commit fest.
There have been way too many complaints about how pg_rewind copies too
much data for nothing to ignore doing something (the last one about
pg_replslot data).  A good first step would be what you are writing in
the paragraph above, so I intend to do that as I am sure that it would
be a good addition.  For now I have marked the proposed patches as
returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: STATISTICS retained in CREATE TABLE ... LIKE (INCLUDING ALL)?

[David Rowley]

On 30 January 2018 at 14:14, David G. Johnston
 wrote:
> This bug has an obvious if annoying work-around and fixing the bug will
> likely cause people's code, that uses said work-around, to fail.  Breaking
> people's working code in stable release branches is generally a no-no.
>
> However, given that this was discovered 4 months after the feature was
> released suggests to me that we are justified, and community-serving, to
> back-patch this.  Put more bluntly, we can ask for more leeway in the first
> few patch releases of a new feature since more people will benefit from 5
> years of a fully-baked feature than may be harmed by said change.  We
> shouldn't abuse that but an obvious new feature bug/oversight like this
> seems reasonable.

That seems quite rational.

To prevent this getting lost I've added it to the March commitfest [1].

In the commitfest application I've classed it (for now) as a bug fix.
If that changes then we can alter it in the commitfest app.

[1] https://commitfest.postgresql.org/17/1501/


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

Re: [HACKERS] REINDEX CONCURRENTLY 2.0

[Michael Paquier]

On Wed, Jan 31, 2018 at 01:48:00AM +0100, Andreas Karlsson wrote:
> I too like the concept, but have not had the time to look into it.

This may happen at some point, for now I am marking the patch as
returned with feedback.
--
Michael


signature.asc
Description: PGP signature

Re: Changing WAL Header to reduce contention during ReserveXLogInsertLocation()

[Pavan Deolasee]

On Thu, Feb 1, 2018 at 11:05 AM, Michael Paquier 
wrote:

> On Sat, Jan 13, 2018 at 03:40:01PM +, Simon Riggs wrote:
> > The new two byte value is protected by CRC. The 2 byte value repeats
> > every 32768 WAL files. Any bit error in that value that made it appear
> > to be a current value would need to have a rare set of circumstances.
>
> If you use the two lower bytes of the segment number, then this gets
> repeated every 64k segments, no?  In terms of space this represents
> 500GB worth of WAL segments with a default segment size.  Hence the more
> PostgreSQL scales, the more there is a risk of collision, and I am
> pretty sure that there are already deployments around allocating
> hundreds of gigs worth of space for the WAL partition.  There are no
> problems of this class if using the 8-byte field xl_prev.  It seems to
> me that we don't want to take any risks here.
>

IMHO we're missing a point here. When xl_prev is changed to a 2-byte value
(as the patch suggests), the risk only comes when an old WAL file is
recycled for some future WAL file and the old and the future WAL file's
segment numbers share the same low order 16-bits. Now as the patch stands,
we take precaution to never reuse a WAL file with conflicting low order
bits.

This particular piece of the patch deals with that:

@@ -3496,6 +3495,16 @@ InstallXLogFileSegment(XLogSegNo *segno, char
*tmppath,
return false;
}
(*segno)++;
+
+   /*
+* If avoid_conflicting_walid is true, then we must not recycle
the
+* WAL files so that the old and the recycled WAL segnos have
the
+* same lower order 16-bits.
+*/
+   if (avoid_conflicting_walid &&
+   XLogSegNoToSegID(tmpsegno) == XLogSegNoToSegID(*segno))
+   (*segno)++;
+
XLogFilePath(path, ThisTimeLineID, *segno, wal_segment_size);
}
}

For example, old WAL file 00010001 will NEVER be reused
as  000100010001. So even if there are any intact old WAL
records in the recycled file, they will be detected correctly during replay
since the SegID stored in the WAL record will not match the SegID as
derived from the WAL file segment number. The replay code does this for
every WAL record it sees.

There were some concerns about bit-flipping, which may inadvertently SegID
stored in the carried over WAL record so that it now matches with the
current WAL files' SegID, but TBH I don't see why we can't trust CRC to
detect that. Because if we can't, then there would be other problems during
WAL replay.

Thanks,
Pavan

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

Re: CURRENT OF causes an error when IndexOnlyScan is used

[Yugo Nagata]

On Wed, 31 Jan 2018 21:12:51 -0500
Tom Lane  wrote:

> Yugo Nagata  writes:
> > I'm sorry the patch attached in the previous mail is broken and
> > not raises a compile error. I attached the fixed patch.
> 
> This patch is almost certainly wrong: you can't assume that the scan-level
> state matches the tuple we are currently processing at top level.  Any
> sort of delaying action, for instance a sort or materialize node in
> between, would break it.

In execCurrentOf(), when FOR UPDATE is not used, search_plan_tree() searches
through the PlanState tree for a scan node and if a sort or materialize node
(for example) is found it fails with the following error.

 ERROR cursor xxx is not a simply updatable scan of table yyy

So, I think what you concern would not occur by the patch as well as the orginal
code. However, I may be missing something. Could you explain more about this if 
so?

> 
> We need to either fix this aspect:
> 
> >> IndexOnlyScan returns a virtual tuple that doesn't have system
> >> column, so we can not get ctid in the same way of other plans.
> 
> or else disallow using IndexOnlyScan when the ctid is needed.

CURRENT OF is used after the scan is executed and a tuple is fetched,
so we can't know whether the ctid is needed or not in advance in this
case. We can raise an error message when CURRENT OF is used
for IndexOnlyScan plan, though.

Regards,

> 
>   regards, tom lane


-- 
Yugo Nagata 

Re: Changing WAL Header to reduce contention during ReserveXLogInsertLocation()

[Michael Paquier]

On Sat, Jan 13, 2018 at 03:40:01PM +, Simon Riggs wrote:
> The new two byte value is protected by CRC. The 2 byte value repeats
> every 32768 WAL files. Any bit error in that value that made it appear
> to be a current value would need to have a rare set of circumstances.

If you use the two lower bytes of the segment number, then this gets
repeated every 64k segments, no?  In terms of space this represents
500GB worth of WAL segments with a default segment size.  Hence the more
PostgreSQL scales, the more there is a risk of collision, and I am
pretty sure that there are already deployments around allocating
hundreds of gigs worth of space for the WAL partition.  There are no
problems of this class if using the 8-byte field xl_prev.  It seems to
me that we don't want to take any risks here.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] generated columns

[Michael Paquier]

On Wed, Jan 31, 2018 at 10:18:04PM +0900, Michael Paquier wrote:
> On Thu, Jan 25, 2018 at 10:26:55PM -0500, Peter Eisentraut wrote:
>> On 1/19/18 00:18, Michael Paquier wrote:
>>> +SELECT a, c FROM gtest12;  -- FIXME: should be allowed
>>> +ERROR:  permission denied for function gf1
>> 
>> This is quite hard to fix and I would like to leave this for a future
>> release.
> 
> I have been looking at that case more closely again, and on the contrary
> I would advocate that your patch is doing the *right* thing.  In short,
> if the generation expression uses a function and the user has only been
> granted access to read the values, it seems to me that it we should
> require that this user also has the right to execute the function. Would
> that be too user-unfriendly?  I think that this could avoid mistakes
> about giving access to unwanted functions when willing to just give a
> SELECT right as the function could be doing more operations.

Attached is the SQL file I used with test cases for the review.  I
forgot to attach it yesterday.

> Hm.  Identity columns and default columns are part of rowtypes. STORED
> columns can alsobe part of a row type with little effort, so in order to
> be consistent with the all the existing behaviors, it seems to me that
> virtually-generated columns should be part of it as well.  I have
> compiled in the attached SQL file how things behave with your
> patch.  Only virtually-generated columns show a blank value.

The tests used are attached.
--
Michael


generated_cases.sql
Description: application/sql


signature.asc
Description: PGP signature

Re: [HACKERS] why not parallel seq scan for slow functions

[Amit Kapila]

On Tue, Jan 30, 2018 at 3:30 AM, Robert Haas  wrote:
> On Sun, Jan 28, 2018 at 10:13 PM, Amit Kapila  wrote:
>> If we want to get rid of Gather (Merge) checks in
>> apply_projection_to_path(), then we need some way to add a projection
>> path to the subpath of gather node even if that is capable of
>> projection as we do now.  I think changing the order of applying
>> scan/join target won't address that unless we decide to do it for
>> every partial path.  Another way could be that we handle that in
>> generate_gather_paths, but I think that won't be the idle place to add
>> projection.
>>
>> If we want, we can compute the parallel-safety of scan/join target
>> once in grouping_planner and then pass it in apply_projection_to_path
>> to address your main concern.
>
> I spent some time today hacking on this; see attached.  It needs more
> work, but you can see what I have in mind.
>

I can see what you have in mind, but I think we still need to change
the parallel safety flag of the path if  *_target is not parallel safe
either inside apply_projection_to_path or may be outside where it is
called.  Basically, I am talking about below code:

@@ -2473,57 +2469,6 @@ apply_projection_to_path(PlannerInfo *root,
{
..
- else if (path->parallel_safe &&
- !is_parallel_safe(root, (Node *) target->exprs))
- {
- /*
- * We're inserting a parallel-restricted target list into a path
- * currently marked parallel-safe, so we have to mark it as no longer
- * safe.
- */
- path->parallel_safe = false;
- }
-
..
}

I can take care of dealing with this unless you think otherwise.

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

Re: Wait for parallel workers to attach

[Amit Kapila]

On Wed, Jan 31, 2018 at 9:53 PM, Robert Haas  wrote:
> On Wed, Jan 31, 2018 at 3:57 AM, Amit Kapila  wrote:
>>> * There might be some opportunity to share some of the new code with
>>> the code recently committed to WaitForParallelWorkersToFinish(). For
>>> one thing, the logic in this block could be refactored into a
>>> dedicated function that is called by both
>>> WaitForParallelWorkersToAttach() and WaitForParallelWorkersToFinish():
>>
>> I had thought about this earlier but left it as the common code was
>> too less, however as you have pointed out, I had extracted the common
>> code into a separate function.
>
> I like it better the other way, so I've changed it back in the
> attached version,
>

Okay, no problem.

> which also works over the comments fairly heavily.
>

+ * However, if the leader needs to wait for
+ * all of its workers or for a specific worker, it may want to call this
+ * function before doing so.

I think suggesting to use this API to wait "for a specific worker"
doesn't seem like a good idea as it doesn't have any such provision.
Other than that the patch looks good.

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

Re: CURRENT OF causes an error when IndexOnlyScan is used

[Tom Lane]

Yugo Nagata  writes:
> I'm sorry the patch attached in the previous mail is broken and
> not raises a compile error. I attached the fixed patch.

This patch is almost certainly wrong: you can't assume that the scan-level
state matches the tuple we are currently processing at top level.  Any
sort of delaying action, for instance a sort or materialize node in
between, would break it.

We need to either fix this aspect:

>> IndexOnlyScan returns a virtual tuple that doesn't have system
>> column, so we can not get ctid in the same way of other plans.

or else disallow using IndexOnlyScan when the ctid is needed.

regards, tom lane

Re: Add RANGE with values and exclusions clauses to the Window Functions

[David G. Johnston]

On Wed, Jan 31, 2018 at 5:06 PM, Tom Lane  wrote:

> We could imagine reimplementing WinGetFuncArgInFrame to fix this, but
> aside from the sheer inefficiency of simple fixes, I'm not very clear
> what seeking relative to WINDOW_SEEK_CURRENT should mean when the current
> row is excluded.  (Of course, the current row could have been out of frame
> before too.  Maybe we should just get rid of WINDOW_SEEK_CURRENT?)
>
>
​The exclusion clause is frame-specific and none of the three frame callers
use WINDOW_SEEK_CURRENT (only the single partition caller does).  So unless
there is an external code concern removing WINDOW_SEEK_CURRENT from being
valid for WinGetFuncArgInFrame seems straight forward and probably could be
done to remove dead code whether frame exclusion is implemented or not.
And it should remain dead since, as you say, in a frame context the current
row may not be represented even today.

The three ​callers of WinGetFuncArgInFrame don't use the isout argument;
they probably need to read that and a new isexcluded argument.  Start at
the head, loop until isout = true || isexcluded = false.

You could create a partition/frame that retains its contiguous property but
you then need to map multiple original row positions onto the single frame
rows that denote the head and tail positions for each.  This seems
considerably more bug-prone; but I don't really have a feel for how
sheer-ly inefficient the iteration would be (assuming it is even plausible).

I do think moving that decision and code to a separate patch would be a
good separation of work.

The obvious use case for me (I haven't tried hard here) would be something
like the question: compare my value to the average value of the 4 previous
and 4 subsequent records.

Implementing the standard is a plus - though agreed that the implementation
itself makes a difference.  With the iterative approach the complexity
seems manageable and performance paid for only by the user of the feature.

David J.

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Thu, 01 Feb 2018 09:48:49 +0900 (JST)
Tatsuo Ishii  wrote:

> > On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii  wrote:
> >> Looks good to me. If there's no objection, especially from Thomas
> >> Munro, I will mark this as "ready for committer".
> > 
> > No objection from me.
> 
> I marked this as "Ready for Committer".

Thank you for reviewing the patch!

Regards,

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata 

Re: list partition constraint shape

[Amit Langote]

On 2018/02/01 6:08, Robert Haas wrote:
> On Mon, Jan 29, 2018 at 1:22 AM, Amit Langote
>  wrote:
>>>  Updated patch attached.  Because I made no changes
>>> other than that, I'll make this as Ready for Committer.
>> Thanks a lot for reviewing.
> 
> Committed and back-patched to v10.

Thank you.

Regards,
Amit

Re: [HACKERS] Refactoring identifier checks to consistently use strcmp

[Michael Paquier]

On Fri, Jan 26, 2018 at 05:58:48PM -0500, Tom Lane wrote:
> Daniel Gustafsson  writes:
>> Oversight, completely missed that one.
> 
> It looks like no core code uses that macro, so it's got no effect
> unless some third-party code does ... but I changed it anyway.

Good point.  I missed this one as well.  I have double-checked the core
code and it seems to me that we are clear now.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Lockable views

[Tatsuo Ishii]

> On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii  wrote:
>> Looks good to me. If there's no objection, especially from Thomas
>> Munro, I will mark this as "ready for committer".
> 
> No objection from me.

I marked this as "Ready for Committer".

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Re: Temporary tables prevent autovacuum, leading to XID wraparound

[Masahiko Sawada]

On Thu, Feb 1, 2018 at 2:01 AM, Robert Haas  wrote:
> On Tue, Jan 30, 2018 at 7:04 PM, Tsunakawa, Takayuki
>  wrote:
>> So a simple improvement would be to assign workers fairly to databases 
>> facing a wraparound risk, as Sawada-san suggested.
>
> Is that always an improvement, or does it make some cases better and
> others worse?

I think the idea would not be an improvement, but just change the
policy. The current launcher's policy is "let's launch a new worker as
much as possible on the database that is at risk of wraparound most".
The idea I suggested makes the cases mentioned on this thread better
while perhaps making other cases worse.

To improve while keeping the current policy, we might want to use the
first idea I proposed. That is, we don't  launch a new worker on a
database impending wraparound if the last table of the database is
being vacuumed. But it needs to share new information such as what
tables exist in each database and which tables already have worker. It
might be overkill in order to deal with only such a corner case
though.

Regards,

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

Re: [HACKERS] [PATCH] Lockable views

[Thomas Munro]

On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii  wrote:
> Looks good to me. If there's no objection, especially from Thomas
> Munro, I will mark this as "ready for committer".

No objection from me.

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

Re: [HACKERS] Surjective functional indexes

[Simon Riggs]

On 10 January 2018 at 09:54, Konstantin Knizhnik
 wrote:

> (new version attached)

Why this comment?

Current implementation of projection optimization has to calculate
index expression twice
in case of hit (value of index expression is not changed) and three
times if values are different.

Old + New for check = 2
plus calculate again in index = 3
?

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

Re: Add RANGE with values and exclusions clauses to the Window Functions

[Tom Lane]

Oliver Ford  writes:
> [ 0001-window-frame-v11.patch ]

I've realized that the exclusion clause aspect of this patch is rather
badly broken.  In particular, the "seek to row" logic in
WinGetFuncArgInFrame is critically dependent on the assumption that the
rows of the frame are contiguous.  Use of an EXCLUDE option makes them
not contiguous, but that doesn't mean you can just return NULL if the
seek hits one of the excluded rows.  The way the spec is written, it's
pretty clear that e.g. first_value() should be the value from the first
row that survives all exclusions.  But as this is coded, if the first
row that'd otherwise be in frame is excluded by EXCLUDE, you'll get
NULL, not the value from the first row that isn't excluded.  An example
of getting the wrong results:

regression=# select x, first_value(x) over (order by x rows between
current row and 1 following exclude current row)
from generate_series(1,10) x;
 x  | first_value 
+-
  1 |
  2 |
  3 |
  4 |
  5 |
  6 |
  7 |
  8 |
  9 |
 10 |
(10 rows)

We could imagine reimplementing WinGetFuncArgInFrame to fix this, but
aside from the sheer inefficiency of simple fixes, I'm not very clear
what seeking relative to WINDOW_SEEK_CURRENT should mean when the current
row is excluded.  (Of course, the current row could have been out of frame
before too.  Maybe we should just get rid of WINDOW_SEEK_CURRENT?)

I'm a bit tempted to rip out the exclusion-clause support and leave the
topic to be revisited later.  It'd have been better done as a separate
patch anyhow IMO, since it seems quite orthogonal to the RANGE or GROUPS
options.  (And TBH, given the lack of field demand for it, I'm not sure
that we want to pay a complexity and performance price for it.)

regards, tom lane

Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 31 January 2018 at 21:03, Amit Langote  wrote:
> Update patch set attached.  Thanks again.

(My apologies for being slow to respond here. I've been on leave this
week and I'm off again next week. I now have a little time to reply)

Hi Amit,

Thanks for incorporating my changes into the patchset. A while ago I
was rebasing the run-time pruning patch on top of this but ran into a
few problems which are all results of my changes.

1. remove_redundant_clauses collapses the PartClause list into the
most restrictive set of clauses. This disallows multiple evaluations
of the PartitionClauseInfo during runtime pruning. I've written a
proposed fix for this and attached it.

2. PartitionClauseInfo->keyclauses is a list of PartClause which is
not a node type. This will cause _copyPartitionClauseInfo() to fail.

I'm still not quite sure the best way to fix #2 since PartClause
contains a FmgrInfo. I do have a local fix which moves PartClause to
primnodes.h and makes it a proper node type. I also added a copy
function which does not copy any of the cache fields in PartClause. It
just sets valid_cache to false. I didn't particularly think this was
the correct fix. I just couldn't think of how exactly this should be
done at the time.

The attached patch also adds the missing nodetag from
PartitionClauseInfo and also fixes up other code so as we don't memset
the node memory to zero, as that would destroy the tag. I ended up
just having extract_partition_key_clauses do the makeNode call. This
also resulted in populate_partition_clauses being renamed to
generate_partition_clauses

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


PartitionClauseInfo_reevaluation.patch
Description: Binary data

Re: WINDOW RANGE patch versus leakproofness

[Dean Rasheed]

On 31 January 2018 at 21:51, Robert Haas  wrote:
> On Wed, Jan 31, 2018 at 5:52 AM, Dean Rasheed  
> wrote:
>> On 30 January 2018 at 16:42, Tom Lane  wrote:
>>> So I'm thinking that (a) we do not need to check for leaky functions used
>>> in window support, and (b) therefore there's no need to avoid leaky
>>> behavior in in_range support functions.  Objections?
>>
>> Yes, I concur. Since window functions can only appear in the SELECT
>> target list and ORDER BY clauses, they should never appear in a qual
>> that gets considered for push down, and thus contain_leaked_vars()
>> should never see a window function.
>
> What about a query that uses window functions within a subquery?
>

A qual containing a subquery is treated as not push down safe, so it
wouldn't even be considered for pushing down into a security barrier
view. On a table with RLS, contain_leaked_vars() would see a subplan
on the restrictinfo's clause and return true, causing the restrictinfo
to be treated as leaky. So in each case, the qual with the subquery
would be executed after any security quals, regardless of what it
contained.

The contents of the subquery aren't currently examined, it just
defaults to leaky. If they were to be examined, the window function
would be found and it would still default to being leaky.

Regards,
Dean

Re: pgsql: doc: clearify trigger behavior for inheritance

[Bruce Momjian]

On Wed, Jan 31, 2018 at 05:13:41PM -0500, Robert Haas wrote:
> On Wed, Jan 31, 2018 at 5:00 PM, Bruce Momjian  wrote:
> > doc:  clarify trigger behavior for inheritance
> >
> > The previous wording added in PG 10 wasn't specific enough about the
> > behavior of statement and row triggers when using inheritance.
> 
> This may be a good change in general, but the change of "affected" to
> "effected" is bad English.

Thanks, fixed.  I struggled with that word.

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

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

Re: proposal: alternative psql commands quit and exit

[Bruce Momjian]

On Sun, Jan 28, 2018 at 06:35:06PM -0500, Bruce Momjian wrote:
> I used Robert's patch and modified it to match the ideas I had above.
> Specifically no white space can be before 'help', 'exit' or 'quit' and
> prompt_status is used to adjust the suggestion.  Here is the visible
> behavior:

So what do people want done with this patch?  Applied?  It looks good to
me.  :-)

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

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

[WIP PATCH] Index scan offset optimisation using visibility map

[Michail Nikolaev]

Hello.

WIP-Patch for optimisation of OFFSET + IndexScan using visibility map.
Patch based on idea of Maxim Boguk [1] with some inspiration from Douglas
Doole [2].
-
Everyone knows - using OFFSET (especially big) is not an good practice.
But in reality they widely used mostly for paging (because it is simple).

Typical situation is some table (for example tickets) with indexes used for
paging\sorting:

VACUUM FULL;
VACUUM ANALYZE ticket;
SET work_mem = '512MB';
SET random_page_cost = 1.0;

CREATE TABLE ticket AS
SELECT
id,
TRUNC(RANDOM() * 100 + 1) AS project_id,
NOW() + (RANDOM() * (NOW()+'365 days' - NOW())) AS created_date,
repeat((TRUNC(RANDOM() * 100 + 1)::text), 1000) as payload
FROM GENERATE_SERIES(1, 100) AS g(id);

CREATE INDEX simple_index ON ticket using btree(project_id, created_date);

And some typical query to do offset on tickets of some project with paging,
some filtration (based on index) and sorting:

SELECT * FROM ticket
WHERE project_id = ?
AND created_date > '20.06.2017'
ORDER BY created_date offset 500 limit 100;

At the current moment IndexScan node will be required to do 600 heap
fetches to execute the query.
But first 500 of them are just dropped by the NodeLimit.

The idea of the patch is to push down offset information in
ExecSetTupleBound (like it done for Top-sort) to IndexScan in case
of simple scan (without projection, reordering and qual). In such situation
we could use some kind of index only scan
(even better because we dont need index data) to avoid fetching tuples
while they are just thrown away by nodeLimit.

Patch is also availble on Github:
https://github.com/michail-nikolaev/postgres/commit/a368c3483250e4c02046d418a27091678cb963f4?diff=split
And some test here:
https://gist.github.com/michail-nikolaev/b7cbe1d6f463788407ebcaec8917d1e0

So, at the moment everything seems to work (check-world is ok too) and I
got next result for test ticket table:
| offset | master | patch
| 100 | ~1.3ms | ~0.7ms
| 1000 | ~5.6ms | ~1.1ms
| 1 | ~46.7ms | ~3.6ms

To continue development I have following questions:
0) Maybe I missed something huge...
1) Is it required to support non-mvvc (dirty) snapshots? They are not
supported for IndexOnlyScan - not sure about IndexScan.
2) Should I try to pass informaiton about such optimisation to
planner/optimizer? It is not too easy with current desigh but seems
possible.
3) If so, should I add something to EXPLAIN?
4) If so, should I add some counters to EXPLAIN ANALYZE? (for example
number of heap fetch avoided).
5) Should I add description of optimisation to
https://www.postgresql.org/docs/10/static/queries-limit.html ?
6) Maybe you have some ideas for additional tests I need to add.

Thanks a lot.

[1]
https://www.postgresql.org/message-id/CAK-MWwQpZobHfuTtHj9%2B9G%2B5%3Dck%2BaX-ANWHtBK_0_D_qHYxWuw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CADE5jYLuugnEEUsyW6Q_4mZFYTxHxaVCQmGAsF0yiY8ZDggi-w%40mail.gmail.com
*** a/src/backend/executor/execParallel.c
--- b/src/backend/executor/execParallel.c
***
*** 1303,1310  ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
  	pwcxt.seg = seg;
  	ExecParallelInitializeWorker(queryDesc->planstate, );
  
! 	/* Pass down any tuple bound */
! 	ExecSetTupleBound(fpes->tuples_needed, queryDesc->planstate);
  
  	/*
  	 * Run the plan.  If we specified a tuple bound, be careful not to demand
--- 1303,1310 
  	pwcxt.seg = seg;
  	ExecParallelInitializeWorker(queryDesc->planstate, );
  
! 	/* Pass down any tuple bound. Offset cannot be optimized because parallel execution. */
! 	ExecSetTupleBound(fpes->tuples_needed, 0, queryDesc->planstate);
  
  	/*
  	 * Run the plan.  If we specified a tuple bound, be careful not to demand
*** a/src/backend/executor/execProcnode.c
--- b/src/backend/executor/execProcnode.c
***
*** 785,793  ExecShutdownNode(PlanState *node)
   *
   * Set a tuple bound for a planstate node.  This lets child plan nodes
   * optimize based on the knowledge that the maximum number of tuples that
!  * their parent will demand is limited.  The tuple bound for a node may
!  * only be changed between scans (i.e., after node initialization or just
!  * before an ExecReScan call).
   *
   * Any negative tuples_needed value means "no limit", which should be the
   * default assumption when this is not called at all for a particular node.
--- 785,794 
   *
   * Set a tuple bound for a planstate node.  This lets child plan nodes
   * optimize based on the knowledge that the maximum number of tuples that
!  * their parent will demand is limited. Also tuples skipped may be used by
!  * child nodes to optimize retrieval of tuples which immediately skipped by
!  * parent (nodeLimit). The tuple bound for a node may only be changed 
!  * between scans (i.e., after node initialization or just before an ExecReScan call).
   *
   * Any negative tuples_needed value means "no limit", which should be the
   * default assumption when this is not called at 

Re: pgsql: doc: clearify trigger behavior for inheritance

[Robert Haas]

On Wed, Jan 31, 2018 at 5:00 PM, Bruce Momjian  wrote:
> doc:  clearify trigger behavior for inheritance
>
> The previous wording added in PG 10 wasn't specific enough about the
> behavior of statement and row triggers when using inheritance.

This may be a good change in general, but the change of "affected" to
"effected" is bad English.

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

Re: WINDOW RANGE patch versus leakproofness

[Robert Haas]

On Wed, Jan 31, 2018 at 5:52 AM, Dean Rasheed  wrote:
> On 30 January 2018 at 16:42, Tom Lane  wrote:
>> So I'm thinking that (a) we do not need to check for leaky functions used
>> in window support, and (b) therefore there's no need to avoid leaky
>> behavior in in_range support functions.  Objections?
>
> Yes, I concur. Since window functions can only appear in the SELECT
> target list and ORDER BY clauses, they should never appear in a qual
> that gets considered for push down, and thus contain_leaked_vars()
> should never see a window function.

What about a query that uses window functions within a subquery?

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

Re: A typo in error message

[Robert Haas]

On Wed, Jan 31, 2018 at 12:53 AM, Michael Paquier
 wrote:
> On Wed, Jan 31, 2018 at 08:47:57AM +0300, Alexander Lakhin wrote:
>> Please consider committing the attached patch to fix a typo in error
>> message.
>
> Yeah..

Committed.

>>   if (cxt->ofType)
>>   ereport(ERROR,
>>   
>> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> -  errmsg("identity colums are not 
>> supported on typed tables")));
>> +  errmsg("identity columns are not 
>> supported on typed tables")));
>>   if (cxt->partbound)
>>   ereport(ERROR,
>>   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>
> The indentation for this ereport() is weird as well here.

Hit it with pgindent to fix this, too.

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

Re: Documentation of pgcrypto AES key sizes

[Robert Haas]

On Sun, Jan 28, 2018 at 6:02 PM, Thomas Munro
 wrote:
> Added to the next CF.

Committed.  I also modified the reference in the regression test along
similar lines, and for good measure, I back-patched this change, as it
constitutes a clear error in the documentation.

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

Re: list partition constraint shape

[Robert Haas]

On Mon, Jan 29, 2018 at 1:22 AM, Amit Langote
 wrote:
>>  Updated patch attached.  Because I made no changes
>> other than that, I'll make this as Ready for Committer.
> Thanks a lot for reviewing.

Committed and back-patched to v10.

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

Re: autoprewarm is fogetting to register a tranche.

[Robert Haas]

On Fri, Dec 22, 2017 at 12:13 AM, Kyotaro HORIGUCHI
 wrote:
> The attached one-liner patch is just adding tranche registration
> to autprewarm.c.

Committed.

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

Re: JIT compiling with LLVM v9.0

[Robert Haas]

On Wed, Jan 31, 2018 at 2:49 PM, Andres Freund  wrote:
>> We could do that, but I'd be more inclined just to let JIT be
>> magically enabled.  In general, if a user could do 'yum install ip4r'
>> (for example) and have that Just Work without any further database
>> configuration, I think a lot of people would consider that to be a
>> huge improvement.  Unfortunately we can't really do that for various
>> reasons, the biggest of which is that there's no way for installing an
>> OS package to modify the internal state of a database that may not
>> even be running at the time.  But as a general principle, I think
>> having to configure both the OS and the DB is an anti-feature, and
>> that if installing an extra package is sufficient to get the
>> new-and-improved behavior, users will like it.
>
> I'm not seing a contradiction between what you describe as desired, and
> what I describe?  If it defaulted to try, that'd just do what you want,
> no? I do think it's important to configure the system so it'll error if
> JITing is not available.

Hmm, I guess that's true.  I'm not sure that we really need a way to
error out if JIT is not available, but maybe we do.

>> Bonus points if it doesn't require a server restart.
>
> I think server restart might be doable (although it'll increase memory
> usage because the shlib needs to be loaded in each backend rather than
> postmaster), but once a session is running I'm fairly sure we do not
> want to retry. Re-checking whether a shlib is available on the
> filesystem every query does not sound like a good idea...

Agreed.

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

Re: JIT compiling with LLVM v9.0

[Andres Freund]

On 2018-01-31 14:45:46 -0500, Robert Haas wrote:
> On Wed, Jan 31, 2018 at 1:34 PM, Andres Freund  wrote:
> >> The first one is a problem that's not going to go away.  If the
> >> problem of JIT being enabled "magically" is something we're concerned
> >> about, we need to figure out a good solution, not just disable the
> >> feature by default.
> >
> > That's a fair argument, and I don't really have a good answer to it. We
> > could have a jit = off/try/on, and use that to signal things? I.e. it
> > can be set to try (possibly default in version + 1), and things will
> > work if it's not installed, but if set to on it'll refuse to work if not
> > enabled. Similar to how huge pages work now.
> 
> We could do that, but I'd be more inclined just to let JIT be
> magically enabled.  In general, if a user could do 'yum install ip4r'
> (for example) and have that Just Work without any further database
> configuration, I think a lot of people would consider that to be a
> huge improvement.  Unfortunately we can't really do that for various
> reasons, the biggest of which is that there's no way for installing an
> OS package to modify the internal state of a database that may not
> even be running at the time.  But as a general principle, I think
> having to configure both the OS and the DB is an anti-feature, and
> that if installing an extra package is sufficient to get the
> new-and-improved behavior, users will like it.

I'm not seing a contradiction between what you describe as desired, and
what I describe?  If it defaulted to try, that'd just do what you want,
no? I do think it's important to configure the system so it'll error if
JITing is not available.


> Bonus points if it doesn't require a server restart.

I think server restart might be doable (although it'll increase memory
usage because the shlib needs to be loaded in each backend rather than
postmaster), but once a session is running I'm fairly sure we do not
want to retry. Re-checking whether a shlib is available on the
filesystem every query does not sound like a good idea...

Greetings,

Andres Freund

Interesting paper: Contention-Aware Lock Scheduling

[Thomas Munro]

Hi hackers,

I saw this today: http://www.vldb.org/pvldb/vol11/p648-tian.pdf

It describes the "LDSF" (largest-dependency-set-first) lock scheduling
algorithm and related work, as an alternative to the FIFO scheduling
used by PostgreSQL and most other RDBMSs.  LDSF been implemented in
MySQL 8.  The TPC-C results shown are impressive.

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

Re: [HACKERS] Partition-wise aggregation/grouping

[Robert Haas]

On Mon, Jan 29, 2018 at 3:42 AM, Jeevan Chalke
 wrote:
> Attached new patch set and rebased it on latest HEAD.

I strongly dislike add_single_path_to_append_rel.  It adds branches
and complexity to code that is already very complex.  Most
importantly, why are we adding paths to fields in
OtherUpperPathExtraData *extra instead of adding them to the path list
of some RelOptInfo?  If we had an appropriate RelOptInfo to which we
could add the paths, then we could make this simpler.

If I understand correctly, the reason you're doing it this way is
because we have no place to put partially-aggregated, non-partial
paths.  If we only needed to worry about the parallel query case, we
could just build an append of partially-aggregated paths for each
child and stick it into the grouped rel's partial pathlist, just as we
already do for regular parallel aggregation.  There's no reason why
add_paths_to_grouping_rel() needs to care about the difference a
Partial Aggregate on top of whatever and an Append each branch of
which is a Partial Aggregate on top of whatever.  However, this won't
work for non-partial paths, because add_paths_to_grouping_rel() needs
to put paths into the grouped rel's pathlist -- and we can't mix
together partially-aggregated paths and fully-aggregated paths in the
same path list.

But, really, the way we're using grouped_rel->partial_pathlist right
now is an awful hack.  What I'm thinking we could do is introduce a
new UpperRelationKind called UPPERREL_PARTIAL_GROUP_AGG, coming just
before UPPERREL_GROUP_AGG.  Without partition-wise aggregate,
partially_grouped_rel's pathlist would always be NIL, and its partial
pathlist would be constructed using the logic in
add_partial_paths_to_grouping_rel, which would need renaming.  Then,
add_paths_to_grouping_rel would use paths from input_rel when doing
non-parallel aggregation and paths from partially_grouped_rel when
doing parallel aggregation.  This would eliminate the ugly
grouped_rel->partial_pathlist = NIL assignment at the bottom of
create_grouping_paths(), because the grouped_rel's partial_pathlist
would never have been (bogusly) populated in the first place, and
hence would not need to be reset.  All of these changes could be made
via a preparatory patch.

Then the main patch needs to worry about four cases:

1. Parallel partition-wise aggregate, grouping key doesn't contain
partition key.  This should just be a matter of adding additional
Append paths to partially_grouped_rel->partial_pathlist.  The existing
code already knows how to stick a Gather and FinalizeAggregate step on
top of that, and I don't see why that logic would need any
modification or addition.  An Append of child partial-grouping paths
should be producing the same output as a partial grouping of an
Append, except that the former case might produce more separate groups
that need to be merged; but that should be OK: we can just throw all
the paths into the same path list and let the cheapest one win.

2. Parallel partition-wise aggregate, grouping key contains partition
key.  For the most part, this is no different from case #1.  We won't
have groups spanning different partitions in this case, but we might
have groups spanning different workers, so we still need a
FinalizeAggregate step.  As an exception, Gather -> Parallel Append ->
[non-partial Aggregate path] would give us a way of doing aggregation
in parallel without a separate Finalize step.  I'm not sure if we want
to consider that to be in scope for this patch.  If we do, then we'd
add the Parallel Append path to grouped_rel->partial_pathlist.  Then,
we could stick Gather (Merge) on top if it to produce a path for
grouped_rel->pathlist using generate_gather_paths(); alternatively, it
can be used by upper planning steps -- something we currently can't
ever make work with parallel aggregation.

3. Non-parallel partition-wise aggregate, grouping key contains
partition key.  Build Append paths from the children of grouped_rel
and add them to grouped_rel->pathlist.

3. Non-parallel partition-wise aggregate, grouping key doesn't contain
partition key.  Build Append paths from the children of
partially_grouped_rel and add them to partially_grouped_rel->pathlist.
Also add code to generate paths for grouped_rel->pathlist by sticking
a FinalizeAggregate step on top of each path from
partially_grouped_rel->pathlist.

Overall, what I'm trying to argue for here is making this feature look
less like its own separate thing and more like part of the general
mechanism we've already got: partial paths would turn into regular
paths via generate_gather_paths(), and partially aggregated paths
would turn into fully aggregated paths by adding FinalizeAggregate.
The existing special case that allows us to build a non-partial, fully
aggregated path from a partial, partially-aggregated path would be
preserved.

I think this would probably eliminate some other problems I see in the
existing design as well.  For 

Re: JIT compiling with LLVM v9.0

[Andres Freund]

Hi,

On 2018-01-31 11:56:59 -0500, Robert Haas wrote:
> On Tue, Jan 30, 2018 at 5:57 PM, Andres Freund  wrote:
> > Given that we need a shared library it'll be best buildsystem wise if
> > all of this is in a directory, and there's a separate file containing
> > the stubs that call into it.
> >
> > I'm not quite sure where to put the code. I'm a bit inclined to add a
> > new
> > src/backend/jit/
> > because we're dealing with code from across different categories? There
> > we could have a pgjit.c with the stubs, and llvmjit/ with the llvm
> > specific code?
> 
> That's kind of ugly, in that if we eventually end up with many
> different parts of the system using JIT, they're all going to have to
> all put their code in that directory rather than putting it with the
> subsystem to which it pertains.

Yea, that's what I really dislike about the idea too.

> On the other hand, I don't really have a better idea.

I guess one alternative would be to leave the individual files in their
subsystem directories, but not in the corresponding OBJS lists, and
instead pick them up from the makefile in the jit shlib?  That might
better...

It's a bit weird because the files would be compiled when make-ing that
directory and rather when the jit shlib one made, but that's not too
bad.


> I'd definitely at least try to keep executor-specific considerations
> in a separate FILE from general JIT infrastructure, and make, as far
> as possible, a clean separation at the API level.

Absolutely.  Right now there's general infrastructure files (error
handling, optimization, inlining), expression compilation, tuple deform
compilation, and I thought to continue keeping the files separately just
like that.

Greetings,

Andres Freund

Re: JIT compiling with LLVM v9.0

[Andres Freund]

Hi,

On 2018-01-31 11:53:25 -0500, Robert Haas wrote:
> On Wed, Jan 31, 2018 at 10:22 AM, Peter Eisentraut
>  wrote:
> > On 1/30/18 21:55, Andres Freund wrote:
> >> I'm open to changing my mind on it, but it seems a bit weird that a
> >> feature that relies on a shlib being installed magically turns itself on
> >> if avaible. And leaving that angle aside, ISTM, that it's a complex
> >> enough feature that it should be opt-in the first release... Think we
> >> roughly did that right for e.g. parallellism.
> >
> > That sounds reasonable, for both of those reasons.
> 
> The first one is a problem that's not going to go away.  If the
> problem of JIT being enabled "magically" is something we're concerned
> about, we need to figure out a good solution, not just disable the
> feature by default.

That's a fair argument, and I don't really have a good answer to it. We
could have a jit = off/try/on, and use that to signal things? I.e. it
can be set to try (possibly default in version + 1), and things will
work if it's not installed, but if set to on it'll refuse to work if not
enabled. Similar to how huge pages work now.

Greetings,

Andres Freund

Re: [HACKERS] MERGE SQL Statement for PG11

[Peter Geoghegan]

On Wed, Jan 31, 2018 at 7:17 AM, Robert Haas  wrote:
> I don't fully grok merge but suppose you have:
>
> WHEN MATCHED AND a = 0 THEN UPDATE ...
> WHEN MATCHED AND a = 1 THEN UPDATE ...
> WHEN NOT MATCHED THEN INSERT ...
>
> Suppose you match a tuple with a = 0 but, upon trying to update it,
> find that it's been updated to a = 1.  It seems like there are a few
> possible behaviors:
>
> 1. Throw an error!  I guess this is what the patch does now.

Right.

> 2. Do absolutely nothing.  I think this is what would happen with an
> ordinary UPDATE; the tuple fails the EPQ recheck and so is not
> updated, but that doesn't trigger anything else.

I think #2 is fine if you're talking about join quals. Which, of
course, you're not. These WHEN quals really do feel like
tuple-at-a-time procedural code, more than set-orientated quals (if
that wasn't true, we'd have to allow cardinality violations, which we
at least try to avoid). Simon said something like "the SQL standard
requires that WHEN quals be evaluated first" at one point, which makes
sense to me.

> 3. Fall through to the NOT MATCHED clause and try that instead.
> Allows MERGE to work as UPSERT in some simple cases, I think.

I probably wouldn't put it that way myself, FWIW.

> 4. Continue walking the chain of WHEN MATCHED items in order and test
> them against the new tuple.  This is actually pretty weird because a
> 0->1 update will fall through to the second UPDATE rule, but a 1->0
> update will fall through to the NOT MATCHED clause.

You have two WHEN MATCHED cases here, which is actually quite a
complicated example. If you didn't, then IIUC there would be no
distinction between #3 and #4.

Whether or not this "pretty weird" behavior would be weirder than
equivalent procedural code consisting of multiple (conditionally
executed) DML statements is subjective. If you imagine that the
equivalent procedural code is comprised of two UPDATE statements and
an INSERT (one statement for every WHEN case), then it's not weird, I
think, or at least is no weirder. If you imagine that it's only one
UPDATE (plus an INSERT), then is does indeed seem weirder.

I'm inclined to think of it as two UPDATE statements (that can only
affect zero or one tuples) rather than one (the statements are inside
a loop that processes many input rows, equivalent to the left side of
MERGE's join). After all, it seems very likely that one of the two
WHEN MATCHED items would actually end up containing a DELETE in real
world queries, and not another UPDATE. That's why I lean towards #4
ever so slightly right now.

> 5. Retry from the top of the chain with the updated tuple.  Could
> theoretically livelock - not sure how much of a risk that is in
> practice.

I'd say the livelock risk is non-zero, but it might still be worth it.
Isn't this like rolling back and repeating the statement in most
real-world cases?

Apparently READ COMMITTED conflict handling in a system that's similar
to Postgres occurs through a statement-level rollback. A little bird
told me that it can repeat again and again, and that there is a little
known mechanism for that to eventually error out after a fixed number
of retries. It might be desirable to emulate that in a rudimentary way
-- by implementing #5. This doesn't seem all that appealing to me
right now, though.

> Maybe there are more options?

Probably.

Minor point on semantics: There is clearly a two phase nature to WHEN
quals, which is the actual structure that Simon chose. Technically,
what you described wouldn't ever require EPQ recheck -- it might
require WHEN recheck. I think we should start being careful about
which we're talking about going forward. Hopefully Simon's MERGE wiki
page can establish a standard lexicon to talk about this stuff without
everyone becoming even more confused. That seems like it would be a
big help.

-- 
Peter Geoghegan

Re: Temporary tables prevent autovacuum, leading to XID wraparound

[Robert Haas]

On Tue, Jan 30, 2018 at 7:04 PM, Tsunakawa, Takayuki
 wrote:
> So a simple improvement would be to assign workers fairly to databases facing 
> a wraparound risk, as Sawada-san suggested.

Is that always an improvement, or does it make some cases better and
others worse?

> One ultimate solution should be the undo-based MVCC that makes vacuuming 
> unnecessary, which you proposed about a year ago...

And blogged about yesterday.

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

Re: JIT compiling with LLVM v9.0

[Robert Haas]

On Tue, Jan 30, 2018 at 5:57 PM, Andres Freund  wrote:
> Given that we need a shared library it'll be best buildsystem wise if
> all of this is in a directory, and there's a separate file containing
> the stubs that call into it.
>
> I'm not quite sure where to put the code. I'm a bit inclined to add a
> new
> src/backend/jit/
> because we're dealing with code from across different categories? There
> we could have a pgjit.c with the stubs, and llvmjit/ with the llvm
> specific code?

That's kind of ugly, in that if we eventually end up with many
different parts of the system using JIT, they're all going to have to
all put their code in that directory rather than putting it with the
subsystem to which it pertains.  On the other hand, I don't really
have a better idea.  I'd definitely at least try to keep
executor-specific considerations in a separate FILE from general JIT
infrastructure, and make, as far as possible, a clean separation at
the API level.

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

Re: [HACKERS] Bug in to_timestamp().

[Dmitry Dolgov]

> On 12 January 2018 at 13:58, Arthur Zakirov  wrote:
>
> I attached new version of the patch. Now (expected output):
>

Thanks for working on that! I haven't followed this thread before, and after a
quick review I have few side questions.

Why not write `is_separator_char` using `isprint`, `isalpha`, `isdigit` from
ctype.h? Something like:

return isprint(*str) && !isalpha(*str) && !isdigit(*str)

>From what I see in the source code they do exactly the same and tests are
successfully passing with this change.

What do you think about providing two slightly different messages for these two
situations:

if (n->type == NODE_TYPE_SPACE && !isspace((unsigned char) *s))
ereport(ERROR,
(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
 errmsg("unexpected character \"%.*s\", expected \"%s\"",
pg_mblen(s), s, n->character),
 errhint("In FX mode, punctuation in the input string "
 "must exactly match the format string.")));
/*
 * In FX mode we insist that separator character from the format
 * string matches separator character from the input string.
 */
else if (n->type == NODE_TYPE_SEPARATOR && *n->character != *s)
ereport(ERROR,
(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
 errmsg("unexpected character \"%.*s\", expected \"%s\"",
pg_mblen(s), s, n->character),
 errhint("In FX mode, punctuation in the input string "
 "must exactly match the format string.")));

E.g. "unexpected space character" and "unexpected separator character". The
difference is quite subtle, but I think a bit of context would never hurt.

Re: JIT compiling with LLVM v9.0

[Robert Haas]

On Wed, Jan 31, 2018 at 10:22 AM, Peter Eisentraut
 wrote:
> On 1/30/18 21:55, Andres Freund wrote:
>> I'm open to changing my mind on it, but it seems a bit weird that a
>> feature that relies on a shlib being installed magically turns itself on
>> if avaible. And leaving that angle aside, ISTM, that it's a complex
>> enough feature that it should be opt-in the first release... Think we
>> roughly did that right for e.g. parallellism.
>
> That sounds reasonable, for both of those reasons.

The first one is a problem that's not going to go away.  If the
problem of JIT being enabled "magically" is something we're concerned
about, we need to figure out a good solution, not just disable the
feature by default.

As far as the second one, looking back at what happened with parallel
query, I found (on a quick read) 13 back-patched commits in
REL9_6_STABLE prior to the release of 10.0, 3 of which I would qualify
as low-importance (improving documentation, fixing something that's
not really a bug, improving a test case).  A couple of those were
really stupid mistakes on my part.  On the other hand, would it have
been overall worse for our users if that feature had been turned on in
9.6?  I don't know.  They would have had those bugs (at least until we
fixed them) but they would have had parallel query, too.  It's hard
for me to judge whether that was a win or a loss, and so here.  Like
parallel query, this is a feature which seems to have a low risk of
data corruption, but a fairly high risk of wrong answers to queries
and/or strange errors.   Users don't like that.  On the other hand,
also like parallel query, if you've got the right kind of queries, it
can make them go a lot faster.  Users DO like that.

So I could go either way on whether to enable this in the first
release.  I definitely would not like to see it stay disabled by
default for a second release unless we find a lot of problems with it.
There's no point in developing new features unless users are going to
get the benefit of them, and while SOME users will enable features
that aren't turned on by default, many will not.

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

Re: [HACKERS] Can ICU be used for a database's default sort order?

[Vladimir Borodin]

Hi.

> 23 июня 2017 г., в 21:32, Peter Eisentraut  
> написал(а):
> 
> On 6/22/17 23:10, Peter Geoghegan wrote:
>> On Thu, Jun 22, 2017 at 7:10 PM, Tom Lane  wrote:
>>> Is there some way I'm missing, or is this just a not-done-yet feature?
>> 
>> It's a not-done-yet feature.
> 
> It's something I hope to address soon.

Will it work only for a particular database? Or for a whole cluster during 
initdb also? Any chance to get this done in 11?

--
May the force be with you…
https://simply.name

Re: CURRENT OF causes an error when IndexOnlyScan is used

[Yugo Nagata]

On Thu, 1 Feb 2018 01:33:49 +0900
Yugo Nagata  wrote:

I'm sorry the patch attached in the previous mail is broken and
not raises a compile error. I attached the fixed patch.

Regards,

> Hi,
> 
> I found that updating a cursor by using CURRENT OF causes the
> following error when the query is executed by IndexOnlyScan. 
> 
>  ERROR:  cannot extract system attribute from virtual tuple
> 
> IndexOnlyScan returns a virtual tuple that doesn't have system
> column, so we can not get ctid in the same way of other plans.
> However, the error message is not convinient and users would
> not understand why the error occurs.
> 
> Attached is a patch to fix this. By this fix, execCurrentOf
> get ctid from IndexScanDesc->xs_ctup.t_self when the plan is
> IndexOnlyScan, and it works sucessfully without errors.
> 
> 
> Here is the example of the error:
> 
> ===
> postgres=# create table test (i int primary key);
> CREATE TABLE
> postgres=# insert into test values(1);
> INSERT 0 1
> postgres=# set enable_seqscan to off;
> SET
> 
> postgres=# explain select * from test where i = 1;
> QUERY PLAN 
> ---
>  Index Only Scan using test_pkey on test  (cost=0.15..8.17 rows=1 width=4)
>Index Cond: (i = 1)
> (2 rows)
> 
> postgres=# begin;
> BEGIN
> postgres=# declare c cursor for select * from test where i = 1;
> DECLARE CURSOR
> postgres=# fetch from c;
>  i 
> ---
>  1
> (1 row)
> 
> postgres=# update test set i=i+1 where current of c;
> ERROR:  cannot extract system attribute from virtual tuple
> ===
> 
> The patch fixes the error and allows this update successfully.
> 
> Regards,
> 
> -- 
> Yugo Nagata 


-- 
Yugo Nagata 
diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index ce7d4ac..aa2da16 100644
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -19,6 +19,7 @@
 #include "utils/lsyscache.h"
 #include "utils/portal.h"
 #include "utils/rel.h"
+#include "access/relscan.h"
 
 
 static char *fetch_cursor_param_value(ExprContext *econtext, int paramId);
@@ -183,21 +184,35 @@ execCurrentOf(CurrentOfExpr *cexpr,
 		if (TupIsNull(scanstate->ss_ScanTupleSlot))
 			return false;
 
-		/* Use slot_getattr to catch any possible mistakes */
-		tuple_tableoid =
-			DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
-		  TableOidAttributeNumber,
-		  ));
-		Assert(!lisnull);
-		tuple_tid = (ItemPointer)
-			DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
-		 SelfItemPointerAttributeNumber,
-		 ));
-		Assert(!lisnull);
-
-		Assert(tuple_tableoid == table_oid);
-
-		*current_tid = *tuple_tid;
+		/* In IndexOnlyScan case, the tuple stored in ss_ScanTupleSlot is a
+		 * virtual tuple that does not have ctid column, so we have to get TID
+		 * from xs_ctup.t_self. */
+		if (IsA(scanstate, IndexOnlyScanState))
+		{
+			IndexScanDesc scan = ((IndexOnlyScanState *)scanstate)->ioss_ScanDesc;
+
+			Assert(RelationGetRelid(scan.heapRelation) == table_oid);
+
+			*current_tid = scan->xs_ctup.t_self;
+		}
+		else
+		{
+			/* Use slot_getattr to catch any possible mistakes */
+			tuple_tableoid =
+DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
+			  TableOidAttributeNumber,
+			  ));
+			Assert(!lisnull);
+			tuple_tid = (ItemPointer)
+DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
+			 SelfItemPointerAttributeNumber,
+			 ));
+			Assert(!lisnull);
+
+			Assert(tuple_tableoid == table_oid);
+
+			*current_tid = *tuple_tid;
+		}
 
 		return true;
 	}

CURRENT OF causes an error when IndexOnlyScan is used

[Yugo Nagata]

Hi,

I found that updating a cursor by using CURRENT OF causes the
following error when the query is executed by IndexOnlyScan. 

 ERROR:  cannot extract system attribute from virtual tuple

IndexOnlyScan returns a virtual tuple that doesn't have system
column, so we can not get ctid in the same way of other plans.
However, the error message is not convinient and users would
not understand why the error occurs.

Attached is a patch to fix this. By this fix, execCurrentOf
get ctid from IndexScanDesc->xs_ctup.t_self when the plan is
IndexOnlyScan, and it works sucessfully without errors.


Here is the example of the error:

===
postgres=# create table test (i int primary key);
CREATE TABLE
postgres=# insert into test values(1);
INSERT 0 1
postgres=# set enable_seqscan to off;
SET

postgres=# explain select * from test where i = 1;
QUERY PLAN 
---
 Index Only Scan using test_pkey on test  (cost=0.15..8.17 rows=1 width=4)
   Index Cond: (i = 1)
(2 rows)

postgres=# begin;
BEGIN
postgres=# declare c cursor for select * from test where i = 1;
DECLARE CURSOR
postgres=# fetch from c;
 i 
---
 1
(1 row)

postgres=# update test set i=i+1 where current of c;
ERROR:  cannot extract system attribute from virtual tuple
===

The patch fixes the error and allows this update successfully.

Regards,

-- 
Yugo Nagata 
diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index ce7d4ac..b37cf3d 100644
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -19,6 +19,7 @@
 #include "utils/lsyscache.h"
 #include "utils/portal.h"
 #include "utils/rel.h"
+#include "access/relscan.h"
 
 
 static char *fetch_cursor_param_value(ExprContext *econtext, int paramId);
@@ -183,21 +184,35 @@ execCurrentOf(CurrentOfExpr *cexpr,
 		if (TupIsNull(scanstate->ss_ScanTupleSlot))
 			return false;
 
-		/* Use slot_getattr to catch any possible mistakes */
-		tuple_tableoid =
-			DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
-		  TableOidAttributeNumber,
-		  ));
-		Assert(!lisnull);
-		tuple_tid = (ItemPointer)
-			DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
-		 SelfItemPointerAttributeNumber,
-		 ));
-		Assert(!lisnull);
-
-		Assert(tuple_tableoid == table_oid);
-
-		*current_tid = *tuple_tid;
+		/* In IndexOnlyScan case, the tuple stored in ss_ScanTupleSlot is a
+		 * virtual tuple that does not have ctid column, so we have to get TID
+		 * from xs_ctup.t_self. */
+		if (IsA(scanstate, IndexOnlyScanState))
+		{
+			IndexScanDesc scan = ((IndexOnlyScanState *)scanstate)->ScanDesc;
+
+			Assert(RelationGetRelid(scan.heapRelation) == table_oid);
+
+			*current_tid = scan->xs_ctup.t_self;
+		}
+		else
+		{
+			/* Use slot_getattr to catch any possible mistakes */
+			tuple_tableoid =
+DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
+			  TableOidAttributeNumber,
+			  ));
+			Assert(!lisnull);
+			tuple_tid = (ItemPointer)
+DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
+			 SelfItemPointerAttributeNumber,
+			 ));
+			Assert(!lisnull);
+
+			Assert(tuple_tableoid == table_oid);
+
+			*current_tid = *tuple_tid;
+		}
 
 		return true;
 	}

Re: Wait for parallel workers to attach

[Robert Haas]

On Wed, Jan 31, 2018 at 3:57 AM, Amit Kapila  wrote:
>> * There might be some opportunity to share some of the new code with
>> the code recently committed to WaitForParallelWorkersToFinish(). For
>> one thing, the logic in this block could be refactored into a
>> dedicated function that is called by both
>> WaitForParallelWorkersToAttach() and WaitForParallelWorkersToFinish():
>
> I had thought about this earlier but left it as the common code was
> too less, however as you have pointed out, I had extracted the common
> code into a separate function.

I like it better the other way, so I've changed it back in the
attached version, which also works over the comments fairly heavily.

> I think we should not touch anything related to Gather (merge) as they
> don't need it for the purpose of correctness.  However, we might want
> to improve them by using this new API at a certain point if the need
> arises.  I guess we can use this API to detect failures early.

I added a comment in this version explaining why it works, so that we
don't forget (again).  If we decide to change it in the future then we
can remove or update the comment.

Another thing I did was known_started_workers ->
known_attached_workers, which I think is more precisely correct.

Please let me know your thoughts about this version.  If it looks OK,
I'll commit it.

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


wait-for-attach-rmh.patch
Description: Binary data

Re: JIT compiling with LLVM v9.0

[Peter Eisentraut]

On 1/30/18 21:55, Andres Freund wrote:
> I'm open to changing my mind on it, but it seems a bit weird that a
> feature that relies on a shlib being installed magically turns itself on
> if avaible. And leaving that angle aside, ISTM, that it's a complex
> enough feature that it should be opt-in the first release... Think we
> roughly did that right for e.g. parallellism.

That sounds reasonable, for both of those reasons.

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

Re: [HACKERS] MERGE SQL Statement for PG11

[Robert Haas]

On Tue, Jan 30, 2018 at 2:28 PM, Peter Geoghegan  wrote:
> What's at issue here specifically is the exact behavior of
> EvalPlanQual() in the context of having *multiple* sets of WHEN quals
> that need to be evaluated one at a time (in addition to conventional
> EPQ join quals). This is a specific, narrow question about the exact
> steps that are taken by EPQ when we have to switch between WHEN
> MATCHED and WHEN NOT MATCHED cases *as we walk the UPDATE chain*.
>
> Right now, I suspect that we will require some minor variation of
> EPQ's logic to account for new risks. The really interesting question
> is what happens when we walk the UPDATE chain, while reevaluating EPQ
> quals alongside WHEN quals, and then determine that no UPDATE/DELETE
> should happen for the first WHEN case -- what then? I suspect that we
> may not want to start from scratch (from the MVCC-visible tuple) as we
> reach the second or subsequent WHEN case, but that's a very tentative
> view, and I definitely want to hear more opinions it. (Simon wants to
> just throw a serialization error here instead, even in READ COMMITTED
> mode, which I see as a cop-out.)

I don't fully grok merge but suppose you have:

WHEN MATCHED AND a = 0 THEN UPDATE ...
WHEN MATCHED AND a = 1 THEN UPDATE ...
WHEN NOT MATCHED THEN INSERT ...

Suppose you match a tuple with a = 0 but, upon trying to update it,
find that it's been updated to a = 1.  It seems like there are a few
possible behaviors:

1. Throw an error!  I guess this is what the patch does now.

2. Do absolutely nothing.  I think this is what would happen with an
ordinary UPDATE; the tuple fails the EPQ recheck and so is not
updated, but that doesn't trigger anything else.

3. Fall through to the NOT MATCHED clause and try that instead.
Allows MERGE to work as UPSERT in some simple cases, I think.

4. Continue walking the chain of WHEN MATCHED items in order and test
them against the new tuple.  This is actually pretty weird because a
0->1 update will fall through to the second UPDATE rule, but a 1->0
update will fall through to the NOT MATCHED clause.

5. Retry from the top of the chain with the updated tuple.  Could
theoretically livelock - not sure how much of a risk that is in
practice.

Maybe there are more options?

My initial reaction is to wonder what's wrong with #2.

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

Re: [HACKERS] generated columns

[Michael Paquier]

On Thu, Jan 25, 2018 at 10:26:55PM -0500, Peter Eisentraut wrote:
> On 1/19/18 00:18, Michael Paquier wrote:
>> +SELECT a, c FROM gtest12;  -- FIXME: should be allowed
>> +ERROR:  permission denied for function gf1
> 
> This is quite hard to fix and I would like to leave this for a future
> release.

I have been looking at that case more closely again, and on the contrary
I would advocate that your patch is doing the *right* thing.  In short,
if the generation expression uses a function and the user has only been
granted access to read the values, it seems to me that it we should
require that this user also has the right to execute the function. Would
that be too user-unfriendly?  I think that this could avoid mistakes
about giving access to unwanted functions when willing to just give a
SELECT right as the function could be doing more operations.

>> +-- domains
>> +CREATE DOMAIN gtestdomain1 AS int CHECK (VALUE < 10);
>> +CREATE TABLE gtest24 (a int PRIMARY KEY, b gtestdomain1 GENERATED
>> ALWAYS AS (a * 2));  -- prohibited
>> +ERROR:  virtual generated column "b" cannot have a domain type
>> CHECK constraints can be used, so I find this restriction confusing.
> 
> We currently don't have infrastructure to support this.  We run all
> CHECK constraints whenever a row is changed, so that is easy.  But we
> don't have facilities to recheck the domain constraint in column b when
> column a is updated.  This could be done, but it's extra work.

Okay, let's leave with this restriction for now.

>> OLD and NEW values for generated columns don't show up. Am I missing
>> something or they should be available?
> 
> This was already discussed a few times in the thread.  I don't know what
> a good solution is.
> 
> I have in this patch added facilties to handle this better in other PLs.
>  So the virtual generated column doesn't show up there in the trigger
> data.  This is possible because we copy the trigger data from the
> internal structures into language-specific hashes/dictionaries/etc.
> 
> In PL/pgSQL, this is a bit more difficult, because we handle the table's
> row type there.  We can't just "hide" the generated column when looking
> at the row type.  Actually, we could do it quite easily, but that would
> probably raise other weirdnesses.
> 
> This also raises a question how a row type with generated columns should
> behave.  I think a generation expression is a property of a table, so it
> does not apply in a row type.  (Just like a default is a property of a
> table and does not apply in row types.)

Hm.  Identity columns and default columns are part of rowtypes. STORED
columns can alsobe part of a row type with little effort, so in order to
be consistent with the all the existing behaviors, it seems to me that
virtually-generated columns should be part of it as well.  I have
compiled in the attached SQL file how things behave with your
patch.  Only virtually-generated columns show a blank value.

A empty value is unhelpful for the user, which brings a couple of
possible approaches:
1) Make virtual columns part of a row type, which would make it
consistent with all the other types.
2) For plpgsql, if all rows from OLD or NEW are requested, then print all
columns except the ones virtually-generated. If a virtual column is
directly requested, then issue an error.

I would warmly welcome 1) as this would make all behaviors consistent
with the other PLs and other types of generated columns.  I would
imagine that users would find weird the current inconsistency as well.

>> Per my tests, generated columns can work with column system attributes
>> (xmax, xmin, etc.). Some tests could be added.
> 
> Hard to test that, because the results would be nondeterministic.

tableoid can be deterministic if compared with data from pg_class:
=# CREATE TABLE aa (a int PRIMARY KEY, b int GENERATED ALWAYS AS (tableoid));
CREATE TABLE
=# INSERT INTO aa VALUES (1);
INSERT
=# SELECT aa.a from pg_class c, aa where c.oid = b;
 a
---
 1
(1 row)

The point is just to stress code paths where the attribute number is
negative.

>> -if (tab->relkind == RELKIND_RELATION ||
>> -tab->relkind == RELKIND_PARTITIONED_TABLE)
>> +if ((tab->relkind == RELKIND_RELATION ||
>> + tab->relkind == RELKIND_PARTITIONED_TABLE) &&
>> +get_attgenerated(RelationGetRelid(rel), attnum) != 
>> ATTRIBUTE_GENERATE
>> I think that you should store the result of get_attgenerated() and reuse
>> it multiple times.
> 
> I don't see where that would apply.  I think the hunks you are seeing
> belong to different functions.

Looks like I messed up ATPrepAlterColumnType() and ATExecColumnDefault()
while reading the previous version.  Sorry for the useless noise.

+   /*
+* Foreign keys on generated columns are not yet implemented.
+*/
+   for (i = 0; i < numpks; i++)
+   {
+   if (get_attgenerated(RelationGetRelid(pkrel), pkattnum[i]))
+   ereport(ERROR,
+   

Re: csv format for psql

[Pavel Stehule]

2018-01-31 13:58 GMT+01:00 Daniel Verite :

> Pavel Stehule wrote:
>
> > This format is too important, so some special short or long option can be
> > practical (it will be printed in help)
> >
> > some like --csv
>
> I guess -C/--csv could be used, like there already is -H/--html.
>
> > I found one issue - PostgreSQL default field separator is "|". Maybe good
> > time to use more common "," ?
> >
> > Or when field separator was not explicitly defined, then use "," for CSV,
> > and "|" for other. Although it can be little bit messy
>
> Currently there's a strong analogy between the unaligned
> mode and csv mode. In particular they use the existing pset
> variables fieldsep, fieldsep_zero, recordsep, recordsep_zero
> in the same way. If we want to make csv special with regard
> to the delimiters,  that complicates the user interface
> For instance if fieldsep was changed automatically by
> \pset format csv, it's not obvious if/when we should switch it
> back to its previous state, and how the fieldsep switch done
> automatically would mix or conflict with other \pset
> commands issued by the user.
> Or we need to duplicate these variables. Or duplicate
> only fieldsep, having a fieldsep_csv, leaving out the
> other variables and not being as close to the unaligned
> mode.
> These options don't appeal to me much compared
> to the simplicity of the current patch.
>
> Also, although the comma is the separator defined by the
> RFC4180 and the default for COPY CSV, people also use the
> semicolon extensively (because it's what Excel does I guess),
> which somehow mitigates the importance of comma as the
> default value.
>
>
The functionality is clean and great - default setting "|" is less. I have
not strong opinion about it and I understand to your arguments. Has anybody
some idea?

Regards

Pavel


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

Re: csv format for psql

[Daniel Verite]

Pavel Stehule wrote:

> This format is too important, so some special short or long option can be
> practical (it will be printed in help)
> 
> some like --csv

I guess -C/--csv could be used, like there already is -H/--html.

> I found one issue - PostgreSQL default field separator is "|". Maybe good
> time to use more common "," ?
> 
> Or when field separator was not explicitly defined, then use "," for CSV,
> and "|" for other. Although it can be little bit messy

Currently there's a strong analogy between the unaligned
mode and csv mode. In particular they use the existing pset
variables fieldsep, fieldsep_zero, recordsep, recordsep_zero
in the same way. If we want to make csv special with regard
to the delimiters,  that complicates the user interface
For instance if fieldsep was changed automatically by
\pset format csv, it's not obvious if/when we should switch it
back to its previous state, and how the fieldsep switch done
automatically would mix or conflict with other \pset
commands issued by the user.
Or we need to duplicate these variables. Or duplicate
only fieldsep, having a fieldsep_csv, leaving out the
other variables and not being as close to the unaligned
mode.
These options don't appeal to me much compared
to the simplicity of the current patch.

Also, although the comma is the separator defined by the
RFC4180 and the default for COPY CSV, people also use the
semicolon extensively (because it's what Excel does I guess),
which somehow mitigates the importance of comma as the
default value.


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

Re: [HACKERS] Why forcing Hot_standby_feedback to be enabled when creating a logical decoding slot on standby

[sanyam jain]

Hi,
I am trying to create logical decoding slot on a standby.
Enabling Hot_standby_feedback on standby is one of the requirement ; which will 
delay vacuuming of stale data on Master server.
I am working on a hack to avoid Hot_standby_feedback so that Standby doesn't 
have any dependency on Master (except receiving WAL).

Hot_standby_feedback restricts Master to do early vacuuming of catalog relation 
which will be in decoding WAL record using "time travelling snapshot" on a 
Standby.

The other way to prevent early vacuuming on Standby can be by pausing recovery 
on Standby when a vacuum record belonging to catalog relation is encountered.
And when walsender process belonging to logical slot on Standby reaches this 
record it will resume the recovery by executing SetRecoveryPause(false).

To check whether VACUUM record belongs to a catalog relation i simply check if 
relationID < 1.

This hack will only work for a single logical slot on standby.
Pausing recovery will increase the size of pg_xlog directory as walreceiver 
will continue receiving WAL.
Querying standby might result in wrong output.

Thanks,
Sanyam Jain

Re: csv format for psql

[Daniel Verite]

Fabien COELHO wrote:

> > This patch implements csv as an output format in psql
> > (\pset format csv).
> 
> Would you consider registering it in the next CF?

Done here:

https://commitfest.postgresql.org/17/1500/

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

Re: WINDOW RANGE patch versus leakproofness

[Dean Rasheed]

On 30 January 2018 at 16:42, Tom Lane  wrote:
> So I'm thinking that (a) we do not need to check for leaky functions used
> in window support, and (b) therefore there's no need to avoid leaky
> behavior in in_range support functions.  Objections?
>

Yes, I concur. Since window functions can only appear in the SELECT
target list and ORDER BY clauses, they should never appear in a qual
that gets considered for push down, and thus contain_leaked_vars()
should never see a window function.

Moreover, contain_leaked_vars() is intentionally coded defensively, so
if it ever does somehow see a window function (or any other unexpected
node type) it will return true and the resulting qual/restrictinfo
will be marked leaky, and not pushed through security barriers.

Regards,
Dean

Re: Help needed in using 'on_dsm_detach' callback

[Gaddam Sai Ram]

Hello Thomas,
  
Thank you for your suggestions, needless to say it helped a lot. As 
you suggested, I have created an initial dsm segment and used it create dsa 
area as well as to detach dsm. Thus, it helped me in using 'on_dsm_detach' 
callback. I have tested the code and it works well. I did post a code snippet, 
just verify if I have missed anything.

CREATE DSA:

  old_context = MemoryContextSwitchTo(TopMemoryContext);



  segment = dsm_create(DSA_INITIAL_SEGMENT_SIZE, 0); /* Initial segment 
size 1mb */

  dsm_pin_segment(segment);



  area = dsa_create_in_place(dsm_segment_address(segment), 
DSA_INITIAL_SEGMENT_SIZE, GraphIndex_tranche_id(), segment);

  on_dsm_detach(segment, detach_func, 
PointerGetDatum(dsm_segment_address(segment)));



  MemoryContextSwitchTo(old_context);

  

  dsa_pin(area);

  dsm_pin_mapping(segment);

  dsa_pin_mapping(area);



  /* Saving dsa_handle in shmem for serving other processes */

  GraphIndexShmem-graphindex_dsa_handle = dsm_segment_handle(segment);



  PROC_DSA_AREA = area;





ATTACH DSA:



  old_context = MemoryContextSwitchTo(TopMemoryContext);



  segment = dsm_attach(GraphIndexShmem-graphindex_dsa_handle);

  area = dsa_attach_in_place(dsm_segment_address(segment), segment);



  on_dsm_detach(segment, detach_func, 
PointerGetDatum(dsm_segment_address(segment)));



  MemoryContextSwitchTo(old_context);



  dsm_pin_mapping(segment);

  dsa_pin_mapping(area);



  PROC_DSA_AREA = area;



Thank you,
G. Sai Ram







 On Wed, 24 Jan 2018 13:53:52 +0530 Thomas Munro 
thomas.mu...@enterprisedb.com wrote 




On Wed, Jan 24, 2018 at 8:37 PM, Gaddam Sai Ram 

gaddamsaira...@zohocorp.com wrote: 

 Found that there is a callback for dsa detach but that function requires 

 segment pointer as an argument, Should be as below: 

 

 on_dsm_detach(PROC_DSA_AREA-segment_maps[0].segment, detach_func); 

 ... 

 But i couldn't access that segment, as DSA_AREA struct is defined in 
dsa.c, 

 so I am unable to include. 

 

 Any other ways to get dsa detach event, or to access DSA Segment pointer? 

 

There are two ways to create DSA areas: dsa_create() and 

dsa_create_in_place(). In all existing in-core uses of DSA areas, 

they are created "in place". That means that you have to supply the 

initial DSM segment and then they live inside that, and they will 

quietly create more DSM segments as needed if they run out of space. 

If you do it that way you get to install detach hooks for that root 

DSM segment because you have your hands on it. Maybe you should do 

that too? Basically just replace your current DSA creation, handle 

sharing and attaching code with the DSM equivalents (make it, say, 1MB 

in size), and then use dsa_{create,attach}_in_place() in the space 

inside it. The easiest way would be to give *all* the space in this 

root DSM segment using dsm_segment_address() to get the "place" to put 

the DSA area, or you could use a shm_toc to put that + other fixed 

size stuff inside it (the way execParallel.c does). 

 

It probably seems a bit weird and circular that DSA areas manage a set 

of DSM segments, but can themselves live in a user-supplied DSM 

segment. DSM segments have two roles: they are shared memory, and in 

this capacity a DSA area owns and uses them as raw storage, but they 

are also a kind of general shared resource ownership/management 

mechanism that comes with some free shared space, and in this capacity 

they can be used to own a DSA area (or shared files, or interprocess 

queues, or ..). 

 

Hope that helps. 

 

-- 

Thomas Munro 

http://www.enterprisedb.com 

Re: Transform for pl/perl

[Arthur Zakirov]

Hello,

On Fri, Jan 12, 2018 at 11:47:39AM +0300, Anthony Bykov wrote:
> Hello, thank you for your message.
> The problem was that different perl compilers uses different infinity
> representations. Some of them use "Inf" others - use "inf". So, in
> attachments there is a new version of the patch.

I've noticed a possible bug:

> + /* json key in v */
> + key = pstrdup(v.val.string.val);
> + keyLength = v.val.string.len;
> + JsonbIteratorNext(, , true);

I think it is worth to use pnstrdup() here, because v.val.string.val is
not necessarily null-terminated as the comment says:

> struct JsonbValue
> ...
>   struct
>   {
>   int len;
>   char   *val;/* Not necessarily 
> null-terminated */
>   }   string; /* String primitive 
> type */

Consider an example:

=# CREATE FUNCTION testSVToJsonb3(val jsonb) RETURNS jsonb
LANGUAGE plperl
TRANSFORM FOR TYPE jsonb
AS $$
return $_->{"1"};
$$;

=# SELECT testSVToJsonb3('{"1":{"2":[3,4,5]},"2":3}');
 testsvtojsonb3 

 (null)

But my perl isn't good, so the example maybe isn't good too.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Re: Wait for parallel workers to attach

[Amit Kapila]

On Mon, Jan 29, 2018 at 4:01 AM, Peter Geoghegan  wrote:
> On Sat, Jan 27, 2018 at 12:14 AM, Amit Kapila  wrote:
>
> I also found that all of these errors were propagated. The patch helps
> parallel CREATE INDEX as expected/designed.
>

Great!

> Some small things that I noticed about the patch:
>
> * Maybe "if (!known_started_workers[i])" should be converted to "if
> (known_started_workers[i]) continue;", to decrease the indentation
> level of the WaitForParallelWorkersToAttach() loop.
>

Changed as per suggestion.

> * There might be some opportunity to share some of the new code with
> the code recently committed to WaitForParallelWorkersToFinish(). For
> one thing, the logic in this block could be refactored into a
> dedicated function that is called by both
> WaitForParallelWorkersToAttach() and WaitForParallelWorkersToFinish():
>
>> +   else if (status == BGWH_STOPPED)
>> +   {
>> +   /*
>> +* Check whether the worker ended up stopped without ever
>> +* attaching to the error queue.  If so, the postmaster
>> +* was unable to fork the worker or it exited without
>> +* initializing properly.  We must throw an error, since
>> +* the caller may have been expecting the worker to do
>> +* some work before exiting.
>> +*/
>> +   mq = shm_mq_get_queue(pcxt->worker[i].error_mqh);
>> +   if (shm_mq_get_sender(mq) == NULL)
>> +   ereport(ERROR,
>> +   
>> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>> +errmsg("parallel worker failed to 
>> initialize"),
>> +errhint("More details may be available in 
>> the server log.")));
>> +
>> +   known_started_workers[i] = true;
>> +   ++nknown_started_workers;
>> +   }
>

I had thought about this earlier but left it as the common code was
too less, however as you have pointed out, I had extracted the common
code into a separate function.

> * If we don't actually commit the patch to make nodeGather.c call
> WaitForParallelWorkersToAttach(), which I suspect will happen, then I
> think we should instead at least have a comment saying why it's okay
> that we don't call WaitForParallelWorkersToAttach(). If we go this
> way, the comment should directly replace the
> modify_gather_to_wait_for_attach_v1.patch call to
> WaitForParallelWorkersToAttach() -- this comment should go in
> ExecGather().
>
> * Maybe the comments at the top of WaitForParallelWorkersToAttach()
> should at least allude to the ExecGather() special case I just
> mentioned.
>

I think we should not touch anything related to Gather (merge) as they
don't need it for the purpose of correctness.  However, we might want
to improve them by using this new API at a certain point if the need
arises.  I guess we can use this API to detect failures early.

> * Maybe the comments at the top of WaitForParallelWorkersToAttach()
> should also advise callers that it's a good idea to try to do other
> leader-only work that doesn't involve a WaitLatch() before they call.
>
> In general, WaitForParallelWorkersToAttach() needs to be called before
> any WaitLatch() (or barrier wait, or condition variable wait) that
> waits on workers, and after workers are first launched, but callers
> may be able to arrange to do plenty of other work, just like nbtsort.c
> does. To be clear: IMHO calling WaitForParallelWorkersToAttach()
> should be the rule, not the exception.
>
> * Finally, perhaps the comments at the top of
> WaitForParallelWorkersToAttach() should also describe how it relates
> to WaitForParallelWorkersToFinish().
>
> ISTM that WaitForParallelWorkersToAttach() is a subset of
> WaitForParallelWorkersToFinish(), that does all that is needed to rely
> on nworkers_launched actually being the number of worker processes
> that are attached to the error queue. As such, caller can expect
> propagation of errors from workers using standard parallel message
> interrupts once WaitForParallelWorkersToAttach() returns. You probably
> shouldn't directly reference nworkers_launched, though, since that
> doesn't necessarily have to be involved for parallel client code to
> run into trouble. (You only need to wait on workers changing something
> in shared memory, and failing to actively inform leader of failure
> through a parallel message -- this might not involve testing
> nworkers_launched in the way parallel CREATE INDEX happens to.)
>

I have updated the comments atop WaitForParallelWorkersToAttach() to
address your above two points.

> known_started_workers looks a lot like any_message_received.  Perhaps 
> any_message_received should be renamed to known_started_workers and reused 
> here.  After all, if we know that a
> worker 

Re: [HACKERS] [PATCH] Improve geometric types

[Kyotaro HORIGUCHI]

At Wed, 31 Jan 2018 13:09:09 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20180131.130909.210233873.horiguchi.kyot...@lab.ntt.co.jp>
> At Sun, 21 Jan 2018 21:59:19 +0100, Emre Hasegeli  wrote 
> in 
> > New versions are attached including all changes we discussed.
> 
> Thanks for the new version.
> 
> # there's many changes from the previous version..
> 
> About 0001 and 0002.
> 
> 1."COPT=-DGEODEBUG make" complains as follows.
> 
>   | geo_ops.c:2445:62: error: invalid type argument of unary ‘*’ (have 
> ‘float8 {aka double}’)
>   |   printf("dist_ppoly_internal- segment 0/n distance is %f\n", *result);
> 
> 2. line_construct_pm has been renamed to line_construct. I
>noticed that the patch adds the second block for "(m == 0.0)"
>(from the ealier versions) but it seems to work exactly as the
>same to the "else" block. We need a comment about the reason
>for the seemingly redundant second block.
> 
> 3. point_sl can return -0.0 and that is a thing that this patch
>intends to avoid. line_invsl has the same problem.
> 
> 4. lseg_interpt_line is doing as follows.
> 
>>  if (FPeq(lseg->p[0].x, interpt.x) && FPeq(lseg->p[0].y, interpt.y))
>>*result = lseg->p[0];
>>  else if (FPeq(lseg->p[1].x, interpt.x) && FPeq(lseg->p[1].y, interpt.y))
>>*result = lseg->p[1];
> 
>I suppose we can use point_pt_point for this purpose.
> 
>>  if (point_eq_point(>p[0], ))
>>*result = lseg->p[0];
>>  else if (point_eq_point(>p[1], ))
>>*result = lseg->p[1];
> 
>However I'm not sure that adjusting the intersection to the
>tips of the segment is good or not. Adjusting onto the line
>can be better in another case. lseg_interpt_lseg, for
>instance, checks lseg_contain_point on the line parameter of
>lseg_interpt_line.
> 

> # I'll be back later..

I've been back.

0003: This patch replaces "double" with float and bare arithmetic
and comparisons on double to special functions accompanied
with checking against abnormal values.

 - Almost all of them are eliminated with a few safe exceptions.

- circle_recv(), circle_distance(), dist_pc(), dist_cpoint()
  are using "< 0.0" comparison but it looks fine.

- pg_hypot is right to use bare arithmetics.

! circle_contain_pt() does the following comparison and it
  seems to be out of our current policy.

  point_dt(center, point) <= radius

  I suppose this should use FPle.

  FPle(point_dt(center, point), radius)

  The same is true of circle_contain_pt(), pt_contained_circle .



# Sorry, that' all for today..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [HACKERS] [PATCH] Lockable views

[Yugo Nagata]

On Tue, 30 Jan 2018 19:21:04 +1300
Thomas Munro  wrote:

> On Tue, Jan 30, 2018 at 6:48 PM, Tatsuo Ishii  wrote:
> >>> You need to DROP VIEW lock_view4 and lock_view5 in the regression
> >>> test as well.
> >>
> >> Thank you for reviewing the patch.
> >>
> >> I fixed this and attached a updated patch v6.
> >
> > Looks good to me. If there's no objection, especially from Thomas
> > Munro, I will mark this as "ready for committer".
> 
> About the idea:  it makes some kind of sense to me that we should lock
> the underlying table, in all the same cases that you could do DML on
> the view automatically.  I wonder if this is a problem for the
> soundness:  "Tables appearing in a subquery are ignored and not
> locked."  I can imagine using this for making backwards-compatible
> schema changes, in which case the LOCK-based transaction isolation
> techniques might benefit from this behaviour.  I'd be interested to
> hear about the ideal use case you have in mind.

I think the use case is almost similar to that of auto-updatable views.
There are some purpose to use views, for example 1) preventing from
modifying application due to schema changes, 2) protecting the underlying
table from users without proper privileges, 3) making a shorthand of a
query with complex WHERE condition. When these are updatable views and
users need transaction isolation during updating them, I think the lockable
views feature is benefitical because users don't need to refer to the
underlying table. Users might don't know the underlying table, or even
might not have the privilege to lock this.

> About the patch:  I didn't study it in detail.  It builds, has
> documentation and passes all tests.  Would it be a good idea to add an
> isolation test to show that the underlying relation is actually
> locked?

Whether the underlying relation is actually locked or not is confirmed
in the regression test using pg_locks, so I don't believe that we need
to add an isolation test.
 
> Typo:
> 
> + /* Check permissions with the view owner's priviledge. */
> 
> s/priviledge/privilege/
> 
> Grammar:
> 
> +/*
> + * Check whether the view is lockable.
> + *
> + * Currently, only auto-updatable views can be locked, that is,
> + * views whose definition are simple and that doesn't have
> + * instead of rules or triggers are lockable.
> 
> s/definition are simple and that doesn't/definition is simple and that don't/
> s/instead of/INSTEAD OF/ ?

Thank you for pointing out these. I attached the fixed patch.

Regards,
 
> -- 
> Thomas Munro
> http://www.enterprisedb.com


-- 
Yugo Nagata 
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index b2c7203..6ddd128 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -46,6 +46,14 @@ LOCK [ TABLE ] [ ONLY ] name [ * ]
   
 
   
+   Automatically updatable views (see )
+   that do not have INSTEAD OF triggers or
+   INSTEAD rules are also lockable. When a view is
+   locked, its base relations are also locked recursively with the same
+   lock mode. Tables appearing in a subquery are ignored and not locked.
+  
+
+  
When acquiring locks automatically for commands that reference
tables, PostgreSQL always uses the least
restrictive lock mode possible. LOCK TABLE
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 6479dcb..f6b5962 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,15 +19,20 @@
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
+#include "parser/parsetree.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
+#include "rewrite/rewriteHandler.h"
+#include "access/heapam.h"
 
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
-static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid);
+static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
 static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
 			 Oid oldrelid, void *arg);
+static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait);
+static void LockViewCheck(Relation view, Query *viewquery);
 
 /*
  * LOCK TABLE
@@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt)
 		  RangeVarCallbackForLockTable,
 		  (void *) >mode);
 
-		if (recurse)
-			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait);
+		if (get_rel_relkind(reloid) == RELKIND_VIEW)
+			LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait);
+		else if (recurse)
+			LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId());
 	}
 }
 
@@ -86,15 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 		return;	/* woops, concurrently 

Re: [HACKERS] path toward faster partition pruning

[Amit Langote]

Horiguchi-san,

Thanks for the review.

On 2018/01/30 20:43, Kyotaro HORIGUCHI wrote:
> At Tue, 30 Jan 2018 09:57:44 +0900, Amit Langote wrote:
>> AFAICS, v22 cleanly applies to HEAD (c12693d8f3 [1]), compiles, and make
> I have some random comments on 0002.
> 
> -extract_partition_key_clauses implicitly assumes that the
>  commutator of inequality operator is the same to the original
>  operator. (commutation for such operators is an identity
>  function.)

Yeah, it seems so.

>  I believe it is always true for a sane operator definition, but
>  perhaps wouldn't it be better using commutator instead of
>  opclause->opno as the source of negator if any? (Attached diff 1)

ISTM, the same thing happens with or without the patch.

- pc->opno = OidIsValid(commutator) ? commutator : opclause->opno;
+ pc->opno = comparator;

comparator as added by the patch is effectively equal to the RHS
expression in the deleted line.

> -get_partitions_from_or_clause_args abandons arg_partset with all
>  bit set when partition constraint doesn't refute whole the
>  partition. Finally get_partitions_from_clauses does the same
>  thing but it's waste of cycles and looks weird. It seems to have
>  intended to return immediately there.
> 
>   >   /* Couldn't eliminate any of the partitions. */
>   >   partdesc = RelationGetPartitionDesc(context->relation);
>   > - arg_partset = bms_add_range(NULL, 0, partdesc->nparts - 1);
>   > + return bms_add_range(NULL, 0, partdesc->nparts - 1);
>   > }
>   >  
>   > subcontext.rt_index = context->rt_index;
>   > subcontext.relation = context->relation;
>   > subcontext.clauseinfo = 
>  !> arg_partset = get_partitions_from_clauses();

You're right, fixed.

> -get_partitions_from_or_clause_args converts IN (null) into
>  nulltest and the nulltest doesn't exclude a child that the
>  partition key column can be null.
> 
>  drop table if exists p;
>  create table p (a int, b int) partition by list (a);
>  create table c1 partition of p for values in (1, 5, 7);
>  create table c2 partition of p for values in (4, 6, null);
>  insert into p values (1, 0), (null, 0);
>  
>  explain select tableoid::regclass, * from p where a in (1, null);
>  >QUERY PLAN
>  > -
>  >  Result  (cost=0.00..76.72 rows=22 width=12)
>  >->  Append  (cost=0.00..76.50 rows=22 width=12)
>  >  ->  Seq Scan on c1  (cost=0.00..38.25 rows=11 width=12)
>  >Filter: (a = ANY ('{1,NULL}'::integer[]))
>  >  ->  Seq Scan on c2  (cost=0.00..38.25 rows=11 width=12)
>  >Filter: (a = ANY ('{1,NULL}'::integer[]))
> 
>  Although the clause "a in (null)" doesn't match the (null, 0)
>  row so it donesn't harm finally, I don't think this is a right
>  behavior. null in an SAOP rightop should be just ignored on
>  partition pruning. Or is there any purpose of this behavior?

Yeah, it seems that we're better off ignoring null values appearing the
IN-list.  Framing a IS NULL or IS NOT NULL expression corresponding to a
null value in the SAOP rightop array doesn't seem to be semantically
correct, as you also pointed out.  In ExecEvalScalarArrayOpExpr(), I see
that a null in the rightop array (IN-list) doesn't lead to selecting rows
containing null in the corresponding column.

> - In extract_bounding_datums, clauseinfo is set twice to the same
>   value.

Oops, my bad when merging in David's patch.

Update patch set attached.  Thanks again.

Regards,
Amit
>From 42ca7750e2b89caa3aee0c9ab7479a7f29953fff Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 22 Aug 2017 11:33:27 +0900
Subject: [PATCH v23 1/5] Some interface changes for
 partition_bound_{cmp/bsearch}

Introduces a notion of PartitionBoundCmpArg, which replaces the set
of arguments void *probe and bool probe_is_bound of both
partition_bound_cmp and partition_bound_bsearch.  It wasn't possible
before to specify the number of datums when a non-bound type of
probe is passed.  Slightly tweaking the existing interface to allow
specifying the same seems awkward.  So, instead encapsulate that
into PartitionBoundCmpArg.  Also, modify partition_rbound_datum_cmp
to compare caller-specifed number of datums, instead of
key->partnatts datums.
---
 src/backend/catalog/partition.c | 166 +---
 1 file changed, 123 insertions(+), 43 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index e69bbc0345..de2b53e0c8 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -138,6 +138,31 @@ typedef struct PartitionRangeBound
boollower;  /* this is the lower (vs upper) 
bound */
 } PartitionRangeBound;
 
+/*
+ * PartitionBoundCmpArg - Caller-defined argument to be passed to
+ *   partition_bound_cmp()
+ *

Re: JIT compiling with LLVM v9.0

[Konstantin Knizhnik]

On 31.01.2018 05:48, Thomas Munro wrote:



This seems to be a valid complaint.  I don't think you should be
(indirectly) wrapping Types.h in extern "C".  At a guess, your
llvmjit.h should be doing its own #ifdef __cplusplus'd linkage
specifiers, so you can use it from C or C++, but making sure that you
don't #include LLVM's headers from a bizarro context where __cplusplus
is defined but the linkage is unexpectedly already "C"?

Hm, this seems like a bit of pointless nitpickery by the compiler to me,
but I guess...

Well that got me curious about how GCC could possibly be accepting
that (it certainly doesn't like extern "C" template ... any more than
the next compiler).  I dug a bit and realised that it's the stdlib
that's different:  libstdc++ has its own extern "C++" in ,
while  libc++ doesn't.

The same problem takes place with old versions of GCC: I have to upgrade 
GCC to 7.2 to make it possible to compile this code.

The problem in not in compiler itself, but in libc++ headers.

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