date:20170612

Hi Ashutosh,

On 2017/06/12 20:56, Ashutosh Bapat wrote:
> Hi,
> If we detach a partition and drop the corresponding partitioned table,
> it drops the once-partition now-standalone table as well.

Thanks for the report.  Looks like 8b4d582d279d78 missed the bit about
drop_parent_dependency() that you describe in your analysis below.

> The reason for this is as follows
> An AUTO dependency is recorded between a partitioned table and partition. 
> In
> case of inheritance we record a NORMAL dependency between parent
> and child. While
> detaching a partition, we call RemoveInheritance(), which should have 
> taken
> care of removing this dependency. But it removes the dependencies which 
> are
> marked as NORMAL. When we changed the dependency for partitioned case from
> NORMAL to AUTO by updating StoreCatalogInheritance1(), this function was 
> not
> updated. This caused the dependency to linger behind even after
> detaching the
> partition, thus causing the now standalone table (which was once a
> partition)
> to be dropped when the parent partitioned table is dropped. This patch 
> fixes
> RemoveInheritance() to choose appropriate dependency.
> 
> Attached patch 0001 to fix this.

Looks good to me.  Perhaps, the example in your email could be added as a
test case.

> I see a similar issue-in-baking in case we change the type of
> dependency recorded between a table and the composite type associated
> with using CREATE TABLE ... OF command. 0002 patch addresses the
> potential issue by using a macro while creating and dropping the
> dependency in corresponding functions. There might be more such
> places, but I haven't searched for those.

Might be a good idea too.

Adding this to the open items list.

Thanks,
Amit



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

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

On Tue, Jun 13, 2017 at 12:03 AM, Dean Rasheed  wrote:
> On 12 June 2017 at 17:51, Joe Conway  wrote:
>> On 06/12/2017 07:40 AM, Joe Conway wrote:
>>> On 06/12/2017 01:49 AM, Amit Langote wrote:
 As he mentioned in his reply, Ashutosh's proposal to abstract away the
 relkind checks is interesting in this regard.

>>> I have not looked at Ashutosh's patch yet, but it sounds like a good
>>> idea to me.
>>
>> After looking I remain convinced - +1 in general.
>>
>
> Yes, I think this will probably help, but I worry that it will turn
> into quite a large and invasive patch, and there are a number of
> design choices to be made over the naming and precise set of macros.
> Is this really PG10 material?

No this is not for PG10.

>
> My initial thought, looking at the patch, is that it might be better
> to have all the macros in one file to make them easier to maintain.
>

Right now the macros are listed just below relkind enum in pg_class.h.
Is that a good place or do you think, we should list them in a
separate file?

>
> Barring objections, I'll push my original patch and work up patches
> for the other couple of issues I found.

No objections, the patch is good to go as is. Sorry for high-jacking
this thread.

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


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

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

On Mon, Jun 12, 2017 at 10:21 PM, Joe Conway  wrote:
> On 06/12/2017 07:40 AM, Joe Conway wrote:
>> On 06/12/2017 01:49 AM, Amit Langote wrote:
 On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed  
 wrote:
> It looks like relation_is_updatable() didn't get the message about
> partitioned tables. Thus, for example, information_schema.views and
> information_schema.columns report that simple views built on top of
> partitioned tables are non-updatable, which is wrong. Attached is a
> patch to fix this.
>>>
>>> Thanks for the patch, Dean.
>>>
> I think this kind of omission is an easy mistake to make when adding a
> new relkind, so it might be worth having more pairs of eyes looking
> out for more of the same. I did a quick scan of the rewriter code
> (prompted by the recent similar omission for RLS on partitioned
> tables) and I didn't find any more problems there, but I haven't
> looked elsewhere yet.
>>>
>>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>>> relkind checks is interesting in this regard.
>>>
>>> On 2017/06/12 17:29, Ashutosh Bapat wrote:
 Changes look good to me. In order to avoid such instances in future, I
 have proposed to bundle the conditions as macros in [1].
>>>
>>> It seems that Ashutosh forgot to include the link:
>>>
>>> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=avcyh...@mail.gmail.com
>>
>> I have not looked at Ashutosh's patch yet, but it sounds like a good
>> idea to me.
>
> After looking I remain convinced - +1 in general.
>
> One thing I would change -- in many cases there are ERROR messages
> enumerating the relkinds. E.g.:
> 8<-
> if (!RELKIND_CAN_HAVE_COLUMN_COMMENT(relation->rd_rel->relkind))
> ereport(ERROR,
> (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> errmsg("\"%s\" is not a table, view, materialized view,
> composite type, or foreign table",
> 8<-
>
> I think those messages should also be rewritten to make them less
> relkind specific and more clear, otherwise we still risk getting out of
> sync in the future. Maybe something like this:
> 8<-
>"\"%s\" is not a kind of relation that can have column comments"
> 8<-
> Thoughts?

+1 for bringing this list to a common place. I thought about rewording
the message, but it looks like a user would be interested in the list
of relkinds rather than their connecting property. Dean also seems to
be of the same opinion.

>
> In any case, unless another committer else wants it, and assuming no
> complaints with the concept, I'd be happy to take this.
>

Thanks. I will update the patches with the error message changes and
also add some more macros by first commitfest.


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


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

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

On Mon, Jun 12, 2017 at 2:19 PM, Amit Langote
 wrote:
>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed  
>> wrote:
>>> Hi,
>>>
>>> It looks like relation_is_updatable() didn't get the message about
>>> partitioned tables. Thus, for example, information_schema.views and
>>> information_schema.columns report that simple views built on top of
>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>> patch to fix this.
>
> Thanks for the patch, Dean.
>
>>> I think this kind of omission is an easy mistake to make when adding a
>>> new relkind, so it might be worth having more pairs of eyes looking
>>> out for more of the same. I did a quick scan of the rewriter code
>>> (prompted by the recent similar omission for RLS on partitioned
>>> tables) and I didn't find any more problems there, but I haven't
>>> looked elsewhere yet.
>
> As he mentioned in his reply, Ashutosh's proposal to abstract away the
> relkind checks is interesting in this regard.
>
> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>> Changes look good to me. In order to avoid such instances in future, I
>> have proposed to bundle the conditions as macros in [1].
>
> It seems that Ashutosh forgot to include the link:
>
> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=avcyh...@mail.gmail.com

Sorry and thanks for providing the link.


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


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

Re: [HACKERS] macOS Sierra & System Integrity Protection

Robert Haas  writes:
> ... it turns out that System Integrity Protection
> feature *also* prevents  DYLD_LIBRARY_PATH from being inherited by
> child processes in some manner.

Yeah, this was already known and documented on the lists a year or two
back.  I suggest filing a bug report with Apple; if enough people bitch
about it, maybe they'll rethink.  (I don't have much hope for that,
mind you, but they certainly won't change this without a boatload of
complaints.)

https://www.postgresql.org/message-id/26098.1446697...@sss.pgh.pa.us

> My main purpose in writing this email is to pass along what I learned
> in the hopes of sparing somebody else some trouble, but perhaps there
> is a way to modify our regression test setup so that the tests can
> pass with System Integrity Protection enabled.

Not really.  If you want it to take libpq.dylib from the build tree,
rather than some already-installed location, there is no other option
but DYLD_LIBRARY_PATH.

The really annoying thing is that there's no particular security advantage
to be gained by not passing it through bash invocations.  If they're not
resetting PATH in such cases, which they aren't, where the heck is the
incremental gain from resetting DYLD_LIBRARY_PATH?  A bad guy in control
of the process environment has already won.

regards, tom lane

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

[HACKERS] macOS Sierra & System Integrity Protection

2017-06-12 Thread Robert Haas

I have a new MacBook Pro running Sierra.  I managed to get PostgreSQL
to build after install Xcode, installing MacPorts, installing the
documentation toolchain via some incantation that was apparently
wrong, and then uninstalling and reinstalling the documentation
toolchain per https://trac.macports.org/ticket/44464 - but even after
that, 'make check' was failing: 'psql' repeatedly died with an abort
trap.  Binaries worked fine when I ran them from the command line
(sometimes with DYLD_LIBRARY_PATH, if needed) but when run via
pg_regress, nothing worked.

At first, I thought that the problem was just the
/usr/lib/libpq.5.dylib which xcode had deposited, which I couldn't
move out of the way without disabling System Integrity Protection, but
eventually I got that done.  However, getting that out of the way
didn't fix anything: it turns out that System Integrity Protection
feature *also* prevents  DYLD_LIBRARY_PATH from being inherited by
child processes in some manner.  I *think* you can still set it for a
process that you invoke directly, but it can't be passed down multiple
levels. Because of that, psql, when run from the regression test
environment, can't find libpq at all (or finds the one you already
have installed rather than the one you just built), and bad stuff
happens.  So now I'm running with SIP disabled, and everything works
fine.

My main purpose in writing this email is to pass along what I learned
in the hopes of sparing somebody else some trouble, but perhaps there
is a way to modify our regression test setup so that the tests can
pass with System Integrity Protection enabled.  Disabling it requires
booting into recovery mode and then rebooting back into regular mode,
and is not recommended, so many users may find it inconvenient and
unpalatable.

...Robert


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

[HACKERS] Why are we restricting exported snapshots in subtransactions?

Hi,

ExportSnapshot() has, right at the beginning, the following block:

/*
 * We cannot export a snapshot from a subtransaction because there's no
 * easy way for importers to verify that the same subtransaction is still
 * running.
 */
if (IsSubTransaction())
ereport(ERROR,
(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
 errmsg("cannot export a snapshot from a subtransaction")));

that reasoning doesn't seem to make too much sense to me. Given that
exported snapshots don't make the exporting-transaction's changes
visible, I don't see why that restriction is needed?

As long as the exported snapshot enforces xmin to be retained, which it
does via the pairingheap, I don't understand why we'd have to enforce
that the subtransaction is still running?

I don't have any need for that capability right now, thus am not
planning to submit a patch changing this, but I'm about to apply a patch
to ExportSnapshot() to address one of the v10 open items, so I'd like to
make sure I understand the constraints.

Greetings,

Andres Freund


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

Re: [HACKERS] Race conditions with WAL sender PID lookups

On Thu, Jun 8, 2017 at 1:15 PM, Michael Paquier
 wrote:
> 3) In walreceiver.c's pg_stat_get_wal_receiver's:
> - Launch pg_stat_get_wal_receiver and take a checkpoint on it.
> - Pass the lookups of pid and ready_to_display.
> - Make the WAL receiver stop.
> - The view reports inconsistent data. If the WAL receiver data was
> just initialized, the caller would get back PID values or similar 0.
> Particularly a WAL receiver marked with !ready_to_display could show
> data to the caller. That's not cool.

This can actually leak password information to any user who is a
member of the group DEFAULT_ROLE_READ_ALL_STATS, though the window to
put hands on this password information is very small, it can be
possible if the WAL receiver gets down and restarted for a reason or
another during a maintenance window for example:
1) The user queries pg_stat_wal_receiver, for example take a
breakpoint just after the check on walrcv->ready_to_display.
2) Restart the primary once, forcing the standby to spawn a new WAL receiver.
3) Breakpoint on the WAL receiver process, with something like that to help:
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -243,6 +243,7 @@ WalReceiverMain(void)

/* Fetch information required to start streaming */
walrcv->ready_to_display = false;
+   pg_usleep(1000L); /* 10s */
strlcpy(conninfo, (char *) walrcv->conninfo, MAXCONNINFO);
strlcpy(slotname, (char *) walrcv->slotname, NAMEDATALEN);
startpoint = walrcv->receiveStart;
4) continue the session querying pg_stat_wal_receiver, at this stage
it has information with the WAL receiver data set as
!ready_to_display. If the connection string includes a password, this
becomes visible as well.

If queried at high frequency, pg_stat_wal_receiver may show up some
information. Postgres 9.6 includes this leak as well, but there is no real
leak non-superusers will just see NULL fields for this view. In Postgres
10 though, any member of this group for statistics can see leaking
information. Based on that, this is an open item, so I have added it back
now to the list, moved from the section for older bugs.
-- 
Michael


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

[HACKERS] Fix a typo in shm_mq.c

Hi,

Attached the patch for $subject.

s/Whem/When/

Regards,

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


fix_typo_in_shm_mq_c.patch
Description: Binary data

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

Re: [HACKERS] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

On 2017-06-12 21:03:31 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Possibly too hard to be precise enough in a hint, but a number of these
> > could benefit from one suggesting moving things into FROM, using
> > LATERAL.
> 
> Maybe "You might be able to move the set-returning function into a
> LATERAL FROM item."?

WFM, seems at least better than the current and current-as-proposed
state.


- Andres


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

Re: [HACKERS] pgrowlocks relkind check

On 2017/06/13 0:29, Peter Eisentraut wrote:
> On 4/24/17 21:22, Amit Langote wrote:
>> Hi Stephen,
>>
>> On 2017/04/11 22:17, Stephen Frost wrote:
 create extension pgrowlocks;
 create view one as select 1;
 select pgrowlocks('one');
 -- ERROR:  could not open file "base/68730/68748": No such file or 
 directory

 With the attached patch:

 select pgrowlocks('one');
 ERROR:  "one" is not a table, index, materialized view, sequence, or TOAST
 table
>>>
>>> Good point.
>>>
>>> Thanks, I'll see about committing this shortly.
>>
>> Should I add this to the next commitfest (not an open item per se) or will
>> you be committing it?
> 
> What is happening with this?

FWIW, patch seems simple enough to be committed into 10, unless I am
missing something.

Rebased one attached.

Thanks,
Amit
>From 570ae0006fccb9d4a2a53b93169e548050a12c07 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 11 Apr 2017 16:59:03 +0900
Subject: [PATCH] Add relkind check to pgrowlocks

---
 contrib/pgrowlocks/pgrowlocks.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index 00e2015c5c..a1ee7f8034 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -66,6 +66,8 @@ typedef struct
 #defineAtnum_modes 4
 #defineAtnum_pids  5
 
+static void check_relation_relkind(Relation rel);
+
 Datum
 pgrowlocks(PG_FUNCTION_ARGS)
 {
@@ -112,6 +114,9 @@ pgrowlocks(PG_FUNCTION_ARGS)
aclcheck_error(aclresult, ACL_KIND_CLASS,
   
RelationGetRelationName(rel));
 
+   /* Only some relkinds contain rows */
+   check_relation_relkind(rel);
+
scan = heap_beginscan(rel, GetActiveSnapshot(), 0, NULL);
mydata = palloc(sizeof(*mydata));
mydata->rel = rel;
@@ -298,3 +303,21 @@ pgrowlocks(PG_FUNCTION_ARGS)
 
SRF_RETURN_DONE(funcctx);
 }
+
+/*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+   if (rel->rd_rel->relkind != RELKIND_RELATION &&
+   rel->rd_rel->relkind != RELKIND_INDEX &&
+   rel->rd_rel->relkind != RELKIND_MATVIEW &&
+   rel->rd_rel->relkind != RELKIND_SEQUENCE &&
+   rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+   ereport(ERROR,
+   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+errmsg("\"%s\" is not a table, index, 
materialized view, sequence, or TOAST table",
+   RelationGetRelationName(rel;
+}
-- 
2.11.0


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

Re: [HACKERS] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

Andres Freund  writes:
> Possibly too hard to be precise enough in a hint, but a number of these
> could benefit from one suggesting moving things into FROM, using
> LATERAL.

Maybe "You might be able to move the set-returning function into a
LATERAL FROM item."?

> I'm kinda positively surprised at how non-invasive this turned out, I'd
> afraid there'd be a lot more verbosity to it.  I think the improved
> error messages (message & location), are quite worthwhile an their own.

Yeah, me too --- I'm pretty pleased with the result.

regards, tom lane


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

Re: [HACKERS] pg_subscription_rel entry can be updated concurrently

On Tue, Jun 13, 2017 at 9:52 AM, Masahiko Sawada  wrote:
> Hi,
>
> I often get an error "ERROR:  tuple concurrently updated" when
> changing subscription state(ALTER SUBSCRIPTION or DROP SUBSCRIPTION).
> The cause of this error is that table sync worker and apply worker can
> try to update the same tuple in pg_subscription_rel. Especially it
> often happens when we do initial copy for many tables and change it
> during executing.
>
> I think that touching the same tuple by two worker processes happens
> when aborting replication for a table or a subscription, so it would
> be the same result as when the worker ends up with an error. But I
> think since it's not an appropriate behavior we should deal with it.
> Any thoughts?

Yes, this error is normally not be something that users should see. So
there is something weird around the locking of the parent objects or
in the way the catalog is updated.
-- 
Michael


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

[HACKERS] pg_subscription_rel entry can be updated concurrently

Hi,

I often get an error "ERROR:  tuple concurrently updated" when
changing subscription state(ALTER SUBSCRIPTION or DROP SUBSCRIPTION).
The cause of this error is that table sync worker and apply worker can
try to update the same tuple in pg_subscription_rel. Especially it
often happens when we do initial copy for many tables and change it
during executing.

I think that touching the same tuple by two worker processes happens
when aborting replication for a table or a subscription, so it would
be the same result as when the worker ends up with an error. But I
think since it's not an appropriate behavior we should deal with it.
Any thoughts?

Regards,

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


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

Re: [HACKERS] ICU support on Windows

On Tue, Jun 13, 2017 at 6:47 AM, Alvaro Herrera
 wrote:
> Michael Paquier wrote:
>> I do some low level packaging on Windows (libxml2, libxslt, etc.), and
>> the compilation code usually allows you to usually use the
>> installation paths you want. At the end using only lib/ looks more
>> generic to me, and I did the same renaming as Ashutosh after unzipping
>> their files. There is already "Program Files" and "Program Files
>> (x86)" to make such distinctions.
>
> Oh my.  And then they say Microsoft has the brightest minds in the
> planet ...  (apparently they're all at Facebook nowadays actually. Go
> figure.)

:)

> Is this a problem on the packaging machine only, or does it cause
> fall-out on the machine in which the program runs?

>From Windows point of view, it depends on if you use /MD or /MT with cl.exe:
https://msdn.microsoft.com/en-us/library/2kzt1wy3(v=vs.110).aspx
/MD makes binaries dynamically link to DLLs, making binaries sensitive
to system updates. While /MT makes a binary bigger as the library
becomes embedded into it.

It is possible to see what an exe needs at runtime by using dumpbin
/imports which lists all the dependencies needed by the binaries. And
one can notice which option is used with cl.exe:
- If msvcrXXX.dll is listed (XXX is the MSVC version), then /MD was used.
- If msvcrXXXd.dll is listed, then /MDd was used.
- If nothing is listed, then /MT was used.
IMO, the places where the dll are placed is the problem of the
packager, and Postgres uses /MD so if dll are misplaced there will be
a run-time error because dependencies cannot be loaded.
-- 
Michael


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

Re: [HACKERS] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

On 2017-06-09 17:33:45 -0400, Tom Lane wrote:
> diff --git a/src/backend/catalog/information_schema.sql 
> b/src/backend/catalog/information_schema.sql
> index cbcd6cf..98bcfa0 100644
> --- a/src/backend/catalog/information_schema.sql
> +++ b/src/backend/catalog/information_schema.sql
> @@ -2936,12 +2936,14 @@ CREATE VIEW user_mapping_options AS
>  SELECT authorization_identifier,
> foreign_server_catalog,
> foreign_server_name,
> -   CAST((pg_options_to_table(um.umoptions)).option_name AS 
> sql_identifier) AS option_name,
> +   CAST(opts.option_name AS sql_identifier) AS option_name,
> CAST(CASE WHEN (umuser <> 0 AND authorization_identifier = 
> current_user)
> OR (umuser = 0 AND pg_has_role(srvowner, 'USAGE'))
> -   OR (SELECT rolsuper FROM pg_authid WHERE rolname = 
> current_user) THEN (pg_options_to_table(um.umoptions)).option_value
> +   OR (SELECT rolsuper FROM pg_authid WHERE rolname = 
> current_user)
> + THEN opts.option_value
>   ELSE NULL END AS character_data) AS option_value
> -FROM _pg_user_mappings um;
> +FROM _pg_user_mappings um,
> + pg_options_to_table(um.umoptions) opts;

This really is a lot better...


>  GRANT SELECT ON user_mapping_options TO PUBLIC;
>  
> diff --git a/src/backend/executor/functions.c b/sindex a35ba32..89aea2f 100644
> --- a/src/backend/executor/functions.c
> +++ b/src/backend/executor/functions.c
> @@ -388,6 +388,7 @@ sql_fn_post_column_ref(ParseState *pstat
>   param = ParseFuncOrColumn(pstate,
> 
> list_make1(subfield),
> 
> list_make1(param),
> +   
> pstate->p_last_srf,
> NULL,
> 
> cref->location);
>   }
> diff --git a/src/backend/parser/parse_aindex efe1c37..5241fd2 100644
> --- a/src/backend/parser/parse_agg.c
> +++ b/src/backend/parser/parse_agg.c
> @@ -705,6 +705,13 @@ check_agg_arguments_walker(Node *node,
>   }
>   /* Continue and descend into subtree */
>   }
> + /* We can throw error on sight for a set-returning function */
> + if ((IsA(node, FuncExpr) &&((FuncExpr *) node)->funcretset) ||
> + (IsA(node, OpExpr) &&((OpExpr *) node)->opretset))
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +  errmsg("aggregate function calls cannot 
> contain set-returning function calls"),
> +  parser_errposition(context->pstate, 
> exprLocation(node;

Possibly too hard to be precise enough in a hint, but a number of these
could benefit from one suggesting moving things into FROM, using
LATERAL.

I'm kinda positively surprised at how non-invasive this turned out, I'd
afraid there'd be a lot more verbosity to it.  I think the improved
error messages (message & location), are quite worthwhile an their own.

Greetings,

Andres Freund


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

Re: [HACKERS] Relpartbound, toasting and pg_class

Andres Freund  writes:
> I'm not quite sure where the aversion to adding a toast table to
> pg_class is coming from?

I'm not at all sure it would work, and would rather not introduce
risks of infinite recursion if they're not necessary.

regards, tom lane


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

Re: [HACKERS] Relpartbound, toasting and pg_class

On 2017-06-12 19:00:02 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-06-12 18:10:52 -0400, Tom Lane wrote:
> >> it'd be worthwhile checking some actual examples rather than guessing.
> 
> > It's indeed not very compact.  E.g a simple example with small types:
> 
> > CREATE TABLE partitioned(a int, b int, c timestamptz, data text) PARTITION 
> > BY RANGE (a, b, c);
> > CREATE TABLE partitioned_child1 PARTITION OF partitioned FOR VALUES FROM 
> > (1, 1, '2017-01-01') TO (1, 1, '2017-02-01');
> 
> > postgres[6961][1]=# SELECT relname, pg_column_size(relpartbound), 
> > length(relpartbound), pg_column_size(pg_class) FROM pg_class WHERE 
> > relpartbound IS NOT NULL;
> > ┌───┬┬┬┐
> > │relname│ pg_column_size │ length │ pg_column_size │
> > ├───┼┼┼┤
> > │ partitioned_child1│   1355 │   1351 │   1523 │
> > │ partitioneded_list_committers │   1130 │   8049 │   1298 │
> > └───┴┴┴┘
> 
> So, counting on my fingers, you'd need something like twenty partitioning
> columns before you hit trouble with the RANGE syntax.

Well, that's with 4/8 byte wide types.  I'd be surprised if people only
ever used those.  I'd bet quite a bit that people will start using
jsonb, postgis' geometry and such as partition types, even if it makes
most of us cringe.


> On the whole, I'm inclined to agree with Peter and Alvaro that this is
> fine, at least for the short term.  Even in the long term, I doubt we
> need toastability, just a more compact representation than an expression
> tree.  bytea storage of an array, perhaps?  Or maybe better, use anyarray
> like we do in pg_statistic, so that it prints legibly.

I'm not quite sure where the aversion to adding a toast table to
pg_class is coming from?  Why are we ok with arbitrary and hard to
understand restrictions here, and not elsewhere?

Greetings,

Andres Freund


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

Re: [HACKERS] Relpartbound, toasting and pg_class

Andres Freund  writes:
> On 2017-06-12 18:10:52 -0400, Tom Lane wrote:
>> it'd be worthwhile checking some actual examples rather than guessing.

> It's indeed not very compact.  E.g a simple example with small types:

> CREATE TABLE partitioned(a int, b int, c timestamptz, data text) PARTITION BY 
> RANGE (a, b, c);
> CREATE TABLE partitioned_child1 PARTITION OF partitioned FOR VALUES FROM (1, 
> 1, '2017-01-01') TO (1, 1, '2017-02-01');

> postgres[6961][1]=# SELECT relname, pg_column_size(relpartbound), 
> length(relpartbound), pg_column_size(pg_class) FROM pg_class WHERE 
> relpartbound IS NOT NULL;
> ┌───┬┬┬┐
> │relname│ pg_column_size │ length │ pg_column_size │
> ├───┼┼┼┤
> │ partitioned_child1│   1355 │   1351 │   1523 │
> │ partitioneded_list_committers │   1130 │   8049 │   1298 │
> └───┴┴┴┘

So, counting on my fingers, you'd need something like twenty partitioning
columns before you hit trouble with the RANGE syntax.  I'm willing to live
with that, especially since that's *before* compression.  (Your example
does not show that compression was ineffective; more likely it wasn't
tried, since the pg_class tuple was under 2K.)

The LIST case might be more of a problem, but I'm not sure.  It looks like
that eats circa 150 bytes per value in outfuncs.c format, but they're
*very* repetitive and compress really well.  I get about 16 stored bytes
per value with a long list of integer keys, so it looks like you could
approach 500 values in the LIST before hitting trouble.  Maybe a few less
with wider datatypes.

On the whole, I'm inclined to agree with Peter and Alvaro that this is
fine, at least for the short term.  Even in the long term, I doubt we
need toastability, just a more compact representation than an expression
tree.  bytea storage of an array, perhaps?  Or maybe better, use anyarray
like we do in pg_statistic, so that it prints legibly.

regards, tom lane

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

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

2017-06-12 Thread Peter Geoghegan

On Mon, Jun 12, 2017 at 3:52 AM, Alexey Kondratov
 wrote:
> I am not going to start with "speculative insertion" right now, but it would
> be very
> useful, if you give me a point, where to start. Maybe I will at least try to
> evaluate
> the complexity of the problem.

Speculative insertion has the following special entry points to
heapam.c and execIndexing.c, currently only called within
nodeModifyTable.c:

* SpeculativeInsertionLockAcquire()

* HeapTupleHeaderSetSpeculativeToken()

* heap_insert() called with HEAP_INSERT_SPECULATIVE argument

* ExecInsertIndexTuples() with specInsert = true

* heap_finish_speculative()

* heap_abort_speculative()

Offhand, it doesn't seem like it would be that hard to teach another
heap_insert() caller the same tricks.

>> My advice right now is: see if you can figure out a way of doing what
>> you want without subtransactions at all, possibly by cutting some
>> scope. For example, maybe it would be satisfactory to have the
>> implementation just ignore constraint violations, but still raise
>> errors for invalid input for types.
>
>
> Initially I was thinking only about malformed rows, e.g. less or extra
> columns.
> Honestly, I did not know that there are so many levels and ways where error
> can occur.

My sense is that it's going to be hard to sell a committer on any
design that consumes subtransactions in a way that's not fairly
obvious to the user, and doesn't have a pretty easily understood worse
case. But, that's just my opinion, and it's possible that someone else
will disagree. Try to get a second opinion.

Limiting the feature to just skip rows on the basis of a formally
defined constraint failing (not including type input failure, or a
trigger throwing an error, and probably not including foreign key
failures because they're really triggers) might be a good approach.
MySQL's INSERT IGNORE is a bit like that, I think. (It doesn't *just*
ignore duplicate violations, unlike our ON CONFLICT DO NOTHING
feature).

I haven't thought about this very carefully, but I guess you could do
something like passing a flag to ExecConstraints() that indicates
"don't throw an error; instead, just return false so I know not to
proceed". Plus maybe one or two other cases, like using speculative
insertion to back out of unique violation without consuming a subxact.

-- 
Peter Geoghegan

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

Re: [HACKERS] Relpartbound, toasting and pg_class

On 2017-06-12 18:10:52 -0400, Tom Lane wrote:
> Alvaro Herrera  writes:
> > Tom Lane wrote:
> >> How about gathering some actual evidence on the point --- ie, how big
> >> a partition expression do you need to make it fall over?
> 
> > You'd need a 2kB expression (after compression) in
> > relpartbound before you hit a problem here.  I wouldn't worry about it
> > at this stage ...
> 
> Actually, as long as the expression was less than ~8KB after compression,
> it'd work.  But I don't have a clear idea of complex an expression that
> really is --- we've never made much of an effort to make the outfuncs.c
> representation compact, so maybe there's an issue here?  As I said,
> it'd be worthwhile checking some actual examples rather than guessing.

It's indeed not very compact.  E.g a simple example with small types:

CREATE TABLE partitioned(a int, b int, c timestamptz, data text) PARTITION BY 
RANGE (a, b, c);
CREATE TABLE partitioned_child1 PARTITION OF partitioned FOR VALUES FROM (1, 1, 
'2017-01-01') TO (1, 1, '2017-02-01');

And with LIST style partitioning it'd be quite reasonable to have
significantly longer IN() lists, no?  Compression will save us to some
degree here, but it's not going super far, especially with pglz.  I
think we have some hope of compressing out some of the serialization
overhead, but not more.

postgres[6961][1]=# SELECT relname, pg_column_size(relpartbound), 
length(relpartbound), pg_column_size(pg_class) FROM pg_class WHERE relpartbound 
IS NOT NULL;
┌───┬┬┬┐
│relname│ pg_column_size │ length │ pg_column_size │
├───┼┼┼┤
│ partitioned_child1│   1355 │   1351 │   1523 │
│ partitioneded_list_committers │   1130 │   8049 │   1298 │
└───┴┴┴┘

We can see in the latter, which just is a LIST partition with every
committers name & username, that compression helps, but in the earlier
example from above it doesn't.


> > Not on point, but this conversation reminded me of
> > https://www.commandprompt.com/blog/grant_schema_usage_to_2500_users_no_can_do/
> > wherein you needed 2500 roles in an ACL column before it became a
> > problem -- and the project's stance is not to bother supporting that
> > case.
> 
> Quite on point really.  But there we knew how many entries it took to
> break it, and we also knew that good practice wouldn't hit the problem
> because you'd use groups instead of a lot of individual ACL entries.
> I don't think we're in a position yet to just dismiss this question.

Yea, I don't think those are entirely comparable.  I'm also not sure
it was actually the right decision back then.

Greetings,

Andres Freund


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

Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

Andres Freund  writes:
> On 2017-06-12 17:35:37 -0400, Tom Lane wrote:
>> I thought about trying to insert an assert in GetNewOidWithIndex to
>> ensure that no type OIDs are selected automatically during upgrade,
>> but it seemed a bit too complicated.  Might be a good idea to figure
>> it out though, unless you have an idea for removing that requirement.

> I think the only type oids assigned during pg_upgrade are the oids for
> toast type types, right?

Perhaps that was a problem once, but it isn't now; see 
binary_upgrade_set_next_toast_pg_type_oid().

In fact, we get through the pg_upgrade regression test with the attached
patch, and I really think we ought to commit it.

Having said that, I wouldn't mind trying to reduce the catalog overhead
for toast tables in v11 or beyond.

> a) Do not create a corresponding composite type for toast tables. Not
>super pretty, but I doubt it'd be a huge issue.

I suspect there are places that assume all tables have type OIDs.

> b) Use *one* composite type for all of the toast tables.  That'd not be
>entirely trivial because of pg_type.typrelid.

That might work, with some klugery.  Peter might have some insight about
this --- I'm not sure whether the CREATE TABLE OF TYPE syntax shares
a type OID across all the tables.

regards, tom lane

diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 11ee536..92d943c 100644
*** a/src/backend/catalog/catalog.c
--- b/src/backend/catalog/catalog.c
***
*** 38,43 
--- 38,44 
  #include "catalog/pg_shseclabel.h"
  #include "catalog/pg_subscription.h"
  #include "catalog/pg_tablespace.h"
+ #include "catalog/pg_type.h"
  #include "catalog/toasting.h"
  #include "miscadmin.h"
  #include "storage/fd.h"
*** GetNewOidWithIndex(Relation relation, Oi
*** 340,345 
--- 341,354 
  	ScanKeyData key;
  	bool		collides;
  
+ 	/*
+ 	 * We should never be asked to generate a new pg_type OID during
+ 	 * pg_upgrade; doing so would risk collisions with the OIDs it wants to
+ 	 * assign.  Hitting this assert means there's some path where we failed to
+ 	 * ensure that a type OID is determined by commands in the dump script.
+ 	 */
+ 	Assert(!IsBinaryUpgrade || RelationGetRelid(relation) != TypeRelationId);
+ 
  	InitDirtySnapshot(SnapshotDirty);
  
  	/* Generate new OIDs until we find one not in the table */
*** GetNewRelFileNode(Oid reltablespace, Rel
*** 391,396 
--- 400,412 
  	bool		collides;
  	BackendId	backend;
  
+ 	/*
+ 	 * If we ever get here during pg_upgrade, there's something wrong; all
+ 	 * relfilenode assignments during a binary-upgrade run should be
+ 	 * determined by commands in the dump script.
+ 	 */
+ 	Assert(!IsBinaryUpgrade);
+ 
  	switch (relpersistence)
  	{
  		case RELPERSISTENCE_TEMP:

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

Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

On 2017-06-12 17:35:37 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > In my opinion the problem of:
> 
> > On 2017-06-12 17:13:34 -0400, Tom Lane wrote:
> >> assignments of relfilenodes have to be shortcircuited by pg_upgrade
> >> override calls during a binary-restore run, or we risk filename
> >> collisions.
> 
> > should instead be solved by simply not even trying to preserve
> > relfilenodes.  We can "just" copy/link files to the the new
> > relfilenodes, there's no need to preserve them, in contrast to
> > pg_class.oid etc.  But that's obviously something for the future.
> 
> Right.  But until somebody gets around to fixing that, it's a problem.
> 
> Also, even if we fix this, we still have the issue of type OIDs residing
> on-disk in places like array headers; that results in pg_upgrade having
> to control type OID assignments as well.

Yes, but those should be largely controlled and controllable.  With the
exception of the type oids of toast tables, more on that below.

> I thought about trying to insert an assert in GetNewOidWithIndex to
> ensure that no type OIDs are selected automatically during upgrade,
> but it seemed a bit too complicated.  Might be a good idea to figure
> it out though, unless you have an idea for removing that requirement.

I think the only type oids assigned during pg_upgrade are the oids for
toast type types, right?  I can think of two decent ways to deal with
those:

a) Do not create a corresponding composite type for toast tables. Not
   super pretty, but I doubt it'd be a huge issue.
b) Use *one* composite type for all of the toast tables.  That'd not be
   entirely trivial because of pg_type.typrelid.

Both of these would have the advantage of removing some quite redundant
content from the catalogs.

I think with such a change, we'd have no issue with *additional* toast
tables being created during the run?  We already take care of toast
tables not being created, by forcing the creation in binary upgrade mode
if a toast oid is set.

> Later versions avoid that by using a CTE instead of a temp table.
> I'm not excited enough about this to back-port the patch that
> changed it, so I'm thinking of just adding the assert back to 9.5.

I'm still a bit doubtful that we know enough to consider whether the
assert is actually safe in practice, to backpatch it.  On the other
hand, it'd only affect people building with asserts...

Greetings,

Andres Freund

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

Re: [HACKERS] Relpartbound, toasting and pg_class

Alvaro Herrera  writes:
> Tom Lane wrote:
>> How about gathering some actual evidence on the point --- ie, how big
>> a partition expression do you need to make it fall over?

> You'd need a 2kB expression (after compression) in
> relpartbound before you hit a problem here.  I wouldn't worry about it
> at this stage ...

Actually, as long as the expression was less than ~8KB after compression,
it'd work.  But I don't have a clear idea of complex an expression that
really is --- we've never made much of an effort to make the outfuncs.c
representation compact, so maybe there's an issue here?  As I said,
it'd be worthwhile checking some actual examples rather than guessing.

> Not on point, but this conversation reminded me of
> https://www.commandprompt.com/blog/grant_schema_usage_to_2500_users_no_can_do/
> wherein you needed 2500 roles in an ACL column before it became a
> problem -- and the project's stance is not to bother supporting that
> case.

Quite on point really.  But there we knew how many entries it took to
break it, and we also knew that good practice wouldn't hit the problem
because you'd use groups instead of a lot of individual ACL entries.
I don't think we're in a position yet to just dismiss this question.

regards, tom lane

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

Re: [HACKERS] Relpartbound, toasting and pg_class

Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-06-12 17:10:28 -0400, Peter Eisentraut wrote:
> >> Cases where relacl became too large have been known to exist.  I'm not
> >> sure whether relpartbound can really be that large to change the
> >> scenario significantly.
> 
> > Because it's further increasing the size by something unbounded in size,
> > which'll not uncommonly be large? It makes a fair amount of sense to
> > partition by multiple columns at once (using the expression syntax).
> 
> How about gathering some actual evidence on the point --- ie, how big
> a partition expression do you need to make it fall over?

You'd need a 2kB expression (after compression) in
relpartbound before you hit a problem here.  I wouldn't worry about it
at this stage ...

Not on point, but this conversation reminded me of
https://www.commandprompt.com/blog/grant_schema_usage_to_2500_users_no_can_do/
wherein you needed 2500 roles in an ACL column before it became a
problem -- and the project's stance is not to bother supporting that
case.

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

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

Re: [HACKERS] ICU support on Windows

Michael Paquier wrote:
> On Tue, Jun 13, 2017 at 3:13 AM, Alvaro Herrera
>  wrote:
> > Ashutosh Sharma wrote:

> >> Yes, that's right, Win64 download uses lib64 path and in my case i had
> >> renamed lib64-> lib and bin64-> bin which i guess is not a right thing
> >> to do. I think, we should allow Solution.pm to detect the platform and
> >> make a decision on the library path accordingly. Attached patch does
> >> that. Please have a look let me know your thoughts on this. Thanks.
> >
> > Uh, that's pretty odd.  Is it something libicu-specific?  Because I
> > don't see any other occurrence of \lib64\ anywhere in the MSVC build
> > scripts.
> 
> I do some low level packaging on Windows (libxml2, libxslt, etc.), and
> the compilation code usually allows you to usually use the
> installation paths you want. At the end using only lib/ looks more
> generic to me, and I did the same renaming as Ashutosh after unzipping
> their files. There is already "Program Files" and "Program Files
> (x86)" to make such distinctions.

Oh my.  And then they say Microsoft has the brightest minds in the
planet ...  (apparently they're all at Facebook nowadays actually. Go
figure.)

Is this a problem on the packaging machine only, or does it cause
fall-out on the machine in which the program runs?

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


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

Re: [HACKERS] Relpartbound, toasting and pg_class

Andres Freund  writes:
> On 2017-06-12 17:10:28 -0400, Peter Eisentraut wrote:
>> Cases where relacl became too large have been known to exist.  I'm not
>> sure whether relpartbound can really be that large to change the
>> scenario significantly.

> Because it's further increasing the size by something unbounded in size,
> which'll not uncommonly be large? It makes a fair amount of sense to
> partition by multiple columns at once (using the expression syntax).

How about gathering some actual evidence on the point --- ie, how big
a partition expression do you need to make it fall over?

regards, tom lane


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

Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

Andres Freund  writes:
> In my opinion the problem of:

> On 2017-06-12 17:13:34 -0400, Tom Lane wrote:
>> assignments of relfilenodes have to be shortcircuited by pg_upgrade
>> override calls during a binary-restore run, or we risk filename
>> collisions.

> should instead be solved by simply not even trying to preserve
> relfilenodes.  We can "just" copy/link files to the the new
> relfilenodes, there's no need to preserve them, in contrast to
> pg_class.oid etc.  But that's obviously something for the future.

Right.  But until somebody gets around to fixing that, it's a problem.

Also, even if we fix this, we still have the issue of type OIDs residing
on-disk in places like array headers; that results in pg_upgrade having
to control type OID assignments as well.  I thought about trying to insert
an assert in GetNewOidWithIndex to ensure that no type OIDs are selected
automatically during upgrade, but it seemed a bit too complicated.  Might
be a good idea to figure it out though, unless you have an idea for
removing that requirement.

>> I intend to not only commit this, but back-patch it.  There's enough
>> changes in relevant code paths that logic that is fine in HEAD might
>> not be fine in back branches.

> Hm.

Just found out that 9.4 (and probably older) fail the assertion because
of the temp table created in get_rel_infos().  That's *probably* all
right because the table is *probably* gone from disk by the time we
start the actual restore run.  But we start the new postmaster only
once, with -b, so the assertion applies to the preparatory steps as
well as the restore proper.

Later versions avoid that by using a CTE instead of a temp table.
I'm not excited enough about this to back-port the patch that
changed it, so I'm thinking of just adding the assert back to 9.5.

regards, tom lane

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

Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

2017-06-12 Thread Bruce Momjian

On Sun, Jun 11, 2017 at 09:14:36PM +, Piotr Stefaniak wrote:
> I've never been too excited to improve indent and it's increasingly
> challenging for me to force myself to work on it now, after I've
> invested so much of my spare time into it. So please bear with me if
> there are any errors.

Understood.  You would think that with the number of open-source
programs written in C that there would be more interest in C formatting
tools.  Is the Postgres community the only ones with specific
requirements, or is it just that we settled on an older tool and can't
easily change?  I have reviewed the C formatting options a few times
over the years and every time the other options were worse than what we
had.

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

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

Re: [HACKERS] ICU support on Windows

On Tue, Jun 13, 2017 at 3:13 AM, Alvaro Herrera
 wrote:
> Ashutosh Sharma wrote:
>> > I noticed that this only works if you use the "Win32" download of ICU,
>> > because the "Win64" download uses "lib64" paths.  I'm not sure what the
>> > impact of this is in practice.
>>
>> Yes, that's right, Win64 download uses lib64 path and in my case i had
>> renamed lib64-> lib and bin64-> bin which i guess is not a right thing
>> to do. I think, we should allow Solution.pm to detect the platform and
>> make a decision on the library path accordingly. Attached patch does
>> that. Please have a look let me know your thoughts on this. Thanks.
>
> Uh, that's pretty odd.  Is it something libicu-specific?  Because I
> don't see any other occurrence of \lib64\ anywhere in the MSVC build
> scripts.

I do some low level packaging on Windows (libxml2, libxslt, etc.), and
the compilation code usually allows you to usually use the
installation paths you want. At the end using only lib/ looks more
generic to me, and I did the same renaming as Ashutosh after unzipping
their files. There is already "Program Files" and "Program Files
(x86)" to make such distinctions.
-- 
Michael


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

Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

On 2017-06-12 17:13:34 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-06-11 20:03:28 -0400, Tom Lane wrote:
> >> @@ -391,6 +391,13 @@ GetNewRelFileNode(Oid reltablespace, Rel
> >> bool   collides;
> >> BackendId  backend;
> >> 
> >> +  /*
> >> +   * If we ever get here during pg_upgrade, there's something wrong; all
> >> +   * relfilenode assignments during a binary-upgrade run should be
> >> +   * determined by commands in the dump script.
> >> +   */
> >> +  Assert(!IsBinaryUpgrade);
> >> +
> 
> > I'm very doubtful that a) this doesn't get hit in practice, and b) that
> > we can rely on it going forward.  At least until we change toasting to
> > not use the global oid counter.
> 
> This is not about assignments from the global OID counter; the function
> it's touching is GetNewRelFileNode() not GetNewObjectId().

Ah, that makes more sense. You'd put the backtrace() into
GetNewObjectId() your original message, that's probably why I thought
about it.


> We don't care, for the most part.  But we *do* care about relfilenode
> assignments, for precisely the reason seen in this bug.

Even there I don't think that's a sane assumption *for the future*. We
just need a slight change in the rules about when a toast table is needed
- and that stuff seriously need overhauling - and it doesn't work
anymore.

In my opinion the problem of:
> assignments of relfilenodes have to be shortcircuited by pg_upgrade
> override calls during a binary-restore run, or we risk filename
> collisions.

should instead be solved by simply not even trying to preserve
relfilenodes.  We can "just" copy/link files to the the new
relfilenodes, there's no need to preserve them, in contrast to
pg_class.oid etc.  But that's obviously something for the future.


> I intend to not only commit this, but back-patch it.  There's enough
> changes in relevant code paths that logic that is fine in HEAD might
> not be fine in back branches.

Hm.

Greetings,

Andres Freund


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

Re: [HACKERS] Relpartbound, toasting and pg_class

On 2017-06-12 17:10:28 -0400, Peter Eisentraut wrote:
> On 6/12/17 15:38, Andres Freund wrote:
> > Just noticed that pg_class now has several varlena fields:
> > #ifdef CATALOG_VARLEN   /* variable-length fields start 
> > here */
> > /* NOTE: These fields are not present in a relcache entry's rd_rel 
> > field. */
> > aclitem relacl[1];  /* access permissions */
> > textreloptions[1];  /* access-method-specific options */
> > pg_node_tree relpartbound;  /* partition bound node tree */
> > #endif
> > 
> > of those relpartbound is fairly new. And pretty much unbounded in
> > size. Aren't we going to run into issues because pg_class doesn't have a
> > toast table? It's quite reasonable to use a multi-field composite type
> > as a partition boundary...
> 
> Cases where relacl became too large have been known to exist.  I'm not
> sure whether relpartbound can really be that large to change the
> scenario significantly.

Because it's further increasing the size by something unbounded in size,
which'll not uncommonly be large? It makes a fair amount of sense to
partition by multiple columns at once (using the expression syntax).

- Andres


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

Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

Andres Freund  writes:
> On 2017-06-11 20:03:28 -0400, Tom Lane wrote:
>> @@ -391,6 +391,13 @@ GetNewRelFileNode(Oid reltablespace, Rel
>> bool collides;
>> BackendIdbackend;
>> 
>> +/*
>> + * If we ever get here during pg_upgrade, there's something wrong; all
>> + * relfilenode assignments during a binary-upgrade run should be
>> + * determined by commands in the dump script.
>> + */
>> +Assert(!IsBinaryUpgrade);
>> +

> I'm very doubtful that a) this doesn't get hit in practice, and b) that
> we can rely on it going forward.  At least until we change toasting to
> not use the global oid counter.

This is not about assignments from the global OID counter; the function
it's touching is GetNewRelFileNode() not GetNewObjectId().

GetNewObjectId() definitely does get hit during a binary-upgrade restore,
for all object types that pg_upgrade doesn't try to control the OIDs of
--- which is most.  We don't care, for the most part.  But we *do* care
about relfilenode assignments, for precisely the reason seen in this bug.
*All* assignments of relfilenodes have to be shortcircuited by pg_upgrade
override calls during a binary-restore run, or we risk filename
collisions.  So if this assert ever gets hit, we have something to fix.

I intend to not only commit this, but back-patch it.  There's enough
changes in relevant code paths that logic that is fine in HEAD might
not be fine in back branches.

regards, tom lane


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

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

Hi,

On 2017-06-12 06:16:50 +, sanyam jain wrote:
> I have created a logical decoding slot on a standby but i haven't enabled 
> Hot_standby_feedback.What are the test cases where this setup will fail?

Creating logic slots on the standby is not supported...  It'll simply
currently not work correctly, even if you circumvent the protections
(like not being able to create one on a standby).

- Andres

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

Re: [HACKERS] WIP: Data at rest encryption

2017-06-12 Thread Ants Aasma

On Mon, Jun 12, 2017 at 10:38 PM, Robert Haas  wrote:
> On Mon, Jun 13, 2016 at 11:07 AM, Peter Eisentraut
>  wrote:
>> On 6/7/16 9:56 AM, Ants Aasma wrote:
>>>
>>> Similar things can be achieved with filesystem level encryption.
>>> However this is not always optimal for various reasons. One of the
>>> better reasons is the desire for HSM based encryption in a storage
>>> area network based setup.
>>
>> Could you explain this in more detail?
>
> I don't think Ants ever responded to this point.
>
> I'm curious whether this is something that is likely to be pursued for
> PostgreSQL 11.

Yes, the plan is to pick it up again, Real Soon Now(tm). There are a
couple of loose ends for stuff that should be encrypted, but in the
current state of the patch aren't yet (from the top of my head,
logical decoding and pg_stat_statements write some files). The code
handling keys could really take better precautions as Peter pointed
out in another e-mail. And I expect there to be a bunch of polishing
work to make the APIs as good as they can be.

To answer Peter's question about HSMs, many enterprise deployments are
on top of shared storage systems. For regulatory reasons or to limit
security clearance of storage administrators, the data on shared
storage should be encrypted. Now for there to be any point to this
endeavor, the key needs to be stored somewhere else. This is where
hardware security modules come in. They are basically hardware key
storage appliances that can either output the key when requested, or
for higher security hold onto the key and perform
encryption/decryption on behalf of the user. The patch enables the
user to use a custom shell command to go and fetch the key from the
HSM, for example using the KMIP protocol. Or a motivated person could
write an extension that implements the encryption hooks to delegate
encryption/decryption of blocks to an HSM.

Fundamentally there doesn't seem to be a big benefit of implementing
the encryption at PostgreSQL level instead of the filesystem. The
patch doesn't take any real advantage from the higher level knowledge
of the system, nor do I see much possibility for it to do that. The
main benefit for us is that it's much easier to get a PostgreSQL based
solution deployed.

I'm curious if the community thinks this is a feature worth having?
Even considering that security experts would classify this kind of
encryption as a checkbox feature.

Regards,
Ants Aasma

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

Re: [HACKERS] Relpartbound, toasting and pg_class

On 6/12/17 15:38, Andres Freund wrote:
> Just noticed that pg_class now has several varlena fields:
> #ifdef CATALOG_VARLEN /* variable-length fields start here */
>   /* NOTE: These fields are not present in a relcache entry's rd_rel 
> field. */
>   aclitem relacl[1];  /* access permissions */
>   textreloptions[1];  /* access-method-specific options */
>   pg_node_tree relpartbound;  /* partition bound node tree */
> #endif
> 
> of those relpartbound is fairly new. And pretty much unbounded in
> size. Aren't we going to run into issues because pg_class doesn't have a
> toast table? It's quite reasonable to use a multi-field composite type
> as a partition boundary...

Cases where relacl became too large have been known to exist.  I'm not
sure whether relpartbound can really be that large to change the
scenario significantly.

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


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

Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

Hi,

On 2017-06-11 20:03:28 -0400, Tom Lane wrote:
> I wrote:
> > I believe I've identified the reason why skink and some other buildfarm
> > members have been failing the pg_upgrade test recently.
> > ...
> > Not sure what we want to do about it.  One idea is to make
> > ALTER SEQUENCE not so transactional when in binary-upgrade mode.
> 
> On closer inspection, the only thing that pg_upgrade needs is to be
> able to do ALTER SEQUENCE OWNED BY without a relfilenode bump.

That indeed seems appropriate.  More on the reliability of that below,
though.

> PFA two versions of a patch that fixes this problem, at least to the
> extent that it gets through check-world without triggering the Assert
> I added to GetNewRelFileNode (which HEAD doesn't).  The first one
> is a minimally-invasive hack; the second one puts the responsibility
> for deciding if a sequence rewrite is needed into init_params.  That's
> bulkier but might be useful if we ever grow additional ALTER SEQUENCE
> options that don't need a rewrite.  OTOH, I'm not very clear on what
> such options might look like, so maybe the extra flexibility is useless.
> Thoughts?

I think the flexibility isn't a bad idea, there's certainly other
options (cycle, cache, and with more work additional ones) that could be
changed without a rewrite.

> diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
> index 11ee536..43ef4cd 100644
> --- a/src/backend/catalog/catalog.c
> +++ b/src/backend/catalog/catalog.c
> @@ -391,6 +391,13 @@ GetNewRelFileNode(Oid reltablespace, Rel
>   boolcollides;
>   BackendId   backend;
>  
> + /*
> +  * If we ever get here during pg_upgrade, there's something wrong; all
> +  * relfilenode assignments during a binary-upgrade run should be
> +  * determined by commands in the dump script.
> +  */
> + Assert(!IsBinaryUpgrade);
> +

I'm very doubtful that a) this doesn't get hit in practice, and b) that
we can rely on it going forward.  At least until we change toasting to
not use the global oid counter.  A number of catalog tables have
toastable columns, and the decision when to toast will every now and
then change. Even without changing the compression algorithm, added
columns will push things across boundaries.

I'm not yet sure what the best fix for that will be, but I don't think
we should bake in the assumption that the oid counter won't be touched.

- Andres

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

Re: [HACKERS] v10beta pg_catalog diagrams

On 6/12/17 11:28, Neil Anderson wrote:
> I'm cross posting from general. I did some work to diagram the 
> relationships in pg_catalog for v10. I would like to add it to the 
> developer FAQ here 
> https://wiki.postgresql.org/wiki/Developer_FAQ#Is_there_a_diagram_of_the_system_catalogs_available.3F
>  
> but I thought I should check if people thought it appropriate and useful 
> before I do?
> 
> https://www.postgrescompare.com/2017/06/11/pg_catalog_constraints.html

Go for it.

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


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

Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

Hi,

On 2017-06-11 17:17:07 -0400, Tom Lane wrote:
> I am not very sure why it's so hard to duplicate the misbehavior; perhaps,
> in order to make the failure happen with the current regression tests,
> it's necessary for a background auto-analyze to happen and consume some
> OIDs (for pg_statistic TOAST entries) at just the wrong time.  However,
> I can definitely demonstrate that there are uncontrolled relfilenode
> assignments happening during pg_upgrade's restore run.  I stuck an
> elog() call into GetNewObjectId(), along with generation of a stack
> trace using backtrace(), and here is one example:

Yea, that's not all that surprising :/

> Judging by when we started to see buildfarm failures, I think that
> commit 3d79013b9 probably broke it, but the problem seems latent
> in the whole concept of transactional sequence information.

Hm.

> Not sure what we want to do about it.  One idea is to make
> ALTER SEQUENCE not so transactional when in binary-upgrade mode.

I think what you proposed downthread makes sense - I'd ripped out fairly
similar code in my recent changes, because it was only introduced to
address the concurrency problems the new sequence code had, and it
didn't seem to buy sufficiently much...

- Andres


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

[HACKERS] Relpartbound, toasting and pg_class

Hi,

Just noticed that pg_class now has several varlena fields:
#ifdef CATALOG_VARLEN   /* variable-length fields start here */
/* NOTE: These fields are not present in a relcache entry's rd_rel 
field. */
aclitem relacl[1];  /* access permissions */
textreloptions[1];  /* access-method-specific options */
pg_node_tree relpartbound;  /* partition bound node tree */
#endif

of those relpartbound is fairly new. And pretty much unbounded in
size. Aren't we going to run into issues because pg_class doesn't have a
toast table? It's quite reasonable to use a multi-field composite type
as a partition boundary...

Greetings,

Andres Freund


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

Re: [HACKERS] WIP: Data at rest encryption

2017-06-12 Thread Robert Haas

On Mon, Jun 13, 2016 at 11:07 AM, Peter Eisentraut
 wrote:
> On 6/7/16 9:56 AM, Ants Aasma wrote:
>>
>> Similar things can be achieved with filesystem level encryption.
>> However this is not always optimal for various reasons. One of the
>> better reasons is the desire for HSM based encryption in a storage
>> area network based setup.
>
> Could you explain this in more detail?

I don't think Ants ever responded to this point.

I'm curious whether this is something that is likely to be pursued for
PostgreSQL 11.

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


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

Re: [HACKERS] standby server crashes hard on out-of-disk-space in HEAD

On 2017-06-12 15:12:23 -0400, Robert Haas wrote:
> (On Mon, Jun 12, 2017 at 12:11 PM, Tom Lane  wrote:
> > logfile from a standby server:
> >
> > 2017-06-12 11:43:46.450 EDT [13605] LOG:  started streaming WAL from 
> > primary at 3/E600 on timeline 1
> > 2017-06-12 11:47:46.992 EDT [11261] FATAL:  could not extend file 
> > "base/47578/54806": No space left on device
> > 2017-06-12 11:47:46.992 EDT [11261] HINT:  Check free disk space.
> > 2017-06-12 11:47:46.992 EDT [11261] CONTEXT:  WAL redo at 8/EC7E0CF8 for 
> > XLOG/FPI:
> > 2017-06-12 11:47:46.992 EDT [11261] WARNING:  buffer refcount leak: [1243] 
> > (rel=base/47578/54806, blockNum=5249, flags=0x8a00, refcount=1 1)
> > TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 
> > 2523)
> > 2017-06-12 11:47:47.567 EDT [11259] LOG:  startup process (PID 11261) was 
> > terminated by signal 6: Aborted
> > 2017-06-12 11:47:47.567 EDT [11259] LOG:  terminating any other active 
> > server processes
> > 2017-06-12 11:47:47.584 EDT [11259] LOG:  database system is shut down
> >
> > The FATAL is fine, but we shouldn't have that WARNING I think, and
> > certainly not the assertion failure.

Just for clarification: It's a WARNING so we print all missed leaks,
rather than erroring/asserting at the first leak.  We've for a long
while Asserted there's not a single pin failure (in earlier releases we
asserted out at the first leak).


> Commit 4b4b680c3d6d8485155d4d4bf0a92d3a874b7a65 (Make backend local
> tracking of buffer pins memory efficient., vintage 2014) seems like a
> likely culprit here, but I haven't tested.

I'm not that sure. As written above, the Assert isn't new, and given
this hasn't been reported before, I'm a bit doubtful that it's a general
refcount tracking bug.  The FPI code has been whacked around more
heavily, so it could well be a bug in it somewhere.

- Andres


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

Re: [HACKERS] Transition tables vs ON CONFLICT

2017-06-12 Thread Thomas Munro

On Mon, Jun 12, 2017 at 2:40 PM, Thomas Munro
 wrote:
> On Fri, Jun 9, 2017 at 4:10 PM, Thomas Munro
>  wrote:
>> On Wed, Jun 7, 2017 at 10:47 AM, Peter Geoghegan  wrote:
>>> I suppose you'll need two tuplestores for the ON CONFLICT DO UPDATE
>>> case -- one for updated tuples, and the other for inserted tuples.
>>
>> [...]
>>
>> Here is a patch implementing the above.  It should be applied on top
>> of transition-tuples-from-wctes-v2.patch[2].
>
> Here's a new version of patch #3.

That accidentally removed a comment that I wanted to keep.  Here is a
better version.

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


transition-tuples-from-on-conflict-v3.patch
Description: Binary data

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

Re: [HACKERS] standby server crashes hard on out-of-disk-space in HEAD

2017-06-12 Thread Robert Haas

(On Mon, Jun 12, 2017 at 12:11 PM, Tom Lane  wrote:
> logfile from a standby server:
>
> 2017-06-12 11:43:46.450 EDT [13605] LOG:  started streaming WAL from primary 
> at 3/E600 on timeline 1
> 2017-06-12 11:47:46.992 EDT [11261] FATAL:  could not extend file 
> "base/47578/54806": No space left on device
> 2017-06-12 11:47:46.992 EDT [11261] HINT:  Check free disk space.
> 2017-06-12 11:47:46.992 EDT [11261] CONTEXT:  WAL redo at 8/EC7E0CF8 for 
> XLOG/FPI:
> 2017-06-12 11:47:46.992 EDT [11261] WARNING:  buffer refcount leak: [1243] 
> (rel=base/47578/54806, blockNum=5249, flags=0x8a00, refcount=1 1)
> TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 2523)
> 2017-06-12 11:47:47.567 EDT [11259] LOG:  startup process (PID 11261) was 
> terminated by signal 6: Aborted
> 2017-06-12 11:47:47.567 EDT [11259] LOG:  terminating any other active server 
> processes
> 2017-06-12 11:47:47.584 EDT [11259] LOG:  database system is shut down
>
> The FATAL is fine, but we shouldn't have that WARNING I think, and
> certainly not the assertion failure.

Commit 4b4b680c3d6d8485155d4d4bf0a92d3a874b7a65 (Make backend local
tracking of buffer pins memory efficient., vintage 2014) seems like a
likely culprit here, but I haven't tested.

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


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

Re: [HACKERS] logical replication: \dRp+ and "for all tables"

2017-06-12 Thread Jeff Janes

On Sat, Jun 10, 2017 at 7:42 AM, Tom Lane  wrote:

> Jeff Janes  writes:
> > On Fri, Jun 9, 2017 at 10:20 PM, Masahiko Sawada 
> > wrote:
> >> On Sat, Jun 10, 2017 at 7:29 AM, Jeff Janes 
> wrote:
> >>> That seems unfortunate.  Should the "for all tables" be included as
> >>> another column in \dRp and \dRp+, or at least as a footnote tag in
> \dRp+ ?
>
> >> +1. I was thinking the same. Attached patch adds "All Tables" column
> >> to both \dRp and \dRp+.
>
> > Looks good to me.  Attached with regression test expected output
> changes.
>
> This patch confuses me.  In the first place, I don't see the argument for
> adding the "all tables" property to \dRp output; it seems out of place
> there.

Why?  It is an important property of the publication which is single-valued
and
and easy to represent concisely.  What makes it out of place?

> In the second place, this really fails to respond to what I'd call
> the main usability problem with \dRp+, which is that the all-tables
> property is likely to lead to an unreadably bulky list of affected tables.
> What I'd say the patch ought to do is *replace* \dRp+'s list of affected
> tables with a notation like "(all tables)" when puballtables is true.
>

I'd considered that, but I find the pager does a fine job of dealing with
the bulkiness of the list. I thought it might be a good idea to not only
point out that it is all tables, but also remind people of exactly what
tables those are currently (in case it had slipped their mind that all
tables will include table from other schemas not in their search_path, for
example)

Cheers,

Jef

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

2017-06-12 Thread Joe Conway

On 06/12/2017 11:33 AM, Dean Rasheed wrote:
> On 12 June 2017 at 17:51, Joe Conway  wrote:
>> After looking I remain convinced - +1 in general.
> 
> Yes, I think this will probably help, but I worry that it will turn
> into quite a large and invasive patch, and there are a number of
> design choices to be made over the naming and precise set of macros.
> Is this really PG10 material?


I was wondering the same after responding. Possibly not.


> My initial thought, looking at the patch, is that it might be better
> to have all the macros in one file to make them easier to maintain.


Yeah, that was my thought as well.


>> sync in the future. Maybe something like this:
>> 8<-
>>"\"%s\" is not a kind of relation that can have column comments"
>> 8<-
>> Thoughts?
> 
> -1. I think the existing error messages provide useful information
> that you'd be removing. If you're worried about the error messages
> getting out of sync, then wouldn't it be better to centralise them
> along with the corresponding macros?


I guess that could work too.


> Barring objections, I'll push my original patch and work up patches
> for the other couple of issues I found.


No objections here -- we definitely need to fix those.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

2017-06-12 Thread Dean Rasheed

On 12 June 2017 at 17:51, Joe Conway  wrote:
> On 06/12/2017 07:40 AM, Joe Conway wrote:
>> On 06/12/2017 01:49 AM, Amit Langote wrote:
>>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>>> relkind checks is interesting in this regard.
>>>
>> I have not looked at Ashutosh's patch yet, but it sounds like a good
>> idea to me.
>
> After looking I remain convinced - +1 in general.
>

Yes, I think this will probably help, but I worry that it will turn
into quite a large and invasive patch, and there are a number of
design choices to be made over the naming and precise set of macros.
Is this really PG10 material?

My initial thought, looking at the patch, is that it might be better
to have all the macros in one file to make them easier to maintain.


> One thing I would change -- in many cases there are ERROR messages
> enumerating the relkinds. E.g.:
> 8<-
> if (!RELKIND_CAN_HAVE_COLUMN_COMMENT(relation->rd_rel->relkind))
> ereport(ERROR,
> (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> errmsg("\"%s\" is not a table, view, materialized view,
> composite type, or foreign table",
> 8<-
>
> I think those messages should also be rewritten to make them less
> relkind specific and more clear, otherwise we still risk getting out of
> sync in the future. Maybe something like this:
> 8<-
>"\"%s\" is not a kind of relation that can have column comments"
> 8<-
> Thoughts?
>

-1. I think the existing error messages provide useful information
that you'd be removing. If you're worried about the error messages
getting out of sync, then wouldn't it be better to centralise them
along with the corresponding macros?


> In any case, unless another committer else wants it, and assuming no
> complaints with the concept, I'd be happy to take this.
>

OK, have at it.

Barring objections, I'll push my original patch and work up patches
for the other couple of issues I found.

Regards,
Dean


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

Re: [HACKERS] ICU support on Windows

2017-06-12 Thread Ashutosh Sharma

Hi,

On Jun 12, 2017 11:43 PM, "Alvaro Herrera"  wrote:

Ashutosh Sharma wrote:
> > I noticed that this only works if you use the "Win32" download of ICU,
> > because the "Win64" download uses "lib64" paths.  I'm not sure what the
> > impact of this is in practice.
>
> Yes, that's right, Win64 download uses lib64 path and in my case i had
> renamed lib64-> lib and bin64-> bin which i guess is not a right thing
> to do. I think, we should allow Solution.pm to detect the platform and
> make a decision on the library path accordingly. Attached patch does
> that. Please have a look let me know your thoughts on this. Thanks.

Uh, that's pretty odd.  Is it something libicu-specific?  Because I
don't see any other occurrence of \lib64\ anywhere in the MSVC build
scripts.


Yes, it is specific to libicu.

Re: [HACKERS] ICU support on Windows

Ashutosh Sharma wrote:
> > I noticed that this only works if you use the "Win32" download of ICU,
> > because the "Win64" download uses "lib64" paths.  I'm not sure what the
> > impact of this is in practice.
> 
> Yes, that's right, Win64 download uses lib64 path and in my case i had
> renamed lib64-> lib and bin64-> bin which i guess is not a right thing
> to do. I think, we should allow Solution.pm to detect the platform and
> make a decision on the library path accordingly. Attached patch does
> that. Please have a look let me know your thoughts on this. Thanks.

Uh, that's pretty odd.  Is it something libicu-specific?  Because I
don't see any other occurrence of \lib64\ anywhere in the MSVC build
scripts.

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


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

Re: [HACKERS] ICU support on Windows

2017-06-12 Thread Ashutosh Sharma

Hi,

On Mon, Jun 12, 2017 at 8:39 PM, Peter Eisentraut
 wrote:
> On 6/10/17 05:40, Ashutosh Sharma wrote:
>> With the help of attached patch, we can use icu feature on Windows.
>> All we have to do is, download the ICU libraries from - [1] and add
>> the installation path for icu libraires in config.pl like,
>>
>> icu => 'E:\Users\pg\icu',
>>
>> [1]- http://site.icu-project.org/download/53
>
> Committed that.

Thanks for that.

>
> I noticed that this only works if you use the "Win32" download of ICU,
> because the "Win64" download uses "lib64" paths.  I'm not sure what the
> impact of this is in practice.

Yes, that's right, Win64 download uses lib64 path and in my case i had
renamed lib64-> lib and bin64-> bin which i guess is not a right thing
to do. I think, we should allow Solution.pm to detect the platform and
make a decision on the library path accordingly. Attached patch does
that. Please have a look let me know your thoughts on this. Thanks.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


load_platform_specific_icu_lib.patch
Description: Binary data

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

Re: [HACKERS] Allowing dash character in LTREE

2017-06-12 Thread Cyril Auburtin

interesting, but I like '.' as path separator, it's url-friendly, and easy

I used some simple `pathTEXT CHECK (path ~ '^[\w.-]+$')` but now I
regret it, because it's not as easy to index and optimize requests

2017-05-21 11:28 GMT+02:00 Manuel Kniep :

>
>
>
>
> Manuel Kniep
> Danziger Str. 116
> 10405 Berlin
> Am 20.05.2017 um 10:27 schrieb Cyril Auburtin :
>
> It could be useful to allow the `-` char in allowed LTREE label characters  
> (currently
> a-zA-Z0-9_ https://
> https://
> github.com/adjust/ltreewww.postgresql.org/docs/9.6/static/ltree.html)
>
>
> There is a patched version here
> https://github.com/adjust/ltree
>
> Which allows almost any char but uses '::' as label separator my it's an
> inspiration to do your own patch.
>
> Manuel
>
>
>

Re: [HACKERS] Transactional sequence stuff breaks pg_upgrade

Tom Lane wrote:

> On closer inspection, the only thing that pg_upgrade needs is to be
> able to do ALTER SEQUENCE OWNED BY without a relfilenode bump.  PFA
> two versions of a patch that fixes this problem, at least to the
> extent that it gets through check-world without triggering the Assert
> I added to GetNewRelFileNode (which HEAD doesn't).  The first one
> is a minimally-invasive hack; the second one puts the responsibility
> for deciding if a sequence rewrite is needed into init_params.  That's
> bulkier but might be useful if we ever grow additional ALTER SEQUENCE
> options that don't need a rewrite.  OTOH, I'm not very clear on what
> such options might look like, so maybe the extra flexibility is useless.
> Thoughts?

I vote for the second patch.  I don't have a clear idea either, but I'm
pretty sure the logical-replication people is going to be hacking on
sequences some more, yet, and this is likely to come in handy.

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


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

Re: [HACKERS] logical replication: \dRp+ and "for all tables"

On Sat, Jun 10, 2017 at 11:42 PM, Tom Lane  wrote:
> Jeff Janes  writes:
>> On Fri, Jun 9, 2017 at 10:20 PM, Masahiko Sawada 
>> wrote:
>>> On Sat, Jun 10, 2017 at 7:29 AM, Jeff Janes  wrote:
 That seems unfortunate.  Should the "for all tables" be included as
 another column in \dRp and \dRp+, or at least as a footnote tag in \dRp+ ?
>
>>> +1. I was thinking the same. Attached patch adds "All Tables" column
>>> to both \dRp and \dRp+.
>
>> Looks good to me.  Attached with regression test expected output  changes.
>
> This patch confuses me.  In the first place, I don't see the argument for
> adding the "all tables" property to \dRp output; it seems out of place
> there.

I thought since "all tables" is also a primitive and frequently
accessed information we can add it to \dRp output. But I'm not sure
it's appropriate coping. Is there a criterion for what information we
should add to the "backslash" command or the "backslash+" command?

> In the second place, this really fails to respond to what I'd call
> the main usability problem with \dRp+, which is that the all-tables
> property is likely to lead to an unreadably bulky list of affected tables.
> What I'd say the patch ought to do is *replace* \dRp+'s list of affected
> tables with a notation like "(all tables)" when puballtables is true.
>

+1 for replacing it with "(al tables)" when puballtables is true.

Regards,

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


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

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

2017-06-12 Thread Joe Conway

On 06/12/2017 07:40 AM, Joe Conway wrote:
> On 06/12/2017 01:49 AM, Amit Langote wrote:
>>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed  
>>> wrote:
 It looks like relation_is_updatable() didn't get the message about
 partitioned tables. Thus, for example, information_schema.views and
 information_schema.columns report that simple views built on top of
 partitioned tables are non-updatable, which is wrong. Attached is a
 patch to fix this.
>> 
>> Thanks for the patch, Dean.
>> 
 I think this kind of omission is an easy mistake to make when adding a
 new relkind, so it might be worth having more pairs of eyes looking
 out for more of the same. I did a quick scan of the rewriter code
 (prompted by the recent similar omission for RLS on partitioned
 tables) and I didn't find any more problems there, but I haven't
 looked elsewhere yet.
>> 
>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>> relkind checks is interesting in this regard.
>> 
>> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>>> Changes look good to me. In order to avoid such instances in future, I
>>> have proposed to bundle the conditions as macros in [1].
>> 
>> It seems that Ashutosh forgot to include the link:
>> 
>> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=avcyh...@mail.gmail.com
> 
> I have not looked at Ashutosh's patch yet, but it sounds like a good
> idea to me.

After looking I remain convinced - +1 in general.

One thing I would change -- in many cases there are ERROR messages
enumerating the relkinds. E.g.:
8<-
if (!RELKIND_CAN_HAVE_COLUMN_COMMENT(relation->rd_rel->relkind))
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table, view, materialized view,
composite type, or foreign table",
8<-

I think those messages should also be rewritten to make them less
relkind specific and more clear, otherwise we still risk getting out of
sync in the future. Maybe something like this:
8<-
   "\"%s\" is not a kind of relation that can have column comments"
8<-
Thoughts?

In any case, unless another committer else wants it, and assuming no
complaints with the concept, I'd be happy to take this.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Why restore_command is called for existing files in pg_xlog?

2017-06-12 Thread Jeff Janes

On Mon, Jun 12, 2017 at 5:25 AM, Alex Kliukin  wrote:

> On Fri, Jun 2, 2017, at 11:51 AM, Alexander Kukushkin wrote:
>
> Hello hackers,
> There is one strange and awful thing I don't understand about
> restore_command: it is always being called for every single WAL segment
> postgres wants to apply (even if such segment already exists in pg_xlog)
> until replica start streaming from the master.
>
>
> The real problem this question is related to is being unable to bring a
> former master, demoted after a crash, online, since the WAL segments
> required to get it to the consistent state were not archived while it was
> still a master, and local segments in pg_xlog are ignored when a
> restore_command is defined. The other replicas wouldn't be good candidates
> for promotion as well, as they were way behind the master (because the last
> N WAL segments were not archived and streaming replication had a few
> seconds delay).
>

I don't really understand the problem.  If the other replicas are not
candidates for promotion, than why was the master ever "demoted" in the
first place?  It should just go through normal crash recovery, not PITR
recovery, and therefore will read the files from pg_xlog just fine.

If you already promoted one of the replicas and accepted data changes into
it, and now are thinking that was not a good idea, then there is no off the
shelf automatic way to merge together the two systems.  You have do a
manual inspection of the differences.  To do that, you would start by
starting up (a copy of) the crashed master, using normal crash recovery,
not PITR.

>
> Is this a correct list for such questions, or would it be more appropriate
> to ask elsewhere (i.e. pgsql-bugs?)
>

Probably more appropriate for pgsql-general or pgsql-admin.

Cheers,

Jeff

Re: [HACKERS] Disallowing multiple queries per PQexec()

2017-06-12 Thread Daniel Verite

Tom Lane wrote:

> Bearing in mind that I'm not really for this at all...

It's a band-aid, but certainly there are cases
where a DBA confronted to a badly written website
would just want to be able to:
  ALTER USER webuser SET allow_multiple_queries TO off;

> But if an attacker is able to inject a SET command,
> he's already found a way around it.  So there's no real
> point in locking down the GUC to prevent that.

I can think of the following case, where given the SQL-injectable
   select id from users where email='$email';
an attacker would submit this string in $email:
  foo' AND set_config('allow_multiple_queries', 'on', false)='on
which opens the rest of the session for a second injection, this
time in the form of several colon-separated commands that
would do the actual damage.

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

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

[HACKERS] standby server crashes hard on out-of-disk-space in HEAD

logfile from a standby server:

2017-06-12 11:43:46.450 EDT [13605] LOG:  started streaming WAL from primary at 
3/E600 on timeline 1
2017-06-12 11:47:46.992 EDT [11261] FATAL:  could not extend file 
"base/47578/54806": No space left on device
2017-06-12 11:47:46.992 EDT [11261] HINT:  Check free disk space.
2017-06-12 11:47:46.992 EDT [11261] CONTEXT:  WAL redo at 8/EC7E0CF8 for 
XLOG/FPI: 
2017-06-12 11:47:46.992 EDT [11261] WARNING:  buffer refcount leak: [1243] 
(rel=base/47578/54806, blockNum=5249, flags=0x8a00, refcount=1 1)
TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 2523)
2017-06-12 11:47:47.567 EDT [11259] LOG:  startup process (PID 11261) was 
terminated by signal 6: Aborted
2017-06-12 11:47:47.567 EDT [11259] LOG:  terminating any other active server 
processes
2017-06-12 11:47:47.584 EDT [11259] LOG:  database system is shut down

The FATAL is fine, but we shouldn't have that WARNING I think, and
certainly not the assertion failure.

regards, tom lane


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

Re: [HACKERS] logical replication NOTICE "synchronized table states"

On 6/9/17 19:17, Peter Eisentraut wrote:
> On 6/9/17 17:06, Jeff Janes wrote:
>> When creating a subscription, I get a NOTICE "synchronized table states"
>>
>> What is this supposed to be telling the end user?  Is this just a
>> debugging message not intended to be included in the released code?
> 
> I think we can remove this message.

done

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


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

Re: [HACKERS] Broken hint bits (freeze)

2017-06-12 Thread Vladimir Borodin


> 12 июня 2017 г., в 13:19, Amit Kapila  написал(а):
> 
> On Sun, Jun 11, 2017 at 11:59 PM, Vladimir Borodin  > wrote:
>> 
>> 8 июня 2017 г., в 17:03, Amit Kapila  написал(а):
>> 
>> On Thu, Jun 8, 2017 at 6:49 PM, Dmitriy Sarafannikov
>>  wrote:
>> 
>> 
>> Why didn't rsync made the copies on master and replica same?
>> 
>> 
>> Because rsync was running with —size-only flag.
>> 
>> 
>> IIUC the situation, the new WAL and updated pg_control file has been
>> copied, but not updated data files due to which the WAL has not been
>> replayed on replicas?  If so, why the pg_control file is copied, it's
>> size shouldn't have changed?
>> 
>> 
>> Because on master pg_upgrade moves $prefix/9.5/data/global/pg_control to
>> $prefix/9.5/data/global/pg_control.old and creates new
>> $prefix/9.6/data/global/pg_control without making hardlink. When running
>> rsync from master to replica rsync sees $prefix/9.6/data/global/pg_control
>> on master and checks if it is a hardlink. Since it is not a hardlink and
>> $prefix/9.6/data/global/pg_control does not exist on replica rsync copies
>> it. For data files the logic is different since they are hardlinks,
>> corresponding files exist on replica and they are the same size.
>> 
> 
> Okay, in that case, I guess it is better to run Analyze on master
> after the upgrade is complete (including an upgrade for replicas).  If
> you are worried about the performance of read-only replicas till the
> time Analyze on the master in completed, you might want to use
> --analyze-in-stages of vaccumdb and or use (-j njobs) along with it to
> parallelize the operation.

What about the following sequence?

1. Run pg_upgrade on master,
2. Start it in single-user mode and stop (to get right wal_level in pg_control),
3. Copy pg_control somewhere,
4. Start master, run analyze and stop.
5. Put the control file from step 3 to replicas and rsync them according to the 
documentation.

And I think that step 10.f in the documentation [1] should be fixed to mention 
starting in single-user mode or with disabled autovacuum.

[1] https://www.postgresql.org/docs/devel/static/pgupgrade.html

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

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

[HACKERS] v10beta pg_catalog diagrams

2017-06-12 Thread Neil Anderson


Hi,

I'm cross posting from general. I did some work to diagram the 
relationships in pg_catalog for v10. I would like to add it to the 
developer FAQ here 
https://wiki.postgresql.org/wiki/Developer_FAQ#Is_there_a_diagram_of_the_system_catalogs_available.3F 
but I thought I should check if people thought it appropriate and useful 
before I do?


https://www.postgrescompare.com/2017/06/11/pg_catalog_constraints.html

Thanks,
Neil

--
Neil Anderson
n...@postgrescompare.com
http://www.postgrescompare.com



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

Re: [HACKERS] pgrowlocks relkind check

On 4/24/17 21:22, Amit Langote wrote:
> Hi Stephen,
> 
> On 2017/04/11 22:17, Stephen Frost wrote:
>>> create extension pgrowlocks;
>>> create view one as select 1;
>>> select pgrowlocks('one');
>>> -- ERROR:  could not open file "base/68730/68748": No such file or directory
>>>
>>> With the attached patch:
>>>
>>> select pgrowlocks('one');
>>> ERROR:  "one" is not a table, index, materialized view, sequence, or TOAST
>>> table
>>
>> Good point.
>>
>> Thanks, I'll see about committing this shortly.
> 
> Should I add this to the next commitfest (not an open item per se) or will
> you be committing it?

What is happening with this?

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


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

Re: [HACKERS] ICU support on Windows

On 6/10/17 05:40, Ashutosh Sharma wrote:
> With the help of attached patch, we can use icu feature on Windows.
> All we have to do is, download the ICU libraries from - [1] and add
> the installation path for icu libraires in config.pl like,
> 
> icu => 'E:\Users\pg\icu',
> 
> [1]- http://site.icu-project.org/download/53

Committed that.

I noticed that this only works if you use the "Win32" download of ICU,
because the "Win64" download uses "lib64" paths.  I'm not sure what the
impact of this is in practice.

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


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

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

2017-06-12 Thread Joe Conway

On 06/12/2017 01:49 AM, Amit Langote wrote:
>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed  
>> wrote:
>>> It looks like relation_is_updatable() didn't get the message about
>>> partitioned tables. Thus, for example, information_schema.views and
>>> information_schema.columns report that simple views built on top of
>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>> patch to fix this.
> 
> Thanks for the patch, Dean.
> 
>>> I think this kind of omission is an easy mistake to make when adding a
>>> new relkind, so it might be worth having more pairs of eyes looking
>>> out for more of the same. I did a quick scan of the rewriter code
>>> (prompted by the recent similar omission for RLS on partitioned
>>> tables) and I didn't find any more problems there, but I haven't
>>> looked elsewhere yet.
> 
> As he mentioned in his reply, Ashutosh's proposal to abstract away the
> relkind checks is interesting in this regard.
> 
> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>> Changes look good to me. In order to avoid such instances in future, I
>> have proposed to bundle the conditions as macros in [1].
> 
> It seems that Ashutosh forgot to include the link:
> 
> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=avcyh...@mail.gmail.com

I have not looked at Ashutosh's patch yet, but it sounds like a good
idea to me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Disallowing multiple queries per PQexec()

"Daniel Verite"  writes:
> PGC_POSTMASTER implies that it's an instance-wide setting.
> Is is intentional? I can understand that it's more secure for this not to
> be changeable in an existing session, but it's also much less usable if you
> can't set it per-database and per-user.
> Maybe it should be PGC_SUSET ?

Bearing in mind that I'm not really for this at all... why shouldn't
it be plain old USERSET?  AFAICS, the only argument for this restriction
is to make SQL injection harder.  But if an attacker is able to inject
a SET command, he's already found a way around it.  So there's no real
point in locking down the GUC to prevent that.

Also, generally speaking, GUCs should be phrased positively, ie this
should be named something more like "allow_multiple_queries" (with
opposite sense & default of course).

> + if ((strcmp(commandTagHead, "BEGIN") != 0) ||
> (strcmp(commandTagTail, "COMMIT") != 0) )
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("cannot execute multiple commands unless it is a transaction
> block")));

I haven't read the patch, but surely looking at command tags is not
an appropriate implementation of anything in this line.

regards, tom lane

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

Re: [HACKERS] Disallowing multiple queries per PQexec()

2017-06-12 Thread Daniel Verite

Surafel Temesgen wrote:

> I modified the patch as such and added to commitfest 2017-07.

A couple comments:

+   {"disallow_multiple_queries", PGC_POSTMASTER,
CLIENT_CONN_OTHER,
+   gettext_noop("Disallow multiple queries per query
string."),
+   NULL
+   },

PGC_POSTMASTER implies that it's an instance-wide setting.
Is is intentional? I can understand that it's more secure for this not to
be changeable in an existing session, but it's also much less usable if you
can't set it per-database and per-user.
Maybe it should be PGC_SUSET ?


+   if ((strcmp(commandTagHead, "BEGIN") != 0) ||
(strcmp(commandTagTail, "COMMIT") != 0) )
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
errmsg("cannot execute multiple commands unless it is a transaction
block")));

Shouldn't ROLLBACK be considered too as ending a transaction block?
Also, can it use a more specific code than ERRCODE_SYNTAX_ERROR?
It feels more like a rule violation than a syntax error.



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


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

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

2017-06-12 Thread Amit Kapila

On Mon, Jun 12, 2017 at 6:31 AM, Mithun Cy  wrote:
> Thanks, Amit,
>

+ /* Perform autoprewarm's task. */
+ if (todo_task == TASK_PREWARM_BUFFERPOOL &&
+ !state->skip_prewarm_on_restart)

Why have you removed above comment in the new patch?  I am not
pointing this because above comment is meaningful, rather changing
things in different versions of the patch without informing reviewer
can increase the time to review.  I feel you can write some better
comment here.

1.
new file mode 100644
index 000..6c35fb7
--- /dev/null
+++ b/contrib/pg_prewarm/pg_prewarm--1.1--1.2.sql
@@ -0,0 +1,14 @@
+/* contrib/pg_prewarm/pg_prewarm--1.0--1.1.sql */

In comments, the SQL file name is wrong.

2.
+ /* Define custom GUC variables. */
+ if (process_shared_preload_libraries_in_progress)
+ DefineCustomBoolVariable("pg_prewarm.autoprewarm",
+ "Enable/Disable auto-prewarm feature.",
+ NULL,
+ ,
+ true,
+ PGC_POSTMASTER,
+ 0,
+ NULL,
+ NULL,
+ NULL);
+
+ DefineCustomIntVariable("pg_prewarm.dump_interval",
+   "Sets the maximum time between two buffer pool dumps",
+ "If set to zero, timer based dumping is disabled."
+ " If set to -1, stops autoprewarm.",
+ _interval,
+ AT_PWARM_DEFAULT_DUMP_INTERVAL,
+ AT_PWARM_OFF, INT_MAX / 1000,
+ PGC_SIGHUP,
+ GUC_UNIT_S,
+ NULL,
+ NULL,
+ NULL);
+
+ EmitWarningsOnPlaceholders("pg_prewarm");
+
+ /* If not run as a preloaded library, nothing more to do. */
+ if (!process_shared_preload_libraries_in_progress)
+ return;

a. You can easily write this code such that
process_shared_preload_libraries_in_progress needs to be checked just
once.  Move the define of pg_prewarm.dump_interval at first place and
then check if (!process_shared_preload_libraries_in_progress ) return.

b. Variable name autoprewarm isn't self-explanatory, also if you have
to search the use of this variable in the code, it is difficult
because a lot of unrelated usages can pop-up.  How about naming it as
start_prewarm_worker or enable_autoprewarm or something like that?

3.
+static AutoPrewarmSharedState *state = NULL;

Again, the naming of this variable (state) is not meaningful.  How
about SharedPrewarmState or something like that?

4.
+ ereport(LOG,
+ (errmsg("autoprewarm has started")));
..
+ ereport(LOG,
+ (errmsg("autoprewarm shutting down")));

How about changing messages as "autoprewarm worker started" and
"autoprewarm worker stopped" respectively?

5.
+void
+dump_block_info_periodically(void)
+{
+ TimestampTz last_dump_time = GetCurrentTimestamp();
..
+ if (TimestampDifferenceExceeds(last_dump_time,
+   current_time,
+   (dump_interval * 1000)))
+ {
+ dump_now(true);
..

With above coding, it will not dump the very first time it tries to
dump the blocks information.  I think it is better if it dumps the
first time and then dumps after dump_interval.  I think it is not
difficult to arrange above code to do so if you also think that is
better behavior?

6.
+dump_now(bool is_bgworker)
{
..
+ if (write(fd, buf, buflen) < buflen)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not write to file \"%s\" : %m",
+ transient_dump_file_path)));
..
}

You seem to forget to close and unlink the file in above code path.
There are a lot of places in this function where you have to free
memory or close file in case of an error condition.  You can use
multiple labels to define error exit paths, something like we have
done in DecodeXLogRecord.

7.
+ for (i = 0; i < num_blocks; i++)
+ {
+ /* In case of a SIGHUP, just reload the configuration. */
+ if (got_sighup)
+ {
+ got_sighup = false;
+ ProcessConfigFile(PGC_SIGHUP);
+ }
+
+ /* Have we been asked to stop dump? */
+ if (dump_interval == AT_PWARM_OFF)
+ {
+ pfree(block_info_array);
+ CloseTransientFile(fd);
+ unlink(transient_dump_file_path);
+ return 0;
+ }
+
+ buflen = sprintf(buf, "%u,%u,%u,%u,%u\n",
+ block_info_array[i].database,
+ block_info_array[i].tablespace,
+ block_info_array[i].filenode,
+ (uint32) block_info_array[i].forknum,
+ block_info_array[i].blocknum);
+
+ if (write(fd, buf, buflen) < buflen)
+ {
+ int save_errno = errno;
+
+ CloseTransientFile(fd);
+ unlink(transient_dump_file_path);
+ errno = save_errno;
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not write to file \"%s\": %m",
+ transient_dump_file_path)));
+ }

It seems we can club the above writes into 8K sized writes instead of
one write per block information.

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

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

Re: [HACKERS] Re: Alter subscription..SET - NOTICE message is coming for table which is already removed

On Mon, Jun 12, 2017 at 9:56 PM, Peter Eisentraut
 wrote:
> On 6/11/17 21:40, Masahiko Sawada wrote:
>> Thank you for the patch. The patch fixes this issue but it takes a
>> long time to done ALTER SUBSCRIPTION SET PUBLICATION when
>> max_sync_workers_per_subscription is set high value. Because the
>> removing entry from pg_subscription_rel and launching a new table sync
>> worker select a subscription relation state in the same order, the
>> former doesn't catch up with latter.
>> For example in my environment, when I test the following step with
>> max_sync_workers_per_subscription = 15, all table sync workers were
>> launched once and then killed. How about removing the entry from
>> pg_subscription_rel in the inverse order?
>
> I have committed the patch as is.  Optimizations might be possible, but
> let's keep in mind that the use case of changing the subscription right
> after it was created is a pretty marginal case to begin with.
>

Thank you for committing the patch. Yes, I understood.

Regards,

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


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

Re: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData

On Mon, Jun 12, 2017 at 3:50 PM, amul sul  wrote:
> On Wed, May 17, 2017 at 10:22 PM, Robert Haas  wrote:
> [...]
>> I committed this with fixes for those issues, plus I renamed the macro
>> to partition_bound_accepts_nulls, which I think is more clear.
>>
> partition_bound_accepts_nulls() will alway yield true for a range
> partitioning case, because in RelationBuildPartitionDesc, we forgot to
> set boundinfo->null_index to -1.
>
> The attached patch fixes that.
>

Right now, the partition_bound_accepts_nulls() has two callers viz.
check_new_partition_bound() and get_partition_for_tuple(). Both of
those callers are calling it only in case of LIST partition. So,
having null_index uninitialized in PartitionBoundInfoData is not a
problem. But in general, we shouldn't leave a field uninitialized in
that structure, so +1 for the patch.

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

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

Re: [HACKERS] Re: Alter subscription..SET - NOTICE message is coming for table which is already removed

On 6/11/17 21:40, Masahiko Sawada wrote:
> Thank you for the patch. The patch fixes this issue but it takes a
> long time to done ALTER SUBSCRIPTION SET PUBLICATION when
> max_sync_workers_per_subscription is set high value. Because the
> removing entry from pg_subscription_rel and launching a new table sync
> worker select a subscription relation state in the same order, the
> former doesn't catch up with latter.
> For example in my environment, when I test the following step with
> max_sync_workers_per_subscription = 15, all table sync workers were
> launched once and then killed. How about removing the entry from
> pg_subscription_rel in the inverse order?

I have committed the patch as is.  Optimizations might be possible, but
let's keep in mind that the use case of changing the subscription right
after it was created is a pretty marginal case to begin with.

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


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

Re: [HACKERS] Why restore_command is called for existing files in pg_xlog?

2017-06-12 Thread Alex Kliukin

On Fri, Jun 2, 2017, at 11:51 AM, Alexander Kukushkin wrote:
> Hello hackers,
> There is one strange and awful thing I don't understand about
> restore_command: it is always being called for every single WAL
> segment postgres wants to apply (even if such segment already exists
> in pg_xlog) until replica start streaming from the master.
The real problem this question is related to is being unable to bring a
former master, demoted after a crash, online, since the WAL segments
required to get it to the consistent state were not archived while it
was still a master, and local segments in pg_xlog are ignored when a
restore_command is defined. The other replicas wouldn't be good
candidates for promotion as well, as they were way behind the master
(because the last N WAL segments were not archived and streaming
replication had a few seconds delay).
Is this a correct list for such questions, or would it be more
appropriate to ask elsewhere (i.e. pgsql-bugs?)
> 
> If there is no restore_command in the recovery.conf - it perfectly
> works, i.e. postgres replays existing wal segments and at some point
> connects to the master and start streaming from it.> 
> When recovery_conf is there, starting of a replica could become a real
> problem, especially if restore_command is slow.> 
> Is it possible to change this behavior somehow? First look into
> pg_xlog and only if file is missing or "corrupted" call
> restore_command.> 
> 
> Regards,
> ---
> Alexander Kukushkin

Sincerely,
Alex

Re: [HACKERS] oidin / oidout and InvalidOid

2017-06-12 Thread Andreas Karlsson


On 06/12/2017 01:41 PM, Chapman Flack wrote:

I was recently guilty of writing a less-than-clear SQL example
because I plain forgot whether InvalidOid was 0 (correct) or -1
(my bad).

Would there be any sense in letting oidin_subr accept the string
InvalidOid for 0? I understand that changing oidout could break
existing code outside of the tree. But what if oidout were to be
conservative in what it does, and oidin liberal in what it accepts?


I am not sure I am a fan of this, but if we should have an alias for 
InvalidOid how about reusing '-' which is used by the reg*in functions? 
Or is that too non-obvious?


Andreas


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

[HACKERS] Dropping partitioned table drops a previously detached partition

Hi,
If we detach a partition and drop the corresponding partitioned table,
it drops the once-partition now-standalone table as well.

create table prt1 (a int, b int) partition by range(a);
create table prt1_p1 partition of prt1 for values from (0) to (100);
select oid, relname, relpartbound, relispartition, relkind from
pg_class where relname like 'prt%';
  oid  | relname
---+-
 16453 | prt1
 16456 | prt1_p1
(2 rows)

select * from pg_depend where refobjid = 'prt1'::regclass and objid =
'prt1_p1'::regclass;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
-+---+--++--+-+-
1259 | 16456 |0 |   1259 |16453 |   0 | a
(1 row)

alter table prt1 detach partition prt1_p1;

-- so the dependency exists even after detach
select * from pg_depend where refobjid = 'prt1'::regclass and objid =
'prt1_p1'::regclass;
 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
-+---+--++--+-+-
1259 | 16456 |0 |   1259 |16453 |   0 | a
(1 row)

drop table prt1;

-- Oops, we deleted the once-partition now-standalone table as well
select oid, relname, relpartbound, relispartition, relkind from
pg_class where relname like 'prt%';
 oid | relname | relpartbound | relispartition | relkind
-+-+--++-
(0 rows)

The reason for this is as follows
An AUTO dependency is recorded between a partitioned table and partition. In
case of inheritance we record a NORMAL dependency between parent
and child. While
detaching a partition, we call RemoveInheritance(), which should have taken
care of removing this dependency. But it removes the dependencies which are
marked as NORMAL. When we changed the dependency for partitioned case from
NORMAL to AUTO by updating StoreCatalogInheritance1(), this function was not
updated. This caused the dependency to linger behind even after
detaching the
partition, thus causing the now standalone table (which was once a
partition)
to be dropped when the parent partitioned table is dropped. This patch fixes
RemoveInheritance() to choose appropriate dependency.

Attached patch 0001 to fix this.

I see a similar issue-in-baking in case we change the type of
dependency recorded between a table and the composite type associated
with using CREATE TABLE ... OF command. 0002 patch addresses the
potential issue by using a macro while creating and dropping the
dependency in corresponding functions. There might be more such
places, but I haven't searched for those.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
From 97a647c487761782340d7ebca82b6d3200d79142 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat 
Date: Mon, 12 Jun 2017 16:32:53 +0530
Subject: [PATCH 1/2] Dependency between partitioned table and partition.

An AUTO dependency is recorded between a partitioned table and partition. In
case of inheritance we record a NORMAL dependency between parent and child. While
detaching a partition, we call RemoveInheritance(), which should have taken
care of removing this dependency. But it removes the dependencies which are
marked as NORMAL. When we changed the dependency for partitioned case from
NORMAL to AUTO by updating StoreCatalogInheritance1(), this function was not
updated. This caused the dependency to linger behind even after detaching the
partition, thus causing the now standalone table (which was once a partition)
to be dropped when the parent partitioned table is dropped. This patch fixes
RemoveInheritance() to choose appropriate dependency.

Ashutosh Bapat.
---
 src/backend/commands/tablecmds.c |   35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b61fda9..9aef67b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -283,6 +283,14 @@ struct DropRelationCallbackState
 #define		ATT_COMPOSITE_TYPE		0x0010
 #define		ATT_FOREIGN_TABLE		0x0020
 
+/*
+ * Partition tables are expected to be dropped when the parent partitioned
+ * table gets dropped. Hence for partitioning we use AUTO dependency.
+ * Otherwise, for regular inheritance use NORMAL dependency.
+ */
+#define child_dependency_type(child_is_partition)	\
+	((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
+
 static void truncate_check_rel(Relation rel);
 static List *MergeAttributes(List *schema, List *supers, char relpersistence,
 bool is_partition, List **supOids, List **supconstr,
@@ -439,7 +447,8 @@ static void ATExecEnableDisableRule(Relation rel, char *rulename,
 static void ATPrepAddInherit(Relation child_rel);
 static ObjectAddress ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE

[HACKERS] oidin / oidout and InvalidOid

2017-06-12 Thread Chapman Flack

Hi,

I was recently guilty of writing a less-than-clear SQL example
because I plain forgot whether InvalidOid was 0 (correct) or -1
(my bad).

Would there be any sense in letting oidin_subr accept the string
InvalidOid for 0? I understand that changing oidout could break
existing code outside of the tree. But what if oidout were to be
conservative in what it does, and oidin liberal in what it accepts?

-Chap


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

Re: [HACKERS] Statement-level rollback

2017-06-12 Thread Simon Riggs

On 1 March 2017 at 16:05, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 2/28/17 08:17, Tom Lane wrote:
>>> I do not really see how this would ever get past the compatibility
>>> problems that forced us to give up on server-side autocommit years ago.
>
>> I think it's different because it's not a global setting, it's only a
>> behavior you select explicitly when you start a transaction block.
>
> Yeah, that's the same it-won't-affect-you-if-you-don't-use-it argument
> that we heard for server-side autocommit-off.

This is a frequently requested feature and I think we should push
ahead with it in the next cycle.

We're the World's Most Advanced Open Source Database, so a new
transaction feature fits in with our goals.

> I don't buy it.
> I can think of two reasons even without any caffeine:
>
> 1. The argument for this is mostly, if not entirely, "application
> compatibility".  But it won't succeed at providing that if every
> BEGIN has to be spelled differently than it would be on other DBMSes.
> Therefore there is going to be enormous pressure to allow enabling
> the feature through a GUC, or some other environment-level way,
> and as soon as we do that we've lost.

We already use GUCs for various other transaction level features and
they work just fine.

What we need is a feature that works the way other DBMS do, as an
option. If it should be desirable.

I do accept there are problems and we do have some experience of those problems.

> 2. The proposed feature would affect the internal operation of PL
> functions, so that those would need to become bulletproof against
> being invoked in either operating environment.  Likewise, all sorts
> of intermediate tools like connection poolers would no doubt be broken
> if they don't know about this and support both modes.  (We would have
> to start by fixing postgres_fdw and dblink, for instance.)
>
> In short, you can't make fundamental changes in transactional behavior
> without enormous breakage.  That was the lesson we learned from the
> autocommit fiasco and I do not believe that it's inapplicable here.

I think the point we should take from Tom's comments is...

a) This feature won't be a replacement for PostgreSQL's default
behaviour, at least not in any short/medium term.

b) If we get this feature, about 80% of the work will be fixing all
the small breakages that happen with other tools, plugins etc.. So it
is no small task. If accepted this would be a major feature and will
take much work.

If we want this in Postgres11 then we must have a fully working patch
by start of Sept 2017, plus some analysis of all of the various
breakage points we are expecting to see. So lets do the analysis, so
we know how deep the mud is before we decide to walk through it.

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

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

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

2017-06-12 Thread Alexey Kondratov

Thank you for your comments Peter, there are some points that I did not think about before.On 9 Jun 2017, at 01:09, Peter Geoghegan wrote:Adding a full support of ON CONFLICT DO NOTHING/UPDATE to COPY seemsto be a large separated task and is out of the current project scope, butmaybe there isa relatively simple way to somehow perform internally tuples insert withON CONFLICT DO NOTHING? I have added Peter Geoghegan to cc, asI understand he is the major contributor of UPSERT in PostgreSQL. It wouldbe greatif he will answer this question.I think that there is a way of making COPY use "speculativeinsertion", so that it behaves the same as ON CONFLICT DO NOTHING withno inference specification. Whether or not this is useful depends on alot of things.I am not going to start with "speculative insertion" right now, but it would be very useful, if you give me a point, where to start. Maybe I will at least try to evaluate the complexity of the problem.I think that you need to more formally identify what errors your newCOPY error handling will need to swallowMy advice right now is: see if you can figure out a way of doing whatyou want without subtransactions at all, possibly by cutting somescope. For example, maybe it would be satisfactory to have theimplementation just ignore constraint violations, but still raiseerrors for invalid input for types. Initially I was thinking only about malformed rows, e.g. less or extra columns. Honestly, I did not know that there are so many levels and ways where error can occur. So currently (and especially after your comments) I prefer to focus only on the following list of errors:1) File format issues a. Less columns than needed b. Extra columns2) I am doubt about type mismatch. It is possible to imagine a situation when, e.g. some integers are exported as int, and some as "int", but I am not sure that is is a common situation.3) Some constraint violations, e.g. unique index.First appeared to be easy achievable without subtransactions. I have created a proof of concept version of copy, where the errors handling is turned on by default. Please, see small patch attached (applicable to 76b11e8a43eca4612dfccfe7f3ebd293fb8a46ec) or GUI version on GitHub https://github.com/ololobus/postgres/pull/1/files. It throws warnings instead of errors for malformed lines with less/extra columns and reports line number.Second is probably achievable without subtransactions via the PG_TRY/PG_CATCH around heap_form_tuple, since it is not yet inserted into the heap.But third is questionable without subtransactions, since even if we check constraints once, there maybe various before/after triggers which can modify tuple, so it will not satisfy them. Corresponding comment inside copy.c states: "Note that a BR trigger might modify tuple such that the partition constraint is no satisfied, so we need to check in that case." Thus, there are maybe different situations here, as I understand. However, it a point where "speculative insertion"is able to help.These three cases should cover most real-life scenarios.Is there really much value in ignoring errors due to invalid encoding?Now, I have some doubts about it too. If there is an encoding problem, it is probably about the whole file, not only a few rows.Alexey

copy-errors-v0.1.patch
Description: Binary data

Re: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData

2017-06-12 Thread amul sul

On Wed, May 17, 2017 at 10:22 PM, Robert Haas  wrote:
[...]
> I committed this with fixes for those issues, plus I renamed the macro
> to partition_bound_accepts_nulls, which I think is more clear.
>
partition_bound_accepts_nulls() will alway yield true for a range
partitioning case, because in RelationBuildPartitionDesc, we forgot to
set boundinfo->null_index to -1.

The attached patch fixes that.


Regards,
Amul


set_boundinfo-null_index.patch
Description: Binary data

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

Re: [HACKERS] Broken hint bits (freeze)

2017-06-12 Thread Amit Kapila

On Sun, Jun 11, 2017 at 11:59 PM, Vladimir Borodin  wrote:
>
> 8 июня 2017 г., в 17:03, Amit Kapila  написал(а):
>
> On Thu, Jun 8, 2017 at 6:49 PM, Dmitriy Sarafannikov
>  wrote:
>
>
> Why didn't rsync made the copies on master and replica same?
>
>
> Because rsync was running with —size-only flag.
>
>
> IIUC the situation, the new WAL and updated pg_control file has been
> copied, but not updated data files due to which the WAL has not been
> replayed on replicas?  If so, why the pg_control file is copied, it's
> size shouldn't have changed?
>
>
> Because on master pg_upgrade moves $prefix/9.5/data/global/pg_control to
> $prefix/9.5/data/global/pg_control.old and creates new
> $prefix/9.6/data/global/pg_control without making hardlink. When running
> rsync from master to replica rsync sees $prefix/9.6/data/global/pg_control
> on master and checks if it is a hardlink. Since it is not a hardlink and
> $prefix/9.6/data/global/pg_control does not exist on replica rsync copies
> it. For data files the logic is different since they are hardlinks,
> corresponding files exist on replica and they are the same size.
>

Okay, in that case, I guess it is better to run Analyze on master
after the upgrade is complete (including an upgrade for replicas).  If
you are worried about the performance of read-only replicas till the
time Analyze on the master in completed, you might want to use
--analyze-in-stages of vaccumdb and or use (-j njobs) along with it to
parallelize the operation.


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


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

Re: [HACKERS] Statement-level rollback

2017-06-12 Thread Ideriha, Takeshi

Hello,

This feature hasn't been updated for a long time, 
but I've just been interested in this feature and looking into the mailing list.

From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
> In short, you can't make fundamental changes in transactional behavior without
> enormous breakage.  That was the lesson we learned from the autocommit fiasco
> and I do not believe that it's inapplicable here.

I've just wanted to confirm what "autocommit fiasco" points out.
Are the below threads and git-log relevant discussion?
(If there are any other threads, could you please tell me the link?)

https://www.postgresql.org/message-id/flat/3E54526A.121EBEE5%40tpf.co.jp#3e54526a.121eb...@tpf.co.jp
 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f85f43dfb5b9043ea6b01d8b824c195cd7f9ed3c

Regards,
Takeshi Ideriha



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

Re: [HACKERS] ICU support on Windows

2017-06-12 Thread Ashutosh Sharma

Hi,

On Mon, Jun 12, 2017 at 12:18 PM, Michael Paquier
 wrote:
> On Sat, Jun 10, 2017 at 6:40 PM, Ashutosh Sharma  
> wrote:
>> Currently, we cannot perform ICU enabled build for postgres on Windows
>> platform. However, this can be done on Linux platforms using
>> '--with-icu' configuration parameter. Attached is the patch that
>> allows us to perform icu enabled build for postgres on Windows
>> platform provided that we have correct version of ICU libraries
>> installed on our system.
>>
>> With the help of attached patch, we can use icu feature on Windows.
>> All we have to do is, download the ICU libraries from - [1] and add
>> the installation path for icu libraires in config.pl like,
>
> I have looked at this patch and it looks good to me. Windows is able
> to compile with ICU support using it, which is a good step forward
> into moving on with things in this area.
>
> I have added an open item, it seems to me that it is a mistake to not
> be able to have that support as well on Windows in PG10.

Thanks for reviewing the patch and adding it to the PostgreSQL 10 open
items list.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com


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

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed  
> wrote:
>> Hi,
>>
>> It looks like relation_is_updatable() didn't get the message about
>> partitioned tables. Thus, for example, information_schema.views and
>> information_schema.columns report that simple views built on top of
>> partitioned tables are non-updatable, which is wrong. Attached is a
>> patch to fix this.

Thanks for the patch, Dean.

>> I think this kind of omission is an easy mistake to make when adding a
>> new relkind, so it might be worth having more pairs of eyes looking
>> out for more of the same. I did a quick scan of the rewriter code
>> (prompted by the recent similar omission for RLS on partitioned
>> tables) and I didn't find any more problems there, but I haven't
>> looked elsewhere yet.

As he mentioned in his reply, Ashutosh's proposal to abstract away the
relkind checks is interesting in this regard.

On 2017/06/12 17:29, Ashutosh Bapat wrote:
> Changes look good to me. In order to avoid such instances in future, I
> have proposed to bundle the conditions as macros in [1].

It seems that Ashutosh forgot to include the link:

https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=avcyh...@mail.gmail.com

Thanks,
Amit



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

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed  wrote:
> Hi,
>
> It looks like relation_is_updatable() didn't get the message about
> partitioned tables. Thus, for example, information_schema.views and
> information_schema.columns report that simple views built on top of
> partitioned tables are non-updatable, which is wrong. Attached is a
> patch to fix this.
>
> I think this kind of omission is an easy mistake to make when adding a
> new relkind, so it might be worth having more pairs of eyes looking
> out for more of the same. I did a quick scan of the rewriter code
> (prompted by the recent similar omission for RLS on partitioned
> tables) and I didn't find any more problems there, but I haven't
> looked elsewhere yet.
>

Changes look good to me. In order to avoid such instances in future, I
have proposed to bundle the conditions as macros in [1].

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


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

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/09 20:49, Ashutosh Bapat wrote:
> May be we should pass a flag to predicate_implied_by() to handle NULL
> behaviour for CHECK constraints. Partitioning has shown that it needs
> to use predicate_implied_by() for comparing constraints and there may
> be other cases that can come up in future. Instead of handling it
> outside predicate_implied_by() we may want to change it under a flag.

IMHO, it may not be a good idea to modify predtest.c to suit the
partitioning code's needs.  The workaround of checking that NOT NULL
constraints on partitioning columns exist seems to me to be simpler than
hacking predtest.c to teach it about the new behavior.

Thanks,
Amit

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

Re: [HACKERS] A bug in mapping attributes in ATExecAttachPartition()

On 2017/06/09 20:47, Ashutosh Bapat wrote:
> Thanks for the long explanation. I guess, this should be written in
> comments somewhere in the code there. I see following comment in
> ATExecAttachPartition()
> --
>  *
>  * There is a case in which we cannot rely on just the result of the
>  * proof.
>  */
> 
> --
> 
> I guess, this comment should be expanded to explain what you wrote above.

Tried in the attached updated patch.

Thanks,
Amit
From a98ea14dd3aa57f06d7066ee996c54f42a3014ac Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 8 Jun 2017 14:01:34 +0900
Subject: [PATCH] Fixes around partition constraint handling when attaching

Failure to map attribute numbers in the partition key expressions to
to partition's would cause predicate_implied_by() to unnecessarily
return 'false', in turn causing the failure to skip the validation
scan.

Conversely, failure to map partition's NOT NULL column's attribute
numbers to parent's might cause incorrect conclusion about which
columns of the partition being attached must have NOT NULL constraint
defined on them.

Rearrange code and comments a little around this area to make things
clearer.
---
 src/backend/commands/tablecmds.c  | 122 +++---
 src/test/regress/expected/alter_table.out |  32 
 src/test/regress/sql/alter_table.sql  |  31 
 3 files changed, 126 insertions(+), 59 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b61fda9909..13fbed9431 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13409,6 +13409,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
List   *childrels;
TupleConstr *attachRel_constr;
List   *partConstraint,
+  *partConstraintOrig,
   *existConstraint;
SysScanDesc scan;
ScanKeyData skey;
@@ -13574,14 +13575,37 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
partConstraint = list_make1(make_ands_explicit(partConstraint));
 
/*
+* Adjust the generated constraint to match this partition's attribute
+* numbers.  Save the original to be used later if we decide to proceed
+* with the validation scan after all.
+*/
+   partConstraintOrig = copyObject(partConstraint);
+   partConstraint = map_partition_varattnos(partConstraint, 1, attachRel,
+   
 rel);
+
+   /*
 * Check if we can do away with having to scan the table being attached 
to
 * validate the partition constraint, by *proving* that the existing
 * constraints of the table *imply* the partition predicate.  We include
-* the table's check constraints and NOT NULL constraints in the list of
-* clauses passed to predicate_implied_by().
-*
-* There is a case in which we cannot rely on just the result of the
-* proof.
+* only the table's check constraints in the list of "restrictions" 
passed
+* to predicate_implied_by() and do not include the NullTest expressions
+* corresponding to any NOT NULL constraints that the table might have,
+* because we do not want to depend on predicate_implied_by's proof for
+* the requirement (if any) that there not be any null values in the
+* partition keys of the existing rows.  As written in the comments at
+* the top of predicate_implied_by_simple_clause(), the restriction 
clause
+* that it receives is assumed to be a WHERE restriction wherein NULL
+* result means 'false' so that any rows that it selects must have 
non-null
+* value in the respective column (an implicit NOT NULL constraint).  
But
+* in this case, we are passing the list of restrictions that are 
table's
+* constraints wherein NULL result means 'true', so even rows with null
+* values in the respective columns would have been admitted into the
+* table, whereas, per aforementioned, predicate_implied_by() proof 
would
+* say there cannot be any null values in those columns based on those
+* restrictions.  The only way to confirm NOT NULLness then is by 
looking
+* for explicit NOT NULL constraints on the partition key columns.  If 
any
+* partition key is an expression, for which there cannot be a NOT NULL
+* constraint, we simply give up on skipping the scan.
 */
attachRel_constr = tupleDesc->constr;
existConstraint = NIL;
@@ -13589,42 +13613,36 @@ ATExecAttachPartition(List **wqueue, Relation rel, 
PartitionCmd *cmd)
{
int num_check = attachRel_constr->num_check;
int i;
+   AttrNumber *parent_attnos;

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

On Mon, Jun 12, 2017 at 3:16 PM, sanyam jain  wrote:
> I have created a logical decoding slot on a standby but i haven't enabled
> Hot_standby_feedback.What are the test cases where this setup will fail?

hot_standby_feedback needs to be enabled at all times in logical
decoding so as the node does not remove rows that are still needed for
the decoding, and a VACUUM passing by with a minimal xmin too high
would cause inconsistent decoded data.
-- 
Michael


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

Re: [HACKERS] ICU support on Windows