Re: [bug fix] Cascaded standby cannot start after a clean shutdown

[Michael Paquier]

On Tue, Jun 12, 2018 at 06:30:49AM +, Tsunakawa, Takayuki wrote:
> Thank you so much.  This version looks better.
> 
> +  * this would cause the instance to stop suddendly with a hard failure,
> 
> suddendly -> suddenly

Yep.  Thanks for the extra lookup.
--
Michael


signature.asc
Description: PGP signature

Re: why partition pruning doesn't work?

[David Rowley]

On 12 June 2018 at 10:24, Tom Lane  wrote:
> After looking closer, that code isn't just inefficient, it's flat
> out broken.  The reason is that ExecSetupPartitionPruneState thinks
> it can store some pointers into the target relation's relcache entry
> in the PartitionPruneContext, and then continue to use those pointers
> after closing the relcache entry.  Nope, you can't do that.

I think the best fix is to just have a separate FmgrInfo for each step
and partkey comparison. Some FmgrInfos will end up identical, but
that's probably a small price to pay. Perhaps they should be separate
anyway so that the fn_extra is not shared between different quals
comparing to the same partition key?

I went with an array of FmgrInfos rather than an array of pointers to
FmgrInfos for cache efficiency.  This does require that InvalidOid is
0, since I've palloc0'd that memory, and I'm checking if the cache is
yet to be populated with: if
(!OidIsValid(context->stepcmpfuncs[stateidx].fn_oid))

Patch attached.

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


fix_bogus_fmgrinfo_initialisation.patch
Description: Binary data

Re: Proposal: Partitioning Advisor for PostgreSQL

[Ashutosh Bapat]

On Tue, Jun 12, 2018 at 12:21 AM, Julien Rouhaud  wrote:
>
> I both like and dislike this idea.  The good thing is that it's way
> less hacky than what we did in our prototype, and it's also working
> out of the box.  However, the problem I have with this approach is
> that the generated plans will be quite different from real
> partitioning,  The main features such as partition pruning or
> partition-wise join will probably work, but you'll always have a
> ForeignScan as the primary path and I think that it'll drastically
> limit the planner and the usability.

AFAIR, there is a hook using which we can change the EXPLAIN output,
so we could change the ForeignScan label. But I don't remember that
hook top of my head and a brief look at Explain code didn't reveal
anything. May be there isn't any hook. We may be able add one in that
case or use CustomScan or something like that. I agree that seeing a
ForeignScan in the plan is not a good thing.

Anyway, the work involved in my proposal may not be worth the utility
we get out of this extension, so may not be worth pursuing it further.

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

RE: ON CONFLICT DO NOTHING on pg_dump

[Ideriha, Takeshi]

>From: Surafel Temesgen [mailto:surafel3...@gmail.com] 
>Subject: ON CONFLICT DO NOTHING on pg_dump

>Sometimes I have to maintain two similar database and I have to update one 
>from the other and notice having the option to add ON CONFLICT DO NOTHING 
>clause to >INSERT command in the dump data will allows pg_restore to be done 
>with free of ignore error.

Hi,
I feel like that on-conflict-do-nothing support is useful especially coupled 
with --data-only option.
Only the difference of data can be restored.

>The attache patch add --on-conflect-do-nothing option to pg_dump in order to 
>do the above. 

The followings are some comments.

+  --on-conflect-do-nothing
Here's a typo: conflect -> conflict. This typo also applies to pg_dump.c

printf(_("  --insertsdump data as INSERT commands, 
rather than COPY\n"));
+   printf(_("  --on-conflect-do-nothing dump data as INSERT commands 
with on conflect do nothing\n"));
printf(_("  --no-commentsdo not dump comments\n"));

The output of help should be in alphabetical order according to the convention. 
So changing the order seems logical. 
Please apply my review to the documentation as well.
By the way, 4d6a854 breaks the patch on this point.

+This option is not valid unless --inserts is also 
specified.
+   
   
+   if (dopt.do_nothing && !dopt.dump_inserts)
+   exit_horribly(NULL, "option --on-conflect-do-nothing requires 
option --inserts\n");

How about mentioning --column-inserts? --on-conflict-do-nothing with 
--column-inserts should work.

Do you have any plan to support on-conlict-do-update? Supporting this seems to 
me complicated and take much time so I don't mind not implementing this.

What do you think about adding some test cases? 
command_fails_like() at 001_basic.pl checks command fail pattern with invalid 
comnibation of option.
And 002_pg_dump.pl checks the feature iteself.

Regards,
Takeshi Ideriha

Re: Proposal: Partitioning Advisor for PostgreSQL

[Dilip Kumar]

On Thu, May 24, 2018 at 4:16 PM, Yuzuko Hosoya 
wrote:

> Hello,
>
> I'm Yuzuko Hosoya. This is my first PostgreSQL project participation.
>
> I have been developing partitioning advisor prototype with Julien Rouhaud.
> It will be a new feature of HypoPG[1], which is a PostgreSQL extension, and
> will help partitioning design tuning.  Currently, HypoPG only supports
> index
> design tuning; it allows users to define hypothetical indexes for real
> tables and
> shows resulting queries' plan/cost with EXPLAIN as if they were actually
> constructed.
> Since declarative partitioning will be greatly improved in PostgreSQL 11
> and further
> versions, there are emerging needs to support partitioning design tuning.
> This is
> why we are working on partitioning advisor.  We plan to release the first
> version
> of partitioning advisor for PostgreSQL 11, and then, improve it for
> PostgreSQL 12.
>
>
Interesting.


>
> - Estimating stats
> It is complicated because hypothetical partition has no data.  Currently,
> we compute
> hypothetical partition's size using clauselist_selectivity() according to
> their partition
> bound and original table stats.  As a result, estimate is done with low
> accuracy,
> especially if there is WHERE clause.  We will improve during developing,
> but for now,
> we don't have good ideas.
>

I haven't yet read the patch but curious to know.  Suppose we have table
which is already loaded with some data.  Now, if I create  hypothetical
partitions on that will we create any stat data (mcv, histogram) for
hypothetical
table? because, in this case we already have the data from the main table
and we also have partition boundary for the hypothetical table.  I am not
sure you are already doing this or its an open item?

>
>
> [1] https://github.com/HypoPG/hypopg
>
> Best regards,
> 
> Yuzuko Hosoya
> NTT Open Source Software Center
>
>
>
>


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

Re: Proposal: Partitioning Advisor for PostgreSQL

[Julien Rouhaud]

Hi,

On Tue, Jun 12, 2018 at 11:14 AM, Dilip Kumar  wrote:
> On Thu, May 24, 2018 at 4:16 PM, Yuzuko Hosoya 
> wrote:
>>
>> This is
>> why we are working on partitioning advisor.  We plan to release the first
>> version
>> of partitioning advisor for PostgreSQL 11, and then, improve it for
>> PostgreSQL 12.
>>
>
> Interesting.

Thanks!

>> - Estimating stats
>> It is complicated because hypothetical partition has no data.  Currently,
>> we compute
>> hypothetical partition's size using clauselist_selectivity() according to
>> their partition
>> bound and original table stats.  As a result, estimate is done with low
>> accuracy,
>> especially if there is WHERE clause.  We will improve during developing,
>> but for now,
>> we don't have good ideas.
>
>
> I haven't yet read the patch but curious to know.  Suppose we have table
> which is already loaded with some data.  Now, if I create  hypothetical
> partitions on that will we create any stat data (mcv, histogram) for
> hypothetical table? because, in this case we already have the data from the
> main table and we also have partition boundary for the hypothetical table.
> I am not sure you are already doing this or its an open item?


For now we're simply using the original table statistics, and
appending the partition bounds as qual on the hypothetical partition.
It'll give good result if the query doesn't have quals for the table,
or for simple cases where selectivity functions understand that
expressions such as

(id BETWEEN 1 AND 100) AND (id < 6)

will return only 5 rows, while they can't for expressions like

(id IN (x,y...)) AND (id < z)

In this second case, the estimates are for now therefore quite wrong.
I think that we'd have no other choice than to generate hypothetical
statistics according to the partition bounds, and only compute
selectivity based on the query quals.  It's definitely not simple to
do, but it should be doable with the hooks currently available.

late binding of shared libs for C functions

[Geoff Winkless]

Hi All

Is it possible to use CREATE FUNCTION to link a shared library that
doesn't yet exist? I don't think it is, but I might be missing
something.

If not, would it be something that people would be open to a patch
for? I'm thinking of e.g.

CREATE [ OR REPLACE ] FUNCTION
name ( [ [ argmode ] [ argname ] argtype [ { DEFAULT | = }
default_expr ] [, ...] ] )
[ RETURNS rettype
  | RETURNS TABLE ( column_name column_type [, ...] ) ]
  { LANGUAGE lang_name
| TRANSFORM { FOR TYPE type_name } [, ... ]
| WINDOW
| IMMUTABLE | STABLE | VOLATILE | [ NOT ] LEAKPROOF
| CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT
| [ EXTERNAL ] SECURITY INVOKER | [ EXTERNAL ] SECURITY DEFINER
| PARALLEL { UNSAFE | RESTRICTED | SAFE }
| COST execution_cost
| ROWS result_rows
| SET configuration_parameter { TO value | = value | FROM CURRENT }
| AS 'definition'
-| AS 'obj_file', 'link_symbol'
+| AS 'obj_file', 'link_symbol' [UNBOUNDED]
 } ...
[ WITH ( attribute [, ...] ) ]


(I know UNBOUNDED isn't quite the word - BINDLATE would be better -
but I figured I should try to use an existing reserved keyword...)

We run our SQL scripts before we install binaries (because our
binaries are started by the installer, so having the database in place
is a Good Thing). The binary installer includes the .so. We're now
stuck in a catch-22 where I can't run the SQL script because it
requires the .so to be in place, but I can't run the binary installer
because if I do the SQL won't be updated...

This specific problem is obviously workaround-able, but it occurred to
me that since the libraries are bound late anyway it seems like this
wouldn't cause any serious problems.

Of course chances are I've missed something...

Geoff

Re: Does logical replication supports cross platform servers?

[Andrew Dunstan]

On 06/11/2018 11:28 PM, Craig Ringer wrote:
On 12 June 2018 at 11:04, Haribabu Kommi > wrote:


Hi All,

I am not able to find any docs suggesting that PostgreSQL logical
replication supports
cross platform servers (windows --> Linux or vice-versa).


It should. I don't think there's buildfarm coverage yet, though.






Designing something like that would be more than interesting. We'd need 
two or more co-operating animals, a concept that's currently quite 
foreign to the buildfarm. I'm not saying it can't be done, but it would 
be a substantial piece of work. Probably the place to start would be the 
save-bin feature discussed elsewhere for things like cross-version 
pg_dump/pg_restore checking. We already do that for cross-version 
pg_upgrade testing, so it would need to be generalized.


cheers

andrew

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

Re: why partition pruning doesn't work?

[Andrew Dunstan]

On 06/11/2018 06:41 PM, Andrew Dunstan wrote:



On 06/11/2018 06:24 PM, Tom Lane wrote:


If we had any buildfarm critters running valgrind on
RELCACHE_FORCE_RELEASE or CLOBBER_CACHE_ALWAYS builds, they'd have
detected use of uninitialized memory here ... but I don't think we have
any.  (The combination of valgrind and CCA would probably be too slow to
be practical :-(, though maybe somebody with a fast machine could do
the other thing.)





I don't have a fast machine, but I do have a slow machine already 
running valgrind and not doing much else :-) Let's see how lousyjack 
does with -DRELCACHE_FORCE_RELEASE






It added about 20% to the run time. That's tolerable, so I'll leave it on.

cheers

andrew

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

Re: late binding of shared libs for C functions

[Komяpa]

This thing also bites PostGIS upgrades.

When distro's packaging system decides to upgrade PostGIS, or both
Postgres/PostGIS at the same time, you may often get to a situation when
you only have one version of PostGIS .so installed, and it's not the one
referenced in catalog. There are workarounds that tell you to symlink a
newer PostGIS .so file to the spot an older one is being looked for, and
then do ALTER EXTENSION UPGRADE to get out of this inconsistent state.

This also means PostGIS has to ship stubs for all the functions that should
have been deleted, but may be needed during such hacky upgrade process.
For example,
https://github.com/postgis/postgis/blob/16270b9352e84bc989b9b946d279f16e0de5c2b9/postgis/lwgeom_accum.c#L55


вт, 12 июн. 2018 г. в 13:48, Geoff Winkless :

> Hi All
>
> Is it possible to use CREATE FUNCTION to link a shared library that
> doesn't yet exist? I don't think it is, but I might be missing
> something.
>
> If not, would it be something that people would be open to a patch
> for? I'm thinking of e.g.
>
> CREATE [ OR REPLACE ] FUNCTION
> name ( [ [ argmode ] [ argname ] argtype [ { DEFAULT | = }
> default_expr ] [, ...] ] )
> [ RETURNS rettype
>   | RETURNS TABLE ( column_name column_type [, ...] ) ]
>   { LANGUAGE lang_name
> | TRANSFORM { FOR TYPE type_name } [, ... ]
> | WINDOW
> | IMMUTABLE | STABLE | VOLATILE | [ NOT ] LEAKPROOF
> | CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT
> | [ EXTERNAL ] SECURITY INVOKER | [ EXTERNAL ] SECURITY DEFINER
> | PARALLEL { UNSAFE | RESTRICTED | SAFE }
> | COST execution_cost
> | ROWS result_rows
> | SET configuration_parameter { TO value | = value | FROM CURRENT }
> | AS 'definition'
> -| AS 'obj_file', 'link_symbol'
> +| AS 'obj_file', 'link_symbol' [UNBOUNDED]
>  } ...
> [ WITH ( attribute [, ...] ) ]
>
>
> (I know UNBOUNDED isn't quite the word - BINDLATE would be better -
> but I figured I should try to use an existing reserved keyword...)
>
> We run our SQL scripts before we install binaries (because our
> binaries are started by the installer, so having the database in place
> is a Good Thing). The binary installer includes the .so. We're now
> stuck in a catch-22 where I can't run the SQL script because it
> requires the .so to be in place, but I can't run the binary installer
> because if I do the SQL won't be updated...
>
> This specific problem is obviously workaround-able, but it occurred to
> me that since the libraries are bound late anyway it seems like this
> wouldn't cause any serious problems.
>
> Of course chances are I've missed something...
>
> Geoff
>
>

Re: late binding of shared libs for C functions

[Andrew Dunstan]

On 06/12/2018 06:48 AM, Geoff Winkless wrote:

Hi All

Is it possible to use CREATE FUNCTION to link a shared library that
doesn't yet exist? I don't think it is, but I might be missing
something.

If not, would it be something that people would be open to a patch
for? I'm thinking of e.g.

CREATE [ OR REPLACE ] FUNCTION
 name ( [ [ argmode ] [ argname ] argtype [ { DEFAULT | = }
default_expr ] [, ...] ] )
 [ RETURNS rettype
   | RETURNS TABLE ( column_name column_type [, ...] ) ]
   { LANGUAGE lang_name
 | TRANSFORM { FOR TYPE type_name } [, ... ]
 | WINDOW
 | IMMUTABLE | STABLE | VOLATILE | [ NOT ] LEAKPROOF
 | CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT
 | [ EXTERNAL ] SECURITY INVOKER | [ EXTERNAL ] SECURITY DEFINER
 | PARALLEL { UNSAFE | RESTRICTED | SAFE }
 | COST execution_cost
 | ROWS result_rows
 | SET configuration_parameter { TO value | = value | FROM CURRENT }
 | AS 'definition'
-| AS 'obj_file', 'link_symbol'
+| AS 'obj_file', 'link_symbol' [UNBOUNDED]
  } ...
 [ WITH ( attribute [, ...] ) ]


(I know UNBOUNDED isn't quite the word - BINDLATE would be better -
but I figured I should try to use an existing reserved keyword...)



UNBOUNDED would be terrible. It does not mean the same thing as UNBOUND.

Perhaps something like NO CHECK would meet the case, i.e. we're not 
checking the link at function creation time.


I haven't thought through the other implications yet.

cheers

andrew

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

Re: late binding of shared libs for C functions

[Andrew Gierth]

> "Andrew" == Andrew Dunstan  writes:

 Andrew> Perhaps something like NO CHECK would meet the case, i.e. we're
 Andrew> not checking the link at function creation time.

The real question is why check_function_bodies doesn't cover this;
there's a comment in fmgr_c_validator that this is deliberate, but it's
rather unclear what the advantage is supposed to be:

 * It'd be most consistent to skip the check if !check_function_bodies,
 * but the purpose of that switch is to be helpful for pg_dump loading,
 * and for pg_dump loading it's much better if we *do* check.

-- 
Andrew (irc:RhodiumToad)

Re: late binding of shared libs for C functions

[Andrew Dunstan]

On 06/12/2018 08:46 AM, Andrew Gierth wrote:

"Andrew" == Andrew Dunstan  writes:

  Andrew> Perhaps something like NO CHECK would meet the case, i.e. we're
  Andrew> not checking the link at function creation time.

The real question is why check_function_bodies doesn't cover this;
there's a comment in fmgr_c_validator that this is deliberate, but it's
rather unclear what the advantage is supposed to be:

  * It'd be most consistent to skip the check if !check_function_bodies,
  * but the purpose of that switch is to be helpful for pg_dump loading,
  * and for pg_dump loading it's much better if we *do* check.





Maybe we need a function that will validate all the links, that could be 
called after everything is installed.


cheers

andrew

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

Re: Avoid extra Sort nodes between WindowAggs when sorting can be reused

[Daniel Gustafsson]

> On 30 May 2018, at 18:19, Daniel Gustafsson  wrote:
> 
> Currently, we can only reuse Sort nodes between WindowAgg nodes iff the
> partitioning and ordering clauses are identical.  If a window Sort node
> sortorder is a prefix of another window, we could however reuse the Sort node
> to hopefully produce a cheaper plan.  In src/backend/optimizer/plan/planner.c
> there is a comment alluding to this:
> 
>* ...
>*
>* There is room to be much smarter here, for example detecting whether
>* one window's sort keys are a prefix of another's (so that sorting for
>* the latter would do for the former), or putting windows first that
>* match a sort order available for the underlying query.  For the 
> moment
>* we are content with meeting the spec.
>*/
> 
> The attached patch takes a stab at implementing the sorting on partitioning/
> ordering prefix, inspired by a similar optimization in the Greenplum planner.
> In testing the impact on planning time seems quite minimal, or within the 
> error
> margin.

Attached is a rebased v2 addressing off-list review comments and including a
test.  Parking this in the commitfest.

cheers ./daniel



window_prefix_sort-v2.patch
Description: Binary data

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Ashutosh Bapat]

On Sat, Jun 9, 2018 at 3:48 AM, Alvaro Herrera  wrote:
> On 2018-Jun-08, Alvaro Herrera wrote:
>
>> Actually, the column order doesn't matter for a trigger function,
>> because these don't refer to columns by number but by name.  So unless
>> users write trigger functions in C and use hardcoded column numbers
>> (extremely unlikely), I think this is not an issue.  In other words, in
>> the interesting cases the trigger functions are the same for all
>> partitions -- therefore upgrading from separate per-partition triggers
>> to one master trigger in the partitioned table is not going to be that
>> difficult, ISTM.
>
> One last thing.  This wording has been there since pg10; the only thing
> that changed is that it now says "BEFORE ROW triggers" instead of "Row
> triggers".  I don't think leaving it there one more year is going to get
> us too much grief, TBH.

I am worried about following scenario

-- create partitioned table
create table t1 (a int, b int) partition by range(a);

-- create some tables which will be attached to the partitioned table in future
create table t1p1 (like t1);
create table t1p2 (like t1);

-- those tables have indexes
create index t1p1_a on t1p1(a);
create index t1p2_a on t1p2(a);

-- attach partitions
alter table t1 attach partition t1p1 for values from (0) to (100);
alter table t1 attach partition t1p2 for values from (100) to (200);

-- create index on partitioned table, it doesn't create new indexes on
t1p1 and t1p2 since there are already indexes on a
create index t1_a on t1(a);

-- create triggers, user may create different trigger functions one
for each partition, unless s/he understands that the tables can share
trigger functions
create function trig_t1p1() returns trigger as $$ begin return new;
end;$$ language plpgsql;
create trigger trig_t1p1_1 before update on t1p1 execute procedure trig_t1p1();
create trigger trig_t1p1_2 before update on t1p1 execute procedure trig_t1p1();

create function trig_t1p2() returns trigger as $$ begin return new;
end;$$ language plpgsql;
create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2();

When this schema gets upgraded to v12 or whatever version supports
BEFORE ROW triggers on a partitioned table and the user tries to
create a trigger on a partitioned table
1. somehow the code has to detect that there are already triggers on
the partitions so no need to create new triggers on the partitions. I
don't see how this can be done, unless the user is smart enough to use
the same trigger function for all partitions.

2. user has to drop those triggers and trigger functions and create
trigger on the partitioned table.

May be I am underestimating users but I am sure there will be some
users who will end up with scenario.

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

Re: why partition pruning doesn't work?

[Ashutosh Bapat]

On Tue, Jun 12, 2018 at 3:54 AM, Tom Lane  wrote:
>
> Not sure about a good fix for this.  It seems annoying to copy the
> rel's whole partkey data structure into query-local storage, but
> I'm not sure we have any choice.  On the bright side, there might
> be an opportunity to get rid of repeated runtime fmgr_info lookups
> in cross-type comparison situations.
>

We already do that while building part_scheme. So, if we are in
planner, it's readily available through RelOptInfo. If we need it in
the executor, we need to pass it down from RelOptInfo into one of the
execution states. I haven't looked at the patch to exactly figure out
which of these is true.

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

Re: assert in nested SQL procedure call in current HEAD

[Peter Eisentraut]

On 6/10/18 15:06, Dmitry Dolgov wrote:
>> I added it to the open items list since nobody else seems to have taken
>> notice; from Tom's linked message it seems this should be Peter E's bag?
> I've taken a look at this - indeed, the situation looks similar to what
> described in the linked message, namely after a transaction rollback and
> creation of a new one no active snapshot was pushed. But in this particular
> case the timeframe without an active snapshot is actually limited and includes
> only some initialization and planning activity (after that a new one is
> pushed). The commentary says that "Planner must have a snapshot in case it
> calls user-defined functions." -  I tried to simulate this in order to see 
> what
> would happen, but got no errors. Is there a chance that it's an outdated
> Assert?

The problem with these nested procedure calls is that if the nested call
does a commit, you would get the dreaded "snapshot %p still active"
warning.  So PL/pgSQL in combination with SPI is going out of its way to
make sure that there is no active snapshot when we call out to
ProcessUtility() for the nested CALL.  However, if whatever the nested
CALL executes requires an active snapshot to be set, then there is this
problem.  Apparently, the LANGUAGE SQL procedure requires a snapshot to
be set previously.  This is not a problem, for example, if you replace
the example procedure with LANGUAGE plpgsql, which does its own snapshot
management.  So perhaps a fix here is to teach the LANGUAGE SQL handler
to set a snapshot if there isn't one already.  That would make the
different language handlers behave consistently.  (The alternative is to
get rid of the warning in snapmgr.c.)

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

Re: cursors with prepared statements

[Amit Kapila]

On Mon, Jun 11, 2018 at 9:56 PM, Peter Eisentraut
 wrote:
> On 6/11/18 09:57, Amit Kapila wrote:
>> Sounds like a reasonable approach.  Have you not considered using a
>> special OPEN syntax because there are some other forms of problems
>> with it?
>
> There is no OPEN command in direct SQL.  Do you mean whether I have
> considered introducing an OPEN command?
>

Yes.

>  Yes, but it seems to me that
> that would create weird inconsistencies and doesn't seem very useful in
> practice.
>

Okay, if that doesn't make the job easy, then there is not much use in
pursuing that direction.

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

Re: late binding of shared libs for C functions

[Tom Lane]

Andrew Gierth  writes:
> The real question is why check_function_bodies doesn't cover this;
> there's a comment in fmgr_c_validator that this is deliberate, but it's
> rather unclear what the advantage is supposed to be:

Error detection, ie did you spell the C symbol name correctly.

regards, tom lane

Re: assert in nested SQL procedure call in current HEAD

[Andrew Gierth]

> "Peter" == Peter Eisentraut  writes:

 Peter> The problem with these nested procedure calls is that if the
 Peter> nested call

Did you miss the fact that the issue only occurs when the top-level
procedure does a rollback? The problem is not with nested calls, but
rather with the fact that commit or rollback is leaving ActiveSnapshot
unset, which is (as Tom pointed out at the linked post) not the expected
condition inside PL code.

-- 
Andrew (irc:RhodiumToad)

Re: late binding of shared libs for C functions

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 > Andrew Gierth  writes:
 >> The real question is why check_function_bodies doesn't cover this;
 >> there's a comment in fmgr_c_validator that this is deliberate, but it's
 >> rather unclear what the advantage is supposed to be:

 Tom> Error detection, ie did you spell the C symbol name correctly.

Right, but surely restoring a dump is not the place to be doing that
error check?

-- 
Andrew (irc:RhodiumToad)

Re: late binding of shared libs for C functions

[Komяpa]

>
>  >> The real question is why check_function_bodies doesn't cover this;
>  >> there's a comment in fmgr_c_validator that this is deliberate, but it's
>  >> rather unclear what the advantage is supposed to be:
>
>  Tom> Error detection, ie did you spell the C symbol name correctly.
>
> Right, but surely restoring a dump is not the place to be doing that
> error check?
>


Similar check also happens on pg_upgrade in link mode. It would be more
useful to get it to attempt to upgrade the extension if there is absent
function or absent library error.

Re: pg_config.h.win32 missing a set of flags from pg_config.h.in added in v11 development

[Jonathan S. Katz]

> On Jun 3, 2018, at 4:29 AM, Michael Paquier  wrote:
> 
> On Sat, Jun 02, 2018 at 05:20:57PM -0400, Tom Lane wrote:
>> Heikki Linnakangas  writes:
>>> On 02/06/18 17:09, Tom Lane wrote:
 More concerning is that RHEL6 is on 1.0.1e:
>> 
>>> I was only thinking of requiring 1.0.2 on Windows.
>> 
>> Ah.  Personally, I don't care about that case, but maybe somebody
>> else wants to speak for it?
> 
> That's what I meant as well.  Cutting the minimal OpenSSL version on
> Linux would not come I think without complains so the requirements are
> way higher.  I know of companies compiling and running benchmarks of
> PostgreSQL on past RHEL and SUSE versions, linking to what's locally
> available for simplicity.
> 
> A script which reports the version of OpenSSL should be simple enough
> for MSVC.  And I am ready to bet that at least hamerkop is not using
> OpenSSL 1.0.2, so a simple switch would most likely cause the buildfarm
> to go red.  I am attaching in CC the maintainers of the Windows animals
> so as they can comment.

Wanted to check in and see if any of the Windows maintainers had thoughts
on this issue so we can continue to move it forward.

Thanks!

Jonathan

Re: late binding of shared libs for C functions

[Geoff Winkless]

On Tue, 12 Jun 2018 at 13:41, Andrew Dunstan
 wrote:
> On 06/12/2018 06:48 AM, Geoff Winkless wrote:
> > +| AS 'obj_file', 'link_symbol' [UNBOUNDED]
> > (I know UNBOUNDED isn't quite the word - BINDLATE would be better -
> > but I figured I should try to use an existing reserved keyword...)
>
> UNBOUNDED would be terrible. It does not mean the same thing as UNBOUND.

Indeed. I agree.

> Perhaps something like NO CHECK would meet the case, i.e. we're not
> checking the link at function creation time.

I did wonder about "NO CHECK" but wasn't sure if having two words
would make the parser change more complex.

Geoff

Re: pg_config.h.win32 missing a set of flags from pg_config.h.in added in v11 development

[Christian Ullrich]

*On 2018-06-12 16:21, Jonathan S. Katz wrote:


On Jun 3, 2018, at 4:29 AM, Michael Paquier  wrote:



A script which reports the version of OpenSSL should be simple enough
for MSVC.  And I am ready to bet that at least hamerkop is not using
OpenSSL 1.0.2, so a simple switch would most likely cause the buildfarm
to go red.  I am attaching in CC the maintainers of the Windows animals
so as they can comment.


Wanted to check in and see if any of the Windows maintainers had thoughts
on this issue so we can continue to move it forward.


I can't contribute much; mine all build without OpenSSL anyway.

If I enable it in the future, it will be with libraries from vcpkg [1], 
and they are at 1.0.2o now and unlikely to go backwards.


[1] https://github.com/Microsoft/vcpkg/

--
Christian

Re: late binding of shared libs for C functions

[Christian Ullrich]

* On 2018-06-12 16:35, Geoff Winkless wrote:


On Tue, 12 Jun 2018 at 13:41, Andrew Dunstan wrote:



UNBOUNDED would be terrible. It does not mean the same thing as UNBOUND.


Indeed. I agree.


Perhaps something like NO CHECK would meet the case, i.e. we're not
checking the link at function creation time.


I did wonder about "NO CHECK" but wasn't sure if having two words
would make the parser change more complex.


DEFERRED?

--
Christian

Re: pg_config.h.win32 missing a set of flags from pg_config.h.in added in v11 development

[Andrew Dunstan]

On 06/12/2018 10:40 AM, Christian Ullrich wrote:

*On 2018-06-12 16:21, Jonathan S. Katz wrote:

On Jun 3, 2018, at 4:29 AM, Michael Paquier  
wrote:



A script which reports the version of OpenSSL should be simple enough
for MSVC.  And I am ready to bet that at least hamerkop is not using
OpenSSL 1.0.2, so a simple switch would most likely cause the buildfarm
to go red.  I am attaching in CC the maintainers of the Windows animals
so as they can comment.


Wanted to check in and see if any of the Windows maintainers had 
thoughts

on this issue so we can continue to move it forward.


I can't contribute much; mine all build without OpenSSL anyway.

If I enable it in the future, it will be with libraries from vcpkg 
[1], and they are at 1.0.2o now and unlikely to go backwards.


[1] https://github.com/Microsoft/vcpkg/




Oh, nice, I will have to look into vcpkg. (This could well help in other 
contexts, too)


meanwhile, Bowerbird (MSVC) is on 1.0.1d, lorikeet (Cygwin64) is on 
1.0.2d and jacana (Mingw) doesn't build with openssl.


I will look at upgrading bowerbird ASAP.

cheers

andrew

Re: late binding of shared libs for C functions

[Geoff Winkless]

On Tue, 12 Jun 2018 at 15:44, Christian Ullrich  wrote:
> > I did wonder about "NO CHECK" but wasn't sure if having two words
> > would make the parser change more complex.
>
> DEFERRED?

That's a good shout. I wouldn't mind either of those choices.

So can I assume at least that no-one has an objection to the general principle?

I don't currently have a PG dev environment set up so it's non-trivial
for me to implement, which I'm ok with but not if I'm just wasting my
(and everyone else's) time :)

Cheers

Geoff

Re: late binding of shared libs for C functions

[Andrew Dunstan]

On 06/12/2018 11:09 AM, Geoff Winkless wrote:

On Tue, 12 Jun 2018 at 15:44, Christian Ullrich  wrote:

I did wonder about "NO CHECK" but wasn't sure if having two words
would make the parser change more complex.

DEFERRED?

That's a good shout. I wouldn't mind either of those choices.

So can I assume at least that no-one has an objection to the general principle?

I don't currently have a PG dev environment set up so it's non-trivial
for me to implement, which I'm ok with but not if I'm just wasting my
(and everyone else's) time :)





I would wait a little while. The idea's only been floated for a few hours.

cheers

andrew

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

Re: assert in nested SQL procedure call in current HEAD

[Peter Eisentraut]

On 6/12/18 10:03, Andrew Gierth wrote:
>> "Peter" == Peter Eisentraut  writes:
> 
>  Peter> The problem with these nested procedure calls is that if the
>  Peter> nested call
> 
> Did you miss the fact that the issue only occurs when the top-level
> procedure does a rollback? The problem is not with nested calls, but
> rather with the fact that commit or rollback is leaving ActiveSnapshot
> unset, which is (as Tom pointed out at the linked post) not the expected
> condition inside PL code.

We need that so that we can run things like SET TRANSACTION ISOLATION.

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

Re: why partition pruning doesn't work?

[Robert Haas]

On Mon, Jun 11, 2018 at 6:24 PM, Tom Lane  wrote:
> Not sure about a good fix for this.  It seems annoying to copy the
> rel's whole partkey data structure into query-local storage, but
> I'm not sure we have any choice.  On the bright side, there might
> be an opportunity to get rid of repeated runtime fmgr_info lookups
> in cross-type comparison situations.

Is this the same issue I raised in
https://www.postgresql.org/message-id/flat/CA%2BTgmoYKToP4-adCFFRNrO21OGuH%3Dphx-fiB1dYoqksNYX6YHQ%40mail.gmail.com
or a similar issue that creeps up at execution time?

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

Re: ON CONFLICT DO NOTHING on pg_dump

[Nico Williams]

On Tue, Jun 12, 2018 at 09:05:23AM +, Ideriha, Takeshi wrote:
> >From: Surafel Temesgen [mailto:surafel3...@gmail.com] 
> >Subject: ON CONFLICT DO NOTHING on pg_dump
> 
> >Sometimes I have to maintain two similar database and I have to update one 
> >from the other and notice having the option to add ON CONFLICT DO NOTHING 
> >clause to >INSERT command in the dump data will allows pg_restore to be done 
> >with free of ignore error.
> 
> Hi,
> I feel like that on-conflict-do-nothing support is useful especially coupled 
> with --data-only option.
> Only the difference of data can be restored.

But that's additive-only.  Only missing rows are restored this way, and
differences are not addressed.

If you want restore to restore data properly and concurrently (as
opposed to renaming a new database into place or whatever) then you'd
want a) MERGE, b) dump to generate MERGE statements.  A concurrent data
restore operation would be rather neat.

Nico
-- 

Re: late binding of shared libs for C functions

[Andres Freund]

On 2018-06-12 15:05:16 +0100, Andrew Gierth wrote:
> > "Tom" == Tom Lane  writes:
> 
>  > Andrew Gierth  writes:
>  >> The real question is why check_function_bodies doesn't cover this;
>  >> there's a comment in fmgr_c_validator that this is deliberate, but it's
>  >> rather unclear what the advantage is supposed to be:
> 
>  Tom> Error detection, ie did you spell the C symbol name correctly.
> 
> Right, but surely restoring a dump is not the place to be doing that
> error check?

I'm not convinced that that's true.  Checking that the target system has
the right shared library [version] installed isn't crazy, and you can't
do it at dump time.

If I wanted to do something about it - which I don't really - I'd argue
that check_function_bodies should become an enum or such.

Greetings,

Andres Freund

Re: why partition pruning doesn't work?

[Tom Lane]

Robert Haas  writes:
> On Mon, Jun 11, 2018 at 6:24 PM, Tom Lane  wrote:
>> Not sure about a good fix for this.  It seems annoying to copy the
>> rel's whole partkey data structure into query-local storage, but
>> I'm not sure we have any choice.  On the bright side, there might
>> be an opportunity to get rid of repeated runtime fmgr_info lookups
>> in cross-type comparison situations.

> Is this the same issue I raised in
> https://www.postgresql.org/message-id/flat/CA%2BTgmoYKToP4-adCFFRNrO21OGuH%3Dphx-fiB1dYoqksNYX6YHQ%40mail.gmail.com
> or a similar issue that creeps up at execution time?

Well, it's related to that: *if* we held the relcache entry open for
the duration of the query, and *if* holding such a pin were sufficient
to guarantee the contents of the entry's partition data couldn't change
or even move, then we could avoid doing so much copying.  But as we
discussed then, neither condition is true, and I don't think either one is
cheap to make true.  Certainly there's no logic in the relcache to detect
changes of partition data like we do for, say, triggers.

regards, tom lane

Re: [PATCH v18] GSSAPI encryption support

[Robbie Harwood]

Nico Williams  writes:

> On Mon, Jun 11, 2018 at 04:11:10PM -0400, Robbie Harwood wrote:
>> Nico was kind enough to provide me with some code review.  This should
>> those concerns (clarify short-read behavior and fixing error checking on
>> GSS functions).
>
> Besides the bug you fixed and which I told you about off-list (on IRC,
> specifically), I only have some commentary that does not need any
> action:
>
>  - support for non-Kerberos/default GSS mechanisms
>
>This might require new values for gssmode: prefer-
>and require-.  One could always use SPNEGO if there
>are multiple mechanisms to choose from.  And indeed, you could just
>use SPNEGO if the user has credentials for multiple mechanism.
>
>(Because GSS has no standard mechanism _names_, this means making
>some up.  This is one obnoxious shortcoming of the GSS-API...)

As long as it's better than passing raw OIDs on the CLI...

>  - when the SCRAM channel binding work is done, it might be good to add
>an option for TLS + GSS w/ channel binding to TLS and no gss wrap
>tokens

I think both of these are neat ideas if they'll be used.  Getting GSSAPI
encryption in shouldn't preclude either in its present form (should make
it easier, I hope), but I'm glad to hear of possible future work as
well!

Thanks,
--Robbie


signature.asc
Description: PGP signature

Re: [PATCH v18] GSSAPI encryption support

[Nico Williams]

On Tue, Jun 12, 2018 at 12:36:01PM -0400, Robbie Harwood wrote:
> Nico Williams  writes:
> > On Mon, Jun 11, 2018 at 04:11:10PM -0400, Robbie Harwood wrote:
> >> Nico was kind enough to provide me with some code review.  This should
> >> those concerns (clarify short-read behavior and fixing error checking on
> >> GSS functions).
> >
> > Besides the bug you fixed and which I told you about off-list (on IRC,
> > specifically), I only have some commentary that does not need any
> > action:
> >
> >  - support for non-Kerberos/default GSS mechanisms
> >
> >This might require new values for gssmode: prefer-
> >and require-.  One could always use SPNEGO if there
> >are multiple mechanisms to choose from.  And indeed, you could just
> >use SPNEGO if the user has credentials for multiple mechanism.
> >
> >(Because GSS has no standard mechanism _names_, this means making
> >some up.  This is one obnoxious shortcoming of the GSS-API...)
> 
> As long as it's better than passing raw OIDs on the CLI...

Rite?

I think this can be a follow-on patch, though trying SPNEGO if the user
has credentials for multiple mechanisms (and SPNEGO is indicated) seems
simple enough to do now (no interface changes).

> >  - when the SCRAM channel binding work is done, it might be good to add
> >an option for TLS + GSS w/ channel binding to TLS and no gss wrap
> >tokens
> 
> I think both of these are neat ideas if they'll be used.  Getting GSSAPI
> encryption in shouldn't preclude either in its present form (should make
> it easier, I hope), but I'm glad to hear of possible future work as
> well!

This one can (must) wait.  It has some security benefits.  You get to
use GSS/Kerberos for authentication, but you get an forward security
you'd get from TLS (if the GSS mechanism doesn't provide it, which
Kerberos today does not).

Nico
-- 

Re: why partition pruning doesn't work?

[Robert Haas]

On Tue, Jun 12, 2018 at 12:25 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Mon, Jun 11, 2018 at 6:24 PM, Tom Lane  wrote:
>>> Not sure about a good fix for this.  It seems annoying to copy the
>>> rel's whole partkey data structure into query-local storage, but
>>> I'm not sure we have any choice.  On the bright side, there might
>>> be an opportunity to get rid of repeated runtime fmgr_info lookups
>>> in cross-type comparison situations.
>
>> Is this the same issue I raised in
>> https://www.postgresql.org/message-id/flat/CA%2BTgmoYKToP4-adCFFRNrO21OGuH%3Dphx-fiB1dYoqksNYX6YHQ%40mail.gmail.com
>> or a similar issue that creeps up at execution time?
>
> Well, it's related to that: *if* we held the relcache entry open for
> the duration of the query, and *if* holding such a pin were sufficient
> to guarantee the contents of the entry's partition data couldn't change
> or even move, then we could avoid doing so much copying.  But as we
> discussed then, neither condition is true, and I don't think either one is
> cheap to make true.  Certainly there's no logic in the relcache to detect
> changes of partition data like we do for, say, triggers.

I think we DO hold relations open for the duration of execution
(though not necessarily between planning and execution).  And there is
code in RelationClearRelation to avoid changing rd_partkey and
rd_partdesc if no logical change has occurred.

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

Re: assert in nested SQL procedure call in current HEAD

[Andrew Gierth]

> "Peter" == Peter Eisentraut  writes:

 >> Did you miss the fact that the issue only occurs when the top-level
 >> procedure does a rollback? The problem is not with nested calls, but
 >> rather with the fact that commit or rollback is leaving
 >> ActiveSnapshot unset, which is (as Tom pointed out at the linked
 >> post) not the expected condition inside PL code.

 Peter> We need that so that we can run things like SET TRANSACTION
 Peter> ISOLATION.

While testing this, I ran into another semi-related issue:
shmem_exit_inprogress isn't ever being cleared in the postmaster, which
means that if you ever have a crash-restart, any attempt to do a
rollback in a procedure will then crash or get some other form of
corruption again every time until you manually restart the cluster.

-- 
Andrew (irc:RhodiumToad)

Re: why partition pruning doesn't work?

[Tom Lane]

Robert Haas  writes:
> I think we DO hold relations open for the duration of execution
> (though not necessarily between planning and execution).  And there is
> code in RelationClearRelation to avoid changing rd_partkey and
> rd_partdesc if no logical change has occurred.

Testing with valgrind + RELCACHE_FORCE_RELEASE is sufficient to disprove
that, cf current results from lousyjack (which match my own testing).
The partkey *is* disappearing under us.

While I've not looked into the exact reasons for that, my first guess
is that the partitioned table is not held open because it's not one
of the ones to be scanned.  Are you prepared to change something like
that at this stage of the release cycle?

regards, tom lane

Re: why partition pruning doesn't work?

[Tom Lane]

Robert Haas  writes:
> And there is
> code in RelationClearRelation to avoid changing rd_partkey and
> rd_partdesc if no logical change has occurred.

Oh, and by the way, what's actually in there is

keep_partkey = (relation->rd_partkey != NULL);

I would be interested to see an explanation of why that isn't utterly
broken.

regards, tom lane

Re: found xmin from before relfrozenxid on pg_catalog.pg_authid

[Andres Freund]

Hi,

On 2018-06-11 17:39:14 -0700, Andres Freund wrote:
> I plan to go over the change again tomorrow, and then push. Unless
> somebody has comments before then, obviously.

Done.

- Andres

Re: why partition pruning doesn't work?

[Robert Haas]

On Tue, Jun 12, 2018 at 12:54 PM, Tom Lane  wrote:
> Testing with valgrind + RELCACHE_FORCE_RELEASE is sufficient to disprove
> that, cf current results from lousyjack (which match my own testing).
> The partkey *is* disappearing under us.
>
> While I've not looked into the exact reasons for that, my first guess
> is that the partitioned table is not held open because it's not one
> of the ones to be scanned.  Are you prepared to change something like
> that at this stage of the release cycle?

The partition key is immutable, so it should NOT be able to disappear
out from under us.  Once you have defined the partitioning strategy
for a table and the partitioning keys associated with it, you can't
ever change it.  The only reason we need keep_partkey at all, as
opposed to just assume that the relevant portion of the relcache entry
can't change at all, is because during relation creation we are
briefly in a state where the pg_class row exists and the
pg_partitioned_table row hasn't been set up yet.  So I think your
guess that the relation is not kept open is likely to be correct.

As for whether to change it at this point in the release cycle, I
guess that, to me, depends on how invasive the fix is.

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

Re: pg_config.h.win32 missing a set of flags from pg_config.h.in added in v11 development

[Andrew Dunstan]

On 06/12/2018 10:51 AM, Andrew Dunstan wrote:





meanwhile, Bowerbird (MSVC) is on 1.0.1d, lorikeet (Cygwin64) is on 
1.0.2d and jacana (Mingw) doesn't build with openssl.


I will look at upgrading bowerbird ASAP.





Well, that crashed and burned. What versions of openssl are we supporting?

cheers

andrew

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

Attempt to fix inheritance limitations: unique and foreign key constraints

[Raphael Medaer]

Hi pg-hackers,

I'm trying to fix some of limitations in table inheritance. My first use 
case concerns "referencing" foreign keys in parent table.


The attached patch propagates foreign keys to inherited tables. I'm 
automatically cloning foreign keys from parent table into children (with 
right dependencies). It's the native implementation of work-around 
described in documentation section 5.9.1. (Caveats).


To be honest it's my first pg hack. Thus it might contain (serious) 
issues (please be lenient with it).

Furthermore it's not yet documented (TODO !!)

As far as I tested it works pretty well; the internal triggers are 
installed on CREATE TABLE .. INHERIT and even after ALTER TABLE .. INHERIT.


If you have few minutes to challenge this hack and give your advices, be 
my guest !


Kind regards,

--
Raphael Medaer
Product Development Engineer
Escaux

Escaux, Communication as easy as the web
Chaussée de Bruxelles 408, 1300 Wavre, Belgium
Direct: +3227887564
Main: +3226860900
www.escaux.com 

/Dit bericht is onderworpen aan de voorwaarden beschikbaar op onze 
website .
Ce message est soumis aux conditions disponibles sur notre site web 
.
This message is subject to the terms and conditions available on our 
website . /
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 7a6d158f89..7c15c1292b 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -1047,6 +1047,39 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId)
 	heap_close(constrRel, RowExclusiveLock);
 }
 
+bool
+relation_constraint_oid_exists(Oid relid, Oid conid)
+{
+	boolexists = false;
+	Relationpg_constraint;
+	HeapTuple   tuple;
+	SysScanDesc scan;
+	ScanKeyData key;
+
+	pg_constraint = heap_open(ConstraintRelationId, AccessShareLock);
+
+	ScanKeyInit(&key,
+			Anum_pg_constraint_conrelid,
+			BTEqualStrategyNumber,
+			F_OIDEQ,
+			ObjectIdGetDatum(relid));
+
+	scan = systable_beginscan(pg_constraint, ConstraintRelidIndexId, true,
+			NULL, 1, &key);
+
+	while (HeapTupleIsValid(tuple = systable_getnext(scan)))
+	{
+		if (HeapTupleGetOid(tuple) == conid) {
+			exists = true;
+			break;
+		}
+	}
+
+	systable_endscan(scan);
+	heap_close(pg_constraint, AccessShareLock);
+
+	return exists;
+}
 
 /*
  * get_relation_constraint_oid
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 2ea05f350b..dbb82f95bc 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -279,6 +279,48 @@ deleteDependencyRecordsForClass(Oid classId, Oid objectId,
 	return count;
 }
 
+/**
+ *
+ */
+long
+deleteDependencyRecordsForRefObject(Oid classId, Oid objectId, Oid refObjectId)
+{
+	longcount = 0;
+	ScanKeyData keys[2];
+	SysScanDesc scan;
+	Relationrel;
+	HeapTuple   tuple;
+
+	rel = heap_open(DependRelationId, RowExclusiveLock);
+
+	/* Use index to filter class/object */
+	ScanKeyInit(&keys[0],
+			Anum_pg_depend_classid,
+			BTEqualStrategyNumber, F_OIDEQ,
+			ObjectIdGetDatum(classId));
+	ScanKeyInit(&keys[1],
+			Anum_pg_depend_objid,
+			BTEqualStrategyNumber, F_OIDEQ,
+			ObjectIdGetDatum(objectId));
+	scan = systable_beginscan(rel, DependDependerIndexId, true, NULL, 2, keys);
+
+	while(HeapTupleIsValid(tuple = systable_getnext(scan)))
+	{
+		Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tuple);
+
+		if (depform->refobjid == refObjectId) {
+			CatalogTupleDelete(rel, &tuple->t_self);
+			count++;
+		}
+	}
+
+	systable_endscan(scan);
+	heap_close(rel, RowExclusiveLock);
+
+	return count;
+}
+
+
 /*
  * Adjust dependency record(s) to point to a different object of the same type
  *
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d20991a1e5..fbbbf59afa 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -338,6 +338,10 @@ static void validateCheckConstraint(Relation rel, HeapTuple constrtup);
 static void validateForeignKeyConstraint(char *conname,
 			 Relation rel, Relation pkrel,
 			 Oid pkindOid, Oid constraintOid);
+static void
+createForeignKeyCheckTriggers(Oid myRelOid, Oid refRelOid,
+			  Constraint *fkconstraint, Oid constraintOid,
+			  Oid indexOid);
 static void ATController(AlterTableStmt *parsetree,
 			 Relation rel, List *cmds, bool recurse, LOCKMODE lockmode);
 static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
@@ -410,7 +414,7 @@ static ObjectAddress ATAddCheckConstraint(List **wqueue,
 	 LOCKMODE lockmode);
 static ObjectAddress ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab,
 		  Relation rel, Constraint *fkconstraint, Oid parentConstr,
-		  bool recurse, bool recursing,
+		  bool

Re: [bug fix] Cascaded standby cannot start after a clean shutdown

[Michael Paquier]

On Tue, Jun 12, 2018 at 04:27:50PM +0900, Michael Paquier wrote:
> On Tue, Jun 12, 2018 at 06:30:49AM +, Tsunakawa, Takayuki wrote:
>> Thank you so much.  This version looks better.
>> 
>> + * this would cause the instance to stop suddendly with a hard failure,
>> 
>> suddendly -> suddenly
> 
> Yep.  Thanks for the extra lookup.

Note for everybody on this list: I will be out for a couple of days at
the end of this week, and my intention is to finish wrapping this patch
at the beginning of next week, with a back-patch down to 9.5 where
palloc_extended has been introduced as this has been seen a couple of
times on -bugs or -hackers.  Feel free of course to comment and look at
the thread for more background on the matter.
--
Michael


signature.asc
Description: PGP signature

Re: pg_config.h.win32 missing a set of flags from pg_config.h.in added in v11 development

[Michael Paquier]

On Tue, Jun 12, 2018 at 04:51:59PM -0400, Andrew Dunstan wrote:
> On 06/12/2018 10:51 AM, Andrew Dunstan wrote:
>> meanwhile, Bowerbird (MSVC) is on 1.0.1d, lorikeet (Cygwin64) is on
>> 1.0.2d and jacana (Mingw) doesn't build with openssl.
>> 
>> I will look at upgrading bowerbird ASAP.
> 
> Well, that crashed and burned. What versions of openssl are we supporting?

What kind of failures are you seeing?  I just compiled Postgres two days
ago with MSVC and OpenSSL 1.0.2o (oldest version with a Windows
installer I could find), and that was able to compile.  On HEAD, OpenSSL
should be supported down to 0.9.8.  This thread discusses about whether
we want to enforce HAVE_X509_GET_SIGNATURE_NID unconditionally or not,
as it is disabled now.  Even if the code is linked to 1.0.2 and the flag
is not set, then the code should be able to compile.
--
Michael


signature.asc
Description: PGP signature

Re: Does logical replication supports cross platform servers?

[Haribabu Kommi]

On Tue, Jun 12, 2018 at 1:29 PM Craig Ringer  wrote:

> On 12 June 2018 at 11:04, Haribabu Kommi  wrote:
>
>> Hi All,
>>
>> I am not able to find any docs suggesting that PostgreSQL logical
>> replication supports
>> cross platform servers (windows --> Linux or vice-versa).
>>
>>
> It should. I don't think there's buildfarm coverage yet, though.
>

Thanks for the confirmation.
This is a good use case that must be explicitly mentioned in the docs.
How about the attached patch?

Regards,
Haribabu Kommi
Fujitsu Australia


0001-Another-use-case-of-logical-replication.patch
Description: Binary data

Re: pg_config.h.win32 missing a set of flags from pg_config.h.in added in v11 development

[Michael Paquier]

On Wed, Jun 13, 2018 at 09:07:20AM +0900, Michael Paquier wrote:
> What kind of failures are you seeing?  I just compiled Postgres two days
> ago with MSVC and OpenSSL 1.0.2o (oldest version with a Windows
> installer I could find), and that was able to compile.  On HEAD, OpenSSL
> should be supported down to 0.9.8.  This thread discusses about whether
> we want to enforce HAVE_X509_GET_SIGNATURE_NID unconditionally or not,
> as it is disabled now.  Even if the code is linked to 1.0.2 and the flag
> is not set, then the code should be able to compile.

So, I was looking at this part this morning, and I would suggest the
attached, which enables HAVE_X509_GET_SIGNATURE_NID and
HAVE_SSL_CLEAR_OPTIONS, raising the bar to have at least OpenSSL 1.0.2
on Windows (that's the minimum version easily findable when it comes to
MSI installers anyway these days).  I have checked that the code is able
to compile correctly as well.

HAVE_LDAP_INITIALIZE is added in the list, but this is disabled as I
could not test it.  It could always be possible to revisit that later.
Thomas, what do you think?

Thoughts?
--
Michael
From 0ea00afc34063aa44f5203c5dc39f00d2108fbaf Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 13 Jun 2018 10:55:05 +0900
Subject: [PATCH] Track new configure flags introduced for 11 in
 pg_config.h.win32

The following set of flags mainly matter when building Postgres code
with MSVC and those have been forgotten with latest developments:
- HAVE_LDAP_INITIALIZE, added by 35c0754f, but tracked as disabled for
now.
- HAVE_X509_GET_SIGNATURE_NID, added by 054e8c6c, which is used by
SCRAM's channel binding tls-server-end-point.  Having this flag disabled
would cause this channel binding type to be unsupported for Windows
builds.
- HAVE_SSL_CLEAR_OPTIONS, added recently as of a364dfa4 to disable SSL
compression.

The second and third flags are enabled with this commit, which raises
the bar of OpenSSL support to 1.0.2 on Windows as minimum.  As this is
the TLS version of community and knowing that all recent installers
referred by upstream don't have anymore 1.0.1 or older, we could live
with that requirement.

Author: Michael Paquier
Discussion: https://postgr.es/m/20180529211559.gf6...@paquier.xyz
---
 src/include/pg_config.h.win32 | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 2c701fa718..ce63f3ef10 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -233,6 +233,9 @@
 /* Define to 1 if you have the  header file. */
 /* #undef HAVE_LDAP_H */
 
+/* Define to 1 if you have the `ldap_initialize' function. */
+/* #undef HAVE_LDAP_INITIALIZE */
+
 /* Define to 1 if you have the `crypto' library (-lcrypto). */
 /* #undef HAVE_LIBCRYPTO */
 
@@ -361,6 +364,9 @@
 /* Define to 1 if you have the `srandom' function. */
 /* #undef HAVE_SRANDOM */
 
+/* Define to 1 if you have the `SSL_clear_options' function. */
+#define HAVE_SSL_CLEAR_OPTIONS 1
+
 /* Define to 1 if you have the `SSL_get_current_compression' function. */
 #define HAVE_SSL_GET_CURRENT_COMPRESSION 1
 
@@ -543,6 +549,9 @@
 /* Define to 1 if you have the  header file. */
 /* #undef HAVE_WINLDAP_H */
 
+/* Define to 1 if you have the `X509_get_signature_nid' function. */
+#define HAVE_X509_GET_SIGNATURE_NID 1
+
 /* Define to 1 if the system has the type `_Bool'. */
 /* #undef HAVE__BOOL */
 
-- 
2.17.1



signature.asc
Description: PGP signature

Re: Needless additional partition check in INSERT?

[Amit Langote]

On 2018/06/12 10:37, David Rowley wrote:
> On 12 June 2018 at 09:13, Alvaro Herrera  wrote:
>> Hearing no complaints I pushed it with the proposed shape.
> 
> Thanks for working on it and pushing.

Thank you David and Alvaro.  I think the last solution involving calling
ExecPartitionCheck directly seems to be the best.

Regards,
Amit

Re: pg_config.h.win32 missing a set of flags from pg_config.h.in added in v11 development

[Thomas Munro]

On Wed, Jun 13, 2018 at 2:06 PM, Michael Paquier  wrote:
> HAVE_LDAP_INITIALIZE is added in the list, but this is disabled as I
> could not test it.  It could always be possible to revisit that later.
> Thomas, what do you think?

It should be disabled on Windows, as you have it.

ldap_initialize() is a non-standard extension that provides a way to
use "ldaps" with OpenLDAP, but we don't even support using OpenLDAP on
Windows.  We know how to use Win32's non-standard extension
ldap_sslinit() instead if WIN32 is defined.

The reason we have a configure test is because ancient OpenLDAP as
shipped with ancient macOS and RHEL5 (present in our build farm) don't
have the function, and commercial LDAP API implementations that ship
with big iron Unices probably don't have it either.  It's not in
"ldapext-ldap-c-api-05.txt", which seems to be the last known version
of the eternally drafty standard.  Effectively, we'll fall back to
ldap_init() and disallow "ldaps" unless you have a fairly recent
OpenLDAP or Windows (I suspect that covers > 99% of users).

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

Re: pg_config.h.win32 missing a set of flags from pg_config.h.in added in v11 development

[Michael Paquier]

On Wed, Jun 13, 2018 at 02:35:23PM +1200, Thomas Munro wrote:
> It should be disabled on Windows, as you have it.
> 
> ldap_initialize() is a non-standard extension that provides a way to
> use "ldaps" with OpenLDAP, but we don't even support using OpenLDAP on
> Windows.  We know how to use Win32's non-standard extension
> ldap_sslinit() instead if WIN32 is defined.

Ah, OK.  I did not follow closely enough the thread about ldaps
myself. Thanks for the confirmation.
--
Michael


signature.asc
Description: PGP signature

Re: Fix some error handling for read() and errno

[Michael Paquier]

On Tue, Jun 12, 2018 at 01:19:54PM +0900, Michael Paquier wrote:
> Agreed.  I also quite like the message mentioning directly 2PC files as
> well.  I think that we could gain by making all end messages more
> consistent, as the markers used and the style of each message is
> slightly different, so I would suggest something like that instead to
> gain in consistency:
> if (readBytes < 0)
> ereport(elevel, "could not blah: %m");
> else
> ereport(elevel, "could not blah: %d read, expected %zu");
> 
> My point is that if we use the same markers and the same end messages,
> then those are easier to grep for, and callers are still free to provide
> the head of error messages the way they want depending on the situation.

I have dug again into this stuff, and I have finished with the attached
which uses mainly "could not read file %s: read %d bytes, expected
%zu".  The markers are harder to make consistent without being more
invasive so I stopped on that.

There is also this bit in slru.c which I'd like to discuss:
+   /*
+* Note that this would report success if the number of bytes read is
+* positive, but lacking data so that errno is not set, which would be
+* confusing, so set errno to EIO in this case.
+*/
+   if (errno == 0)
+   errno = EIO;
Please note that I don't necessarily propose to add this in the final
patch, and I think that at least an XXX comment should be added here to
mention that errno may not be set.

Thoughts?
--
Michael
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 1132eef038..805aa521cf 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -684,6 +684,14 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
 	if (read(fd, shared->page_buffer[slotno], BLCKSZ) != BLCKSZ)
 	{
 		pgstat_report_wait_end();
+
+		/*
+		 * Note that this would report success if the number of bytes read is
+		 * positive, but lacking data so that errno is not set, which would be
+		 * confusing, so set errno to EIO in this case.
+		 */
+		if (errno == 0)
+			errno = EIO;
 		slru_errcause = SLRU_READ_FAILED;
 		slru_errno = errno;
 		CloseTransientFile(fd);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 65194db70e..95ab03628f 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1219,6 +1219,7 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
 	uint32		crc_offset;
 	pg_crc32c	calc_crc,
 file_crc;
+	int			r;
 
 	TwoPhaseFilePath(path, xid);
 
@@ -1272,15 +1273,28 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
 	buf = (char *) palloc(stat.st_size);
 
 	pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_READ);
-	if (read(fd, buf, stat.st_size) != stat.st_size)
+	r = read(fd, buf, stat.st_size);
+	if (r != stat.st_size)
 	{
+		int			save_errno = errno;
+
 		pgstat_report_wait_end();
 		CloseTransientFile(fd);
 		if (give_warnings)
-			ereport(WARNING,
-	(errcode_for_file_access(),
-	 errmsg("could not read two-phase state file \"%s\": %m",
-			path)));
+		{
+			if (r < 0)
+			{
+errno = save_errno;
+ereport(WARNING,
+		(errcode_for_file_access(),
+		 errmsg("could not read two-phase state file \"%s\": %m",
+path)));
+			}
+			else
+ereport(WARNING,
+		(errmsg("could not read two-phase state file \"%s\": read %d bytes, expected %zu",
+path, r, stat.st_size)));
+		}
 		pfree(buf);
 		return NULL;
 	}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index adbd6a2126..471ef87842 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3395,21 +3395,24 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
 
 		if (nread > 0)
 		{
+			int			r;
+
 			if (nread > sizeof(buffer))
 nread = sizeof(buffer);
 			errno = 0;
 			pgstat_report_wait_start(WAIT_EVENT_WAL_COPY_READ);
-			if (read(srcfd, buffer, nread) != nread)
+			r = read(srcfd, buffer, nread);
+			if (r != nread)
 			{
-if (errno != 0)
+if (r < 0)
 	ereport(ERROR,
 			(errcode_for_file_access(),
 			 errmsg("could not read file \"%s\": %m",
 	path)));
 else
 	ereport(ERROR,
-			(errmsg("not enough data in file \"%s\"",
-	path)));
+			(errmsg("could not read file \"%s\": read %d bytes, expected %d",
+	path, r, nread)));
 			}
 			pgstat_report_wait_end();
 		}
@@ -4509,7 +4512,8 @@ ReadControlFile(void)
 	 errmsg("could not read from control file: %m")));
 		else
 			ereport(PANIC,
-	 (errmsg("could not read from control file: read %d bytes, expected %d", r, (int) sizeof(ControlFileData;
+	(errmsg("could not read from control file: read %d bytes, expected %zu",
+			r, sizeof(ControlFileData;
 	}
 	pgstat_report_wait_end();
 
@@ -11594,6 +11598,7 @@ XLogPageRead(XLogReaderState *xlogreader

Re: why partition pruning doesn't work?

[Tom Lane]

Robert Haas  writes:
> On Tue, Jun 12, 2018 at 12:54 PM, Tom Lane  wrote:
>> While I've not looked into the exact reasons for that, my first guess
>> is that the partitioned table is not held open because it's not one
>> of the ones to be scanned.  Are you prepared to change something like
>> that at this stage of the release cycle?

> The partition key is immutable, so it should NOT be able to disappear
> out from under us.

Hm.  That could be better documented.

> As for whether to change it at this point in the release cycle, I
> guess that, to me, depends on how invasive the fix is.

It seems not to be that bad: we just need a shutdown call for the
PartitionPruneState, and then we can remember the open relation there.
The attached is based on David's patch from yesterday.

I'm still a bit annoyed at the fmgr_info_copy calls in this.  It'd be
better to use the FmgrInfos in the relcache when applicable.  However,
mixing those with the cross-type ones would seem to require that we change
the API for get_matching_hash_bounds et al from taking "FmgrInfo *" to
taking "FmgrInfo **", which looks rather invasive.

regards, tom lane

diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 33513ff..47c 100644
*** a/src/backend/executor/execPartition.c
--- b/src/backend/executor/execPartition.c
*** adjust_partition_tlist(List *tlist, Tupl
*** 1357,1367 
   *
   * Functions:
   *
!  * ExecSetupPartitionPruneState:
   *		Creates the PartitionPruneState required by each of the two pruning
   *		functions.  Details stored include how to map the partition index
   *		returned by the partition pruning code into subplan indexes.
   *
   * ExecFindInitialMatchingSubPlans:
   *		Returns indexes of matching subplans.  Partition pruning is attempted
   *		without any evaluation of expressions containing PARAM_EXEC Params.
--- 1357,1370 
   *
   * Functions:
   *
!  * ExecCreatePartitionPruneState:
   *		Creates the PartitionPruneState required by each of the two pruning
   *		functions.  Details stored include how to map the partition index
   *		returned by the partition pruning code into subplan indexes.
   *
+  * ExecDestroyPartitionPruneState:
+  *		Deletes a PartitionPruneState. Must be called during executor shutdown.
+  *
   * ExecFindInitialMatchingSubPlans:
   *		Returns indexes of matching subplans.  Partition pruning is attempted
   *		without any evaluation of expressions containing PARAM_EXEC Params.
*** adjust_partition_tlist(List *tlist, Tupl
*** 1382,1389 
   */
  
  /*
!  * ExecSetupPartitionPruneState
!  *		Set up the data structure required for calling
   *		ExecFindInitialMatchingSubPlans and ExecFindMatchingSubPlans.
   *
   * 'planstate' is the parent plan node's execution state.
--- 1385,1392 
   */
  
  /*
!  * ExecCreatePartitionPruneState
!  *		Build the data structure required for calling
   *		ExecFindInitialMatchingSubPlans and ExecFindMatchingSubPlans.
   *
   * 'planstate' is the parent plan node's execution state.
*** adjust_partition_tlist(List *tlist, Tupl
*** 1395,1401 
   * in each PartitionPruneInfo.
   */
  PartitionPruneState *
! ExecSetupPartitionPruneState(PlanState *planstate, List *partitionpruneinfo)
  {
  	PartitionPruneState *prunestate;
  	PartitionPruningData *prunedata;
--- 1398,1404 
   * in each PartitionPruneInfo.
   */
  PartitionPruneState *
! ExecCreatePartitionPruneState(PlanState *planstate, List *partitionpruneinfo)
  {
  	PartitionPruneState *prunestate;
  	PartitionPruningData *prunedata;
*** ExecSetupPartitionPruneState(PlanState *
*** 1435,1445 
  		PartitionPruningData *pprune = &prunedata[i];
  		PartitionPruneContext *context = &pprune->context;
  		PartitionDesc partdesc;
- 		Relation	rel;
  		PartitionKey partkey;
- 		ListCell   *lc2;
  		int			partnatts;
  		int			n_steps;
  
  		/*
  		 * We must copy the subplan_map rather than pointing directly to the
--- 1438,1447 
  		PartitionPruningData *pprune = &prunedata[i];
  		PartitionPruneContext *context = &pprune->context;
  		PartitionDesc partdesc;
  		PartitionKey partkey;
  		int			partnatts;
  		int			n_steps;
+ 		ListCell   *lc2;
  
  		/*
  		 * We must copy the subplan_map rather than pointing directly to the
*** ExecSetupPartitionPruneState(PlanState *
*** 1456,1481 
  		pprune->present_parts = bms_copy(pinfo->present_parts);
  
  		/*
! 		 * Grab some info from the table's relcache; lock was already obtained
! 		 * by ExecLockNonLeafAppendTables.
  		 */
! 		rel = relation_open(pinfo->reloid, NoLock);
  
! 		partkey = RelationGetPartitionKey(rel);
! 		partdesc = RelationGetPartitionDesc(rel);
  
  		context->strategy = partkey->strategy;
  		context->partnatts = partnatts = partkey->partnatts;
! 		context->partopfamily = partkey->partopfamily;
! 		context->partopcintype = partkey->partopcintype;
  		context->partcollation = partkey->partco

Add function to release an allocated SQLDA

[Kato, Sho]

Hi,

I add a function called ECPGfreeSQLDA() becasue there is no API for releasing 
the SQLDA stored the result set.

An example of usage is as follows.
Specify a pointer to sqlda_t to be released as an argument. 

Example:
  exec sql begin declare section;
  char*stmt1 = "SELECT * FROM t1";
  exec sql end declare section;

  sqlda_t *outp_sqlda;

  exec sql prepare st_id2 from :stmt1;
  exec sql declare mycur2 cursor for st_id1;
  exec sql open mycur2;
  exec sql fetch all from mycur2 into descriptor outp_sqlda;
  exec sql close mycur2;
  exec sql deallocate prepare st_id2;
  ECPGfreeSQLDA(outp_sqlda);

The patch is attached.
The threads involved in this patch are as follows.

https://www.postgresql.org/message-id/25C1C6B2E7BE044889E4FE8643A58BA963A42097@G01JPEXMBKW03

https://www.postgresql.org/message-id/flat/0A3221C70F24FB45833433255569204D1F8AD5D6@G01JPEXMBYT05#0A3221C70F24FB45833433255569204D1F8AD5D6@G01JPEXMBYT05

regards,

--
Kato Sho



Add-ECPGfreeSQLDA.patch
Description: Add-ECPGfreeSQLDA.patch

Re: BUG #15237: I got "ERROR: source for a multiple-column UPDATE item must be a sub-SELECT or ROW() expression"

[Andrew Gierth]

[Pruning the CC list and moving this to -hackers]

> "Tom" == Tom Lane  writes:

 Tom> It's telling you what to do: use a ROW() expression, ie
 Tom> update fvt_obj_operate_update_table_033 set (c_int) = row(20)
 Tom> where c_int = 20;

 >> Yeah, but (a) this used to work, and has worked since at least as
 >> far back as 9.0, and (b) the spec requires it to work.

 Tom> As far as (a) goes, it was an intentional compatibility breakage,

So here I have a problem: the fact that it was a compatibility breakage
seems not to have been discussed *or even so much as mentioned* on
-hackers at any time.

The original patch, a self-described "lazy-sunday project", made
absolutely no mention of any compatibility issue, simply discussed the
new options it was allowing, and got no feedback or review. Had it
mentioned or even hinted that existing queries might be broken, I would
hope that there would have been more discussion at the time.

It then apparently went unnoticed until after the release of pg 10, at
which point it got retroactively documented (in the release notes and
nowhere else), in response to a brief discussion of a user complaint
that happened on -general and not -hackers (or even -bugs).

 Tom> cf commits 86182b189 and 906bfcad7,

I encourage everyone to refer to these and decide whether this is
sufficient grounds or sufficient discussion to introduce a breaking
change, even a minor one.

 Tom> and as for (b), the SQL committee is just nuts here.

We all know that the spec is a dog's breakfast, yes.

But breaking backwards compatibility AND the spec, even if it's in order
to support other parts of the spec, needs to be something that's
actually discussed and the tradeoff assessed rather than being snuck
through not only without discussion but also without even asking "is
there any sane way we can avoid breaking this".

 Tom> The expectation in 906bfcad7 was that we'd move towards the
 Tom> *other* thing the spec demands, namely that the RHS can be any
 Tom> a_expr yielding a suitable row value.

The spec doesn't have anything that corresponds to our a_expr,
obviously.

 Tom> We can't do that if it's unclear what is or isn't a ROW()
 Tom> expression, because then the intended semantics would be
 Tom> undecidable.

As far as I can tell, the spec distinguishes a lot of these cases only
by means of the type of the value.

For example:

 can be a  which is a 
which must have a row type. (Note that for example (select row(1,2))
is a nonparenthesized value expression primary even though it has parens
around it.)

Or  can be a  which can be one of several <$type value expression>
productions all of which contain  as one of the possible expansions. So for example
(select 1) ends up being treated as row((select 1)).

 Tom> And I don't think we want a situation in which adding "extra"
 Tom> parens changes a legal command into an illegal one.

To some extent I agree, but the spec does rely on this in places:

 Tom> For example, suppose f(...) returns a single-column tuple result.
 Tom> This should be legal, if x matches the type of the single column:

 Tom>   update ... set (x) = f(...)

 Tom> but if we try to do what you seem to have in mind, this would not
 Tom> be:

 Tom>   update ... set (x) = (f(...))

The spec indeed says that this is not valid, since it would wrap an
additional ROW() around the result, and would try and assign the row
value to x.

 Tom> That's sufficiently brain-dead that I don't think we want to go
 Tom> there.

Maybe so, but not without discussion.

-- 
Andrew (irc:RhodiumToad)

Re: why partition pruning doesn't work?

[David Rowley]

On 13 June 2018 at 16:15, Tom Lane  wrote:
> It seems not to be that bad: we just need a shutdown call for the
> PartitionPruneState, and then we can remember the open relation there.
> The attached is based on David's patch from yesterday.
>
> I'm still a bit annoyed at the fmgr_info_copy calls in this.  It'd be
> better to use the FmgrInfos in the relcache when applicable.  However,
> mixing those with the cross-type ones would seem to require that we change
> the API for get_matching_hash_bounds et al from taking "FmgrInfo *" to
> taking "FmgrInfo **", which looks rather invasive.

I've looked over this and it seems better than mine. Especially so
that you've done things so that the FmgrInfo is copied into a memory
context that's not about to get reset.

One small thing is that I'd move the:

context.partrel = NULL;

to under:

/* These are not valid when being called from the planner */

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

Re: Portability concerns over pq_sendbyte?

[Michael Paquier]

On Mon, Jun 11, 2018 at 02:25:44PM +0900, Michael Paquier wrote:
> On Wed, Jun 06, 2018 at 12:27:58PM -0400, Alvaro Herrera wrote:
>> Do you have an answer to this question?  Does anybody else?
>> 
>> (My guts tell me it'd be better to change these routines to take
>> unsigned values, without creating extra variants.  But guts frequently
>> misspeak.)
> 
> My guts are telling me as well to not have more variants.  On top of
> that it seems to me that we'd want to rename any new routines to include
> "uint" in their name instead of "int", and for compatibility with past
> code pq_sendint should not be touched.

And also pq_sendint64 needs to be kept around for compatibility.  I have
quickly looked at how much code would be involved here and there are
quite close to 240 code paths which involve the new routines.  Please
see attached for reference, I have not put much thoughts into it to be
honest, so that's really at an early stage.
--
Michael
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index 745497c76f..7c8da5bf22 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -1234,23 +1234,23 @@ hstore_send(PG_FUNCTION_ARGS)
 
 	pq_begintypsend(&buf);
 
-	pq_sendint32(&buf, count);
+	pq_senduint32(&buf, count);
 
 	for (i = 0; i < count; i++)
 	{
 		int32		keylen = HSTORE_KEYLEN(entries, i);
 
-		pq_sendint32(&buf, keylen);
+		pq_senduint32(&buf, keylen);
 		pq_sendtext(&buf, HSTORE_KEY(entries, base, i), keylen);
 		if (HSTORE_VALISNULL(entries, i))
 		{
-			pq_sendint32(&buf, -1);
+			pq_senduint32(&buf, -1);
 		}
 		else
 		{
 			int32		vallen = HSTORE_VALLEN(entries, i);
 
-			pq_sendint32(&buf, vallen);
+			pq_senduint32(&buf, vallen);
 			pq_sendtext(&buf, HSTORE_VAL(entries, base, i), vallen);
 		}
 	}
diff --git a/src/backend/access/common/printsimple.c b/src/backend/access/common/printsimple.c
index 3c4d227712..c5aade3e44 100644
--- a/src/backend/access/common/printsimple.c
+++ b/src/backend/access/common/printsimple.c
@@ -34,19 +34,19 @@ printsimple_startup(DestReceiver *self, int operation, TupleDesc tupdesc)
 	int			i;
 
 	pq_beginmessage(&buf, 'T'); /* RowDescription */
-	pq_sendint16(&buf, tupdesc->natts);
+	pq_senduint16(&buf, tupdesc->natts);
 
 	for (i = 0; i < tupdesc->natts; ++i)
 	{
 		Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
 
 		pq_sendstring(&buf, NameStr(attr->attname));
-		pq_sendint32(&buf, 0);	/* table oid */
-		pq_sendint16(&buf, 0);	/* attnum */
-		pq_sendint32(&buf, (int) attr->atttypid);
-		pq_sendint16(&buf, attr->attlen);
-		pq_sendint32(&buf, attr->atttypmod);
-		pq_sendint16(&buf, 0);	/* format code */
+		pq_senduint32(&buf, 0);	/* table oid */
+		pq_senduint16(&buf, 0);	/* attnum */
+		pq_senduint32(&buf, (uint32) attr->atttypid);
+		pq_senduint16(&buf, attr->attlen);
+		pq_senduint32(&buf, attr->atttypmod);
+		pq_senduint16(&buf, 0);	/* format code */
 	}
 
 	pq_endmessage(&buf);
@@ -67,7 +67,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self)
 
 	/* Prepare and send message */
 	pq_beginmessage(&buf, 'D');
-	pq_sendint16(&buf, tupdesc->natts);
+	pq_senduint16(&buf, tupdesc->natts);
 
 	for (i = 0; i < tupdesc->natts; ++i)
 	{
@@ -76,7 +76,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self)
 
 		if (slot->tts_isnull[i])
 		{
-			pq_sendint32(&buf, -1);
+			pq_senduint32(&buf, -1);
 			continue;
 		}
 
diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index a1d4415704..8b85e34baf 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -203,7 +203,7 @@ SendRowDescriptionMessage(StringInfo buf, TupleDesc typeinfo,
 	/* tuple descriptor message type */
 	pq_beginmessage_reuse(buf, 'T');
 	/* # of attrs in tuples */
-	pq_sendint16(buf, natts);
+	pq_senduint16(buf, natts);
 
 	if (proto >= 3)
 		SendRowDescriptionCols_3(buf, typeinfo, targetlist, formats);
@@ -280,12 +280,12 @@ SendRowDescriptionCols_3(StringInfo buf, TupleDesc typeinfo, List *targetlist, i
 			format = 0;
 
 		pq_writestring(buf, NameStr(att->attname));
-		pq_writeint32(buf, resorigtbl);
-		pq_writeint16(buf, resorigcol);
-		pq_writeint32(buf, atttypid);
-		pq_writeint16(buf, att->attlen);
-		pq_writeint32(buf, atttypmod);
-		pq_writeint16(buf, format);
+		pq_writeuint32(buf, resorigtbl);
+		pq_writeuint16(buf, resorigcol);
+		pq_writeuint32(buf, atttypid);
+		pq_writeuint16(buf, att->attlen);
+		pq_writeuint32(buf, atttypmod);
+		pq_writeuint16(buf, format);
 	}
 }
 
@@ -309,9 +309,9 @@ SendRowDescriptionCols_2(StringInfo buf, TupleDesc typeinfo, List *targetlist, i
 
 		pq_sendstring(buf, NameStr(att->attname));
 		/* column ID only info appears in protocol 3.0 and up */
-		pq_sendint32(buf, atttypid);
-		pq_sendint16(buf, att->attlen);
-		pq_sendint32(buf, atttypmod);
+		pq_senduint32(buf, atttypid);
+		pq_senduint16(buf, att->attlen);
+		pq_senduint32(buf, atttypmod);
 		/* format info only appears in protocol 3.0 and up */
 	}
 }
@@ -395,7 +395,7 @@ printtup(Tuple

Server crashed with dense_rank on partition table.

[Rajkumar Raghuwanshi]

Hi,

I am getting server crash with below query.

CREATE TABLE pagg_tab (a int, b int, c text) PARTITION BY LIST(c);
CREATE TABLE pagg_tab_p1 PARTITION OF pagg_tab FOR VALUES IN ('',
'0001', '0002', '0003');
CREATE TABLE pagg_tab_p2 PARTITION OF pagg_tab FOR VALUES IN ('0004',
'0005', '0006', '0007');
CREATE TABLE pagg_tab_p3 PARTITION OF pagg_tab FOR VALUES IN ('0008',
'0009', '0010', '0011');
INSERT INTO pagg_tab SELECT i % 20, i % 30, to_char(i % 12, 'FM') FROM
generate_series(0, 36) i;
ANALYZE pagg_tab;
SELECT dense_rank(b) WITHIN GROUP (ORDER BY a) FROM pagg_tab GROUP BY b
ORDER BY 1;

postgres=# SELECT dense_rank(b) WITHIN GROUP (ORDER BY a) FROM pagg_tab
GROUP BY b ORDER BY 1;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

logfile have this

2018-06-12 21:29:54.930 IST [69580] STATEMENT:  drop table pagg_tab;
TRAP: BadArgument("!(((context) != ((void *)0) && (const
Node*)((context)))->type) == T_AllocSetContext) || const
Node*)((context)))->type) == T_SlabContext) || const
Node*)((context)))->type) == T_GenerationContext", File: "mcxt.c",
Line: 775)
2018-06-12 21:29:55.552 IST [69571] LOG:  server process (PID 69580) was
terminated by signal 6: Aborted
2018-06-12 21:29:55.552 IST [69571] DETAIL:  Failed process was running:
SELECT dense_rank(b) WITHIN GROUP (ORDER BY a) FROM pagg_tab GROUP BY b
ORDER BY 1;
2018-06-12 21:29:55.552 IST [69571] LOG:  terminating any other active
server processes

and here is core file content

Loaded symbols for /lib64/libnss_files.so.2
Core was generated by `postgres: edb postgres [local]
SELECT   '.
Program terminated with signal 6, Aborted.
#0  0x003dd2632495 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
64  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
Missing separate debuginfos, use: debuginfo-install
keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
libcom_err-1.41.12-23.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
openssl-1.0.1e-57.el6.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0  0x003dd2632495 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x003dd2633c75 in abort () at abort.c:92
#2  0x00a32622 in ExceptionalCondition (
conditionName=0xc99320 "!(((context) != ((void *)0) && (const
Node*)((context)))->type) == T_AllocSetContext) || const
Node*)((context)))->type) == T_SlabContext) || const
Node*)((context)))->type) == T_Generatio"..., errorType=0xc99312
"BadArgument", fileName=0xc993f5 "mcxt.c", lineNumber=775) at assert.c:54
#3  0x00a6be3a in MemoryContextAlloc (context=0x134a708, size=8) at
mcxt.c:775
#4  0x00a6d0a6 in MemoryContextStrdup (context=0x134a708,
string=0xc60413 "integer") at mcxt.c:1153
#5  0x00a6d0e9 in pstrdup (in=0xc60413 "integer") at mcxt.c:1163
#6  0x00927328 in format_type_extended (type_oid=23, typemod=-1,
flags=0) at format_type.c:224
#7  0x009275c4 in format_type_be (type_oid=23) at format_type.c:330
#8  0x006d41ed in CheckVarSlotCompatibility (slot=0x134a6a8,
attnum=1, vartype=23) at execExprInterp.c:1883
#9  0x006d4062 in CheckExprStillValid (state=0x1388370,
econtext=0x134a6a8) at execExprInterp.c:1823
#10 0x006d3f5e in ExecInterpExprStillValid (state=0x1388370,
econtext=0x134a6a8, isNull=0x7ffe988f3907) at execExprInterp.c:1780
#11 0x0098a116 in ExecEvalExprSwitchContext (state=0x1388370,
econtext=0x134a6a8, isNull=0x7ffe988f3907) at
../../../../src/include/executor/executor.h:303
#12 0x0098a191 in ExecQual (state=0x1388370, econtext=0x134a6a8) at
../../../../src/include/executor/executor.h:372
#13 0x0098a1e3 in ExecQualAndReset (state=0x1388370,
econtext=0x134a6a8) at ../../../../src/include/executor/executor.h:389
#14 0x0098cb96 in hypothetical_dense_rank_final
(fcinfo=0x7ffe988f3a40) at orderedsetaggs.c:1389
#15 0x006f3a5d in finalize_aggregate (aggstate=0x1368f98,
peragg=0x1382a38, pergroupstate=0x1382be8, resultVal=0x13829f8,
resultIsNull=0x1382a18) at nodeAgg.c:965
#16 0x006f3ff0 in finalize_aggregates (aggstate=0x1368f98,
peraggs=0x1382a38, pergroup=0x1382be8) at nodeAgg.c:1172
#17 0x006f516a in agg_retrieve_direct (aggstate=0x1368f98) at
nodeAgg.c:1887
#18 0x006f4a6d in ExecAgg (pstate=0x1368f98) at nodeAgg.c:1551
#19 0x00718972 in ExecProcNode (node=0x1368f98) at
../../../src/include/executor/executor.h:237
#20 0x00718abe in ExecSort (pstate=0x1368e80) at nodeSort.c:107
#21 0x006e6b26 in ExecProcNodeFirst (node=0x1368e80) at
execProcnode.c:445
#22 0x006dbd61 in ExecProcNode (node=0x1368e80) at
../../../src/include/executor/executor.h:237
#23 0x006de71b in ExecutePlan (estate=0x1368c68,
planstate=0x1368e80, use_parallel_mode=false, operation=CMD_SELECT,
sendTuples=true,

Re: Server crashed with dense_rank on partition table.

[Michael Paquier]

On Wed, Jun 13, 2018 at 11:08:38AM +0530, Rajkumar Raghuwanshi wrote:
> postgres=# SELECT dense_rank(b) WITHIN GROUP (ORDER BY a) FROM pagg_tab
> GROUP BY b ORDER BY 1;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.

Indeed, thanks for the test case.  This used to work in v10 but this is
failing with v11 so I am adding an open item.  The plans of the pre-10
query and the query on HEAD are rather similar, and the memory context
at execution time looks messed up.
--
Michael


signature.asc
Description: PGP signature

Re: Duplicate Item Pointers in Gin index

[Masahiko Sawada]

On Thu, Feb 22, 2018 at 10:26 AM, Masahiko Sawada  wrote:
> On Thu, Feb 22, 2018 at 8:28 AM, Peter Geoghegan  wrote:
>> On Wed, Feb 21, 2018 at 3:02 PM, R, Siva  wrote:
>>> Did you mean pin on the metapage buffer during ginInsertCleanup and not lock
>>> during addition of tuples to the accumulator? The exclusive lock on metapage
>>> buffer is released after reading/locking head of pending list and before we
>>> process pages/add tuples to the accumulator in ginInsertCleanup [1].
>>
>> AFAICT, nobody ever holds just a pin on the metapage as some kind of
>> interlock (since nobody else ever acquires a "super exclusive lock" on
>> the metapage -- if anybody else ever did that, then simply holding a
>> pin might make sense as a way of blocking the "super exclusive" lock
>> acquisition). Maybe you're thinking of the root page of posting trees?
>>
>> I think that Sawada-san simply means that holding an ExclusiveLock on
>> the metapage makes writers block each other, and concurrent VACUUMs.
>> At least, for as long as they're in ginInsertCleanup().
>
> Yes, but I realized my previous mail was wrong, sorry. Insertion to
> pending list doesn't acquire ExclusiveLock on metapage. So we can
> insert tuples to pending list while cleaning up.
>

Sorry for the very late response.

FWIW, I've looked at this again. I think that the situation Siva
reported in the first mail can happen before we get commit 3b2787e.
That is, gin indexes had had a data corruption bug. I've reproduced
the situation with PostgreSQL 10.1 and observed that a gin index can
corrupt. However, gingetbitmap (fortunately?) returned a correct
result even when the gin index is corrupted.

The minimum situation I reproduced is that each gin entry has two
pointers to the same TID as follows.

gin-entry 1 gin-entry2
(1, 147) (1, 147)
(1, 147) (1, 147)

The above situation is surely corrupted where I executed the all steps
Siva described in the first mail. The first TID of both entries points
to an already-vacuumed itempointer (the tuple is inserted, deleted and
vacuumed), whereas the second entries points to a live itempointer on
heap. In entryGetItem, since we check advancePast it doesn't return
the second TIDs in both posting list case and posting tree case. Also
even in partial match case, since TIDbitmap eliminates the duplication
entryGetItem can return a correct result. The corrupted gin index
returned a correct result actually but no assertion failure happened.

I'm not sure how you figured this duplicated item pointers issue out
but what I got through investigating this issue is that gin indexes
could return a correct result without no assertion failure even if it
somewhat corrupted. So maybe having amcheck for gin indexes would
resolve part of problems.

Regards,

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

Re: Duplicate Item Pointers in Gin index

[Peter Geoghegan]

On Tue, Jun 12, 2018 at 11:01 PM, Masahiko Sawada  wrote:
> FWIW, I've looked at this again. I think that the situation Siva
> reported in the first mail can happen before we get commit 3b2787e.
> That is, gin indexes had had a data corruption bug. I've reproduced
> the situation with PostgreSQL 10.1 and observed that a gin index can
> corrupt.

So, you've recreated the problem with Postgres from before 3b2787e,
but not after 3b2787e? Are you suggesting that 3b2787e might have
fixed it, or that it only hid the problem, or something else?

How did you recreate the problem? Do you have a test case you can share?

> However, gingetbitmap (fortunately?) returned a correct
> result even when the gin index is corrupted.

That's not really surprising.

> I'm not sure how you figured this duplicated item pointers issue out
> but what I got through investigating this issue is that gin indexes
> could return a correct result without no assertion failure even if it
> somewhat corrupted. So maybe having amcheck for gin indexes would
> resolve part of problems.

That seems like a really good idea. As you know, I have misgivings
about this area.

-- 
Peter Geoghegan