date:20170803

Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization

2017-08-03 Thread Masahiko Sawada

On Thu, Aug 3, 2017 at 2:00 AM, Robert Haas  wrote:
> On Wed, Aug 2, 2017 at 12:34 PM, Tom Lane  wrote:
>> Of course.  It's also a heck of a lot more flexible.  Adding on another
>> ad-hoc option that does the minimum possible amount of work needed to
>> address one specific problem is always going to be less work; but after
>> we repeat that process five or ten times, we're going to have a mess.
>
> Well, I still like Masahiko-san's proposal, but I'm not prepared to
> keep arguing about it right now.  Maybe some other people will weigh
> in with an opinion.
>

My motivation of this proposal is same as what Robert has. I
understand that ad-hoc option can solve only the part of big problem
and it could be cause of mess. However It seems me that the script
especially for table initialization will not be flexible than we
expected. I mean, even if we provide some meta commands for table
initialization or data loading, these meta commands work for only
pgbench tables (i.g., pgbench_accounts, pgbench_branches and so on).
If we want to create other tables and load data to them as we want we
can do that using psql -f. So an alternative ways is having a flexible
style option for example --custom-initialize = { [load, create_pkey,
create_fkey, vacuum], ... }. That would solve this in a better way.

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] pg_stop_backup(wait_for_archive := true) on standby server

2017-08-03 Thread Michael Paquier

On Thu, Aug 3, 2017 at 4:29 AM, Stephen Frost  wrote:
> I'll provide another update tomorrow.  Hopefully Michael is able to produce
> a 9.6 patch, otherwise I'll do it.

I have sent an updated version of the patch, with something that can
be used for 9.6 as well. It would be nice to get something into the
next set of minor releases.
-- 
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] Confusing error message in pgbench

2017-08-03 Thread Fabien COELHO




Indeed.  It doesn't look that hard: AFAICS the problem is just that
process_sql_command() is making premature decisions about whether to
extract parameters from the SQL commands.  Proposed patch attached.


Great. Patch looks good to me.


Too me as well: code looks ok, patch applies, compiles, make check 
ok, manual tests with pgbench ok.


That is one more patch about pgbench in the queue.

--
Fabien.


--
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] map_partition_varattnos() and whole-row vars

2017-08-03 Thread Amit Langote

Fujita-san,

Thanks for the review.

On 2017/08/03 16:01, Etsuro Fujita wrote:
> On 2017/08/02 15:21, Amit Langote wrote:
>> On 2017/08/02 1:33, Amit Khandekar wrote:
>>> ---
>>>
>>> Few more comments :
>>>
>>> @@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,
>>>   var->varlevelsup == context->sublevels_up)
>>>   {
>>>   /* Found a matching variable, make the substitution */
>>>
>>> - Var*newvar = (Var *) palloc(sizeof(Var));
>>> + Var*newvar = copyObject(var);
>>>int attno = var->varattno;
>>>
>>>   *newvar = *var;
>>>
>>> Here, "*newvar = *var" should be removed.
>>
>> Done.
> 
> I'm not sure this change is a good idea, because the copy by "*newvar =
> *var" would be more efficient than the copyObject().  (We have this
> optimization in other places as well.  See eg, copyVar() in setrefs.c.)

OK, done.

> Here are some other comments:
> 
> +/* If the callers expects us to convert the same, do so. */
> +if (OidIsValid(context->to_rowtype))
> +{
> +ConvertRowtypeExpr *r;
> +
> +/* Var itself is converted to the requested rowtype. */
> +newvar->vartype = context->to_rowtype;
> +
> +/*
> + * And a conversion step on top to convert back to the
> + * original type.
> + */
> +r = makeNode(ConvertRowtypeExpr);
> +r->arg = (Expr *) newvar;
> +r->resulttype = var->vartype;
> +r->convertformat = COERCE_IMPLICIT_CAST;
> +r->location = -1;
> +
> +return (Node *) r;
> +}
> 
> Why not do this conversion if to_rowtype is valid and it's different from
> the rowtype of the original whole-row Var like the previous patch? Also, I
> think it's better to add an assertion that the rowtype of the original
> whole-row Var is a named one.  So, what I have in mind is:
> 
>   if (OidIsValid(context->to_rowtype))
>   {
> Assert(var->vartype != RECORDOID)
> if (var->vartype != context->to_rowtype)
> {
>   ConvertRowtypeExpr *r;
> 
>   /* Var itself is converted to the requested rowtype. */
>   ...
>   /* And a conversion step on top to convert back to the ... */
>   ...
>   return (Node *) r;
> }
>   }

Sounds good, so done.

> Here is the modification to the map_variable_attnos()'s API:
> 
>  map_variable_attnos(Node *node,
> int target_varno, int sublevels_up,
> const AttrNumber *attno_map, int
> map_length,
> -   bool *found_whole_row)
> +   bool *found_whole_row, Oid
> to_rowtype)
> 
> This is nitpicking, but I think it would be better to put the new argument
> to_rowtype right before the output argument found_whole_row.

I consider this a good suggestion.  I guess we tend to list all the input
arguments before any output arguments.  So done as you suggest.

> + * RelationGetRelType
> + *Returns the rel's pg_type OID.
> + */
> +#define RelationGetRelType(relation) \
> +((relation)->rd_rel->reltype)
> 
> This macro is used in only one place.  Do we really need that?  (This
> macro would be useful for other places such as expand_inherited_rtentry,
> but I think it's better to introduce this in a separate patch, maybe for
> PG11.)

Alright, dropped RelationGetRelType from the patch.

> +-- check that wholerow vars in the RETUNING list work with partitioned
> tables
> 
> Typo: s/RETUNING/RETURNING/

Fixed.

Attached updated patches.

Thanks,
Amit
From 9b2d16ec4c8eadd7261849d5aa0f34ee2577b405 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 26 Jul 2017 16:45:46 +0900
Subject: [PATCH 1/2] Fix map_partition_varattnos to not error on
 found_whole_row

It was designed assuming that the expressions passed to it can never
contain whole-row vars, but it's wrong since it's called from places
that pass it WCO constraint expressions and RETURNING target list
expressions, which can very well contain whole-row vars.

Move the responsibility of error'ing out to the callers, because they
are in position to know that finding whole-row vars is an error
condition.

Adds test in insert.sql and updatable_views.sql.

Reported by: Rajkumar Raghuwanshi
Report: 
https://postgr.es/m/CAKcux6%3Dz38gH4K6YAFi%2BYvo5tHTwBL4tam4VM33CAPZ5dDMk1Q%40mail.gmail.com
---
 src/backend/catalog/partition.c   | 20 ++--
 src/backend/commands/tablecmds.c  |  8 +++-
 src/backend/executor/nodeModifyTable.c| 18 ++
 src/include/catalog/partition.h   |  3 ++-
 src/test/regress/expected/insert.out  | 10 ++
 src/test/regress/expected/updatable_views.out | 10 ++

Re: [HACKERS] map_partition_varattnos() and whole-row vars

2017-08-03 Thread Etsuro Fujita


On 2017/08/02 15:21, Amit Langote wrote:

Thanks Fuita-san and Amit for reviewing.

On 2017/08/02 1:33, Amit Khandekar wrote:

On 1 August 2017 at 15:11, Etsuro Fujita  wrote:

On 2017/07/31 18:56, Amit Langote wrote:

Yes, that's what's needed here.  So we need to teach
map_variable_attnos_mutator() to convert whole-row vars just like it's
done in adjust_appendrel_attrs_mutator().



Seems reasonable.  (Though I think it might be better to do this kind of
conversion in the planner, not the executor, because that would increase the
efficiency of cached plans.)


That's a good point, although it sounds like a bigger project that, IMHO,
should be undertaken separately, because that would involve designing for
considerations of expanding inheritance even in the INSERT case.


Agreed.  I think that would be definitely a material for PG11.


I think the work of shifting to planner should be taken as a different
task when we shift the preparation of DispatchInfo to the planner.


Yeah, I think it'd be a good idea to do those projects together.  That is,
doing what Fujita-san suggested and expanding partitioned tables in
partition bound order in the planner.


OK


---

Few more comments :

@@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,
  var->varlevelsup == context->sublevels_up)
  {
  /* Found a matching variable, make the substitution */

- Var*newvar = (Var *) palloc(sizeof(Var));
+ Var*newvar = copyObject(var);
   int attno = var->varattno;

  *newvar = *var;

Here, "*newvar = *var" should be removed.


Done.


I'm not sure this change is a good idea, because the copy by "*newvar = 
*var" would be more efficient than the copyObject().  (We have this 
optimization in other places as well.  See eg, copyVar() in setrefs.c.)


Here are some other comments:

+   /* If the callers expects us to convert the 
same, do so. */
+   if (OidIsValid(context->to_rowtype))
+   {
+   ConvertRowtypeExpr *r;
+
+   /* Var itself is converted to the 
requested rowtype. */
+   newvar->vartype = context->to_rowtype;
+
+   /*
+* And a conversion step on top to 
convert back to the
+* original type.
+*/
+   r = makeNode(ConvertRowtypeExpr);
+   r->arg = (Expr *) newvar;
+   r->resulttype = var->vartype;
+   r->convertformat = COERCE_IMPLICIT_CAST;
+   r->location = -1;
+
+   return (Node *) r;
+   }

Why not do this conversion if to_rowtype is valid and it's different 
from the rowtype of the original whole-row Var like the previous patch? 
Also, I think it's better to add an assertion that the rowtype of the 
original whole-row Var is a named one.  So, what I have in mind is:


  if (OidIsValid(context->to_rowtype))
  {
Assert(var->vartype != RECORDOID)
if (var->vartype != context->to_rowtype)
{
  ConvertRowtypeExpr *r;

  /* Var itself is converted to the requested rowtype. */
  ...
  /* And a conversion step on top to convert back to the ... */
  ...
  return (Node *) r;
}
  }

Here is the modification to the map_variable_attnos()'s API:

 map_variable_attnos(Node *node,
int target_varno, int sublevels_up,
const AttrNumber *attno_map, 
int map_length,

-   bool *found_whole_row)
+   bool *found_whole_row, Oid 
to_rowtype)


This is nitpicking, but I think it would be better to put the new 
argument to_rowtype right before the output argument found_whole_row.


+ * RelationGetRelType
+ * Returns the rel's pg_type OID.
+ */
+#define RelationGetRelType(relation) \
+   ((relation)->rd_rel->reltype)

This macro is used in only one place.  Do we really need that?  (This 
macro would be useful for other places such as expand_inherited_rtentry, 
but I think it's better to introduce this in a separate patch, maybe for 
PG11.)


+-- check that wholerow vars in the RETUNING list work with partitioned 
tables


Typo: s/RETUNING/RETURNING/

Sorry for the delay.

Best regards,
Etsuro Fujita



--
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_stop_backup(wait_for_archive := true) on standby server

2017-08-03 Thread Michael Paquier

On Wed, Aug 2, 2017 at 7:58 PM, Stephen Frost  wrote:
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> Do you need a back-patchable version for 9.6? I could get one out of
>> my pocket if necessary.
>
> I was just trying to find a bit of time to generate exactly that- if
> you have a couple spare cycles, it would certainly help.

OK, here you go. Even if archive_mode = always has been introduced in
9.5, but non-exclusive mode is a 9.6 feature, so here are patches down
to this version. I am pretty satisfied by this, and I included all the
comments and error message corrections reviewed up to now. I noticed
some incorrect comments, doc typos and an incorrect indentation as
well for the WARNING reported to the user when waiting for the
archiving.
-- 
Michael


pg_stop_backup_on_standby_v6_96.patch
Description: Binary data


pg_stop_backup_on_standby_v6_master.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] Red-Black tree traversal tests

2017-08-03 Thread Aleksander Alekseev

Hi Victor,

> I forgot to attach the patch. Sorry.
> Here it is.

I would say that this patch is in a pretty good shape now. And I see a
99% code coverage of rbtree.c. Let's see what committers think.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] map_partition_varattnos() and whole-row vars

2017-08-03 Thread Etsuro Fujita


On 2017/08/03 17:12, Amit Langote wrote:

Attached updated patches.


Thanks for the patch!  That looks good to me.

Best regards,
Etsuro Fujita



--
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] INSERT ON CONFLICT and partitioned tables

2017-08-03 Thread Jeevan Ladhe

Thanks Amit for addressing the comment.

The patch looks good to me. I have no more comments.
Verified that v2 patch applies cleanly and make check passes.

Thanks,
Jeevan Ladhe

Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization

2017-08-03 Thread Fabien COELHO



Hello,


My motivation of this proposal is same as what Robert has. I
understand that ad-hoc option can solve only the part of big problem
and it could be cause of mess. However It seems me that the script
especially for table initialization will not be flexible than we
expected. I mean, even if we provide some meta commands for table
initialization or data loading, these meta commands work for only
pgbench tables (i.g., pgbench_accounts, pgbench_branches and so on).
If we want to create other tables and load data to them as we want we
can do that using psql -f. So an alternative ways is having a flexible
style option for example --custom-initialize = { [load, create_pkey,
create_fkey, vacuum], ... }. That would solve this in a better way.


Personnaly, I could be fine with a limited number of long options to 
adjust pgbench initialization to various needs, eg --use-hash-index, 
--skip-whetever-index, etc.


The flexible --custom-init idea outlined above looks nice as well.


As for a more generic solution, the easy part are the "CREATE" stuff and 
the transaction script stuff (existing pgbench scripts).


For the CREATE stuff, the script language is SQL, the command to use it is 
"psql"...


The real and hard part is to fill tables with meaningful pseudo-random 
test data which do not violate constraints for any non trivial schema 
involving foreign keys and various unique constraints.


The solution for this is SQL for trivial cases, think of:

  "INSERT INTO Foo() SELECT ... FROM generate_series(...);"

For instance the pgbench initialization is really close to:

 psql -Dscale=10

Re: [HACKERS] Change in "policy" on dump ordering?

2017-08-03 Thread Michael Banck

Am Donnerstag, den 27.07.2017, 15:52 -0400 schrieb Tom Lane:
> So I'm thinking we should consider the multi-pass patch I posted as
> a short-term, backpatchable workaround, which we could hope to
> replace with dependency logic later.

+1, it would be really nice if this could be fixed in the back-branches 
before v11.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer


-- 
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] Support for Secure Transport SSL library on macOS as OpenSSL alternative

2017-08-03 Thread Heikki Linnakangas

On 08/03/2017 01:02 PM, Daniel Gustafsson wrote:

In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I
presented a WIP patch for adding support for the Apple Secure Transport SSL
library on macOS as, an alternative to OpenSSL. That patch got put on the
backburner for a bit, but I’ve now found the time to make enough progress to
warrant a new submission for discussions on this (and hopefully help hacking).

Hooray!

Keychains
=
The frontend has support for using PEM files for certificates and keys. It can
also look up the key for the certificate in a Keychain, or both certificate and
key in a Keychain. The root certificate is still just read from a PEM file.

Why can't you have the root certificate in the keychain, too? Just not
implemented yet, or is there some fundamental problem with it?

The existence of an sslcert config trumps a keychain, but when a keychain is
used I’m currently using the sslcert parameter (awaiting a discussion on how to
properly do this) for the certificate label required to search the keychain.

There is a new frontend parameter called “keychain” with which a path to a
custom Keychain file can be passed. If set, this Keychain will be searched as
well as the default. If not, only the default user Keychain is used. There is
nothing that modifies the Keychain in this patch, it can read identities
(certificates and its key) but not alter them in any way.

OpenSSL also has a mechanism somewhat similar to the keychain, called
"engines". You can e.g. keep the private key corresponding a certificate
on a smart card, and speak to it with an OpenSSL "smart card reader"
engine. If you do that, the 'sslkey' syntax is ":name>". Perhaps we should adopt that syntax here as well? For example,
to read the client certificate from the key chain, you would use libpq
options like "keychain=/home/heikki/my_keychain
sslcert=keychain:my_client_cert".

“keychain” is obviously a very Secure Transport specific name, and I personally
think we should avoid that. Any new configuration added here should take
future potential implementation into consideration such that avoid the need for
lots of backend specific knobs. “sslcertstore” comes to mind as an
alternative, but we’d also need parameters to point into the certstore for
finding what we need. Another option would be to do a URL based scheme
perhaps.

I wouldn't actually mind using implementation-specific terms like
"keychain" here. It makes it clear that it's implementation-specific. I
think it would be confusing, to use the same generic option name, like
"sslcertstore", for both a macOS keychain and e.g. the private key store
in Windows. Or GNU Keyring. In the worst case, you might even have
multiple such "key stores" on the same system, so you'd anyways need a
way to specify which one you mean.

Actually, perhaps it should be made even more explicit, and call it
"secure_transport_keychain". That's quite long, though.

Wrt. keychains, is there a system-global or per-user keychain in macOS?
And does this patch use them? If you load a CRL into a global keychain,
does it take effect?

Testing
===
In order to test this we need to provide an alternative to the openssl calls in
src/test/ssl/Makefile for Secure Transport.

Those openssl commands are only needed to re-generate the test
certificates. The certificates are included in the git repository, so
you only need to re-generate them if you want to modify them or add new
ones. I think it's OK to require the openssl tool for that, it won't be
needed just to run the test suite.

Documentation
=
I have started fiddling with this a little, but to avoid spending time on the
wrong thing I have done very little awaiting the outcome of discussions here.
I have tried to add lots of comments to the code however, to explain the quirks
of Secure Transport.

I think this patch in general is in very good shape, and the next step
is to write the documentation. In particular, I'd like to see
documentation on how the keychain stuff should work. It'll be easier to
discuss the behavior and the interactions, once it's documented.

In fact, better to write the documentation for that now, and not bother
to change the code, until after we've discussed and are happy with the
documented behavior.

I went into this thinking I would write a README for how to implement a new SSL
library. But in the end, turns out the refactoring that went into our SSL code
earlier made that part almost too easy to warrant that. It’s really quite
straightforward.

That's nice to hear!

- Heikki

--
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] Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)

2017-08-03 Thread Michael Paquier

On Mon, Jul 31, 2017 at 6:13 PM, Robert Haas  wrote:
> On Fri, Jul 28, 2017 at 10:35 AM, Andreas Joseph Krogh
>  wrote:
>> I'm reading https://www.postgresql.org/docs/10/static/pgupgrade.html to try
>> to understand how to upgrade standby-servers using pg_upgrade with pg10.
>>
>> The text in step 10 sais:
>> "You will not be running pg_upgrade on the standby servers, but rather
>> rsync", which to me sounds like rsync, in step 10-f, should be issued on the
>> standy servers. Is this the case? If so I don't understand how the standby's
>> data is upgraded and what "remote_dir" is. If rsync is supposed to be issued
>> on the primary then I think it should be explicitly mentioned, and step 10-f
>> should provide a clarer example with more detailed values for the
>> directory-structures involved.
>>
>> I really think section 10 needs improvement as I'm certainly not comfortable
>> upgrading standbys following the existing procedure.
>
> Yeah, I don't understand it either, and I have never been convinced
> that there's any safe way to do it other than recloning the standbys
> from the upgraded master.

Here are my 2c on the matter. 10-f means that the upgraded node may
have generated WAL with wal_level = minimal, which, at least it seems
to me, that we have a risk of having inconsistent data pages if only a
rsync is used on the old standbys. Like Robert, the flow we used in
the products I work on is to re-create standbys from scratch after the
upgrade using a fresh backup, with a VM cloning. An upgrade here is an
in-place process not only linked to Postgres, so standby VMs are made
of many services, some are being linked to Postgres. So this choice is
mainly decided by those dependencies, still it feels safer anyway.
-- 
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] Update comments in nodeModifyTable.c

2017-08-03 Thread Etsuro Fujita


On 2017/08/02 4:07, Robert Haas wrote:

On Tue, Aug 1, 2017 at 12:31 AM, Etsuro Fujita
 wrote:

Maybe I'm missing something, but I'm not sure that's a good idea because the
change says like we might have 'wholerow' only for the FDW case, but that
isn't correct because we would have 'wholerow' for a view as well. ISTM that
views should be one of the typical cases, so I'd like to propose to modify
the sentence starting with 'Typically' to something like this: "Typically,
this will be a 'ctid' or 'wholerow' attribute, but in the case of a foreign
data wrapper it might be a set of junk attributes sufficient to identify the
remote row."  What do you think about that?


Works for me.


I updated the patch that way.  Attached is a new version of the patch.

Best regards,
Etsuro Fujita
diff --git a/src/backend/executor/nodeModifyTable.c 
b/src/backend/executor/nodeModifyTable.c
index 0dde0ed..0199c9d 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1696,7 +1696,7 @@ ExecModifyTable(PlanState *pstate)
 * the old relation tuple.
 *
 * Foreign table updates have a wholerow 
attribute when the
-* relation has an AFTER ROW trigger.  Note 
that the wholerow
+* relation has a row-level trigger.  Note that 
the wholerow
 * attribute does not carry system columns.  
Foreign table
 * triggers miss seeing those, except that we 
know enough here
 * to set t_tableOid.  Quite separately from 
this, the FDW may
@@ -2182,8 +2182,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
/*
 * Initialize the junk filter(s) if needed.  INSERT queries need a 
filter
 * if there are any junk attrs in the tlist.  UPDATE and DELETE always
-* need a filter, since there's always a junk 'ctid' or 'wholerow'
-* attribute present --- no need to look first.
+* need a filter, since there's always at least one junk attribute 
present
+* --- no need to look first.  Typically, this will be a 'ctid' or
+* 'wholerow' attribute, but in the case of a foreign data wrapper it
+* might be a set of junk attributes sufficient to identify the remote
+* row.
 *
 * If there are multiple result relations, each one needs its own junk
 * filter.  Note multiple rels are only possible for UPDATE/DELETE, so 
we
@@ -2251,7 +2254,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
else if (relkind == 
RELKIND_FOREIGN_TABLE)
{
/*
-* When there is an AFTER 
trigger, there should be a
+* When there is a row-level 
trigger, there should be a
 * wholerow attribute.
 */
j->jf_junkAttNo = 
ExecFindJunkAttribute(j, "wholerow");

-- 
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] Transactions involving multiple postgres foreign servers

2017-08-03 Thread Michael Paquier

On Mon, Jul 31, 2017 at 7:27 PM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>
>> An alternative approach is to have some kind of other identifier,
>> let's call it a distributed transaction ID (DXID) which is mapped by
>> each node onto a local XID.
>
> Postgres-XL seems to manage this problem by using a transaction manager
> node, which is in charge of assigning snapshots.  I don't know how that
> works, but perhaps adding that concept here could be useful too.  One
> critical point to that design is that the app connects not directly to
> the underlying Postgres server but instead to some other node which is
> or connects to the node that manages the snapshots.
>
> Maybe Michael can explain in better detail how it works, and/or how (and
> if) it could be applied here.

XL (and XC) use a transaction ID that plugs in directly with the
internal XID assigned by Postgres, actually bypassing what Postgres
assigns to each backend if a transaction needs one. So if transactions
are not heavenly shared among multiple nodes, performance gets
impacted. Now when we worked on this project we noticed that we gained
in performance by reducing the number of requests and grouping them
together, so a proxy layer has been added between the global
transaction manager and Postgres to group those requests. This does
not change the fact that read-committed transactions still need
snapshots for each query, which is consuming. So this approach hurts
less with analytic queries, and more with OLTP.

2PC transaction status was tracked as well in the GTM. This allows
fancy things like being able to prepare a transaction on node 1, and
commit it on node 2 for example. I am not honestly sure that you need
to add anything at clog level for example, but I think that having at
the FDW level the meta data of a transaction stored as a rather
correct approach on the matter. That's what greenplum actually does if
I recall correctly (Heikki save me!): it has one coordinator with such
metadata handling, and bunch of underlying nodes that store the data.
Citus does also that if I recall correctly. So instead of
decentralizing this information, this gets stored in a Postgres
coordinator instance.
-- 
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] reload-through-the-top-parent switch the partition table

2017-08-03 Thread Ashutosh Bapat

On Wed, Aug 2, 2017 at 11:47 PM, David G. Johnston
 wrote:
> On Wed, Aug 2, 2017 at 10:58 AM, Tom Lane  wrote:
>>
>> Robert Haas  writes:
>> > On Wed, Aug 2, 2017 at 1:08 PM, Tom Lane  wrote:
>> >> --restore-via-partition-root ?
>>
>> > I worry someone will think that pg_dump is now restoring stuff, but it
>> > isn't.
>>
>> Well, the point is that the commands it emits will cause the eventual
>> restore to go through the root.  Anyway, I think trying to avoid using
>> a verb altogether is going to result in a very stilted option name.
>>
>> I notice that the option list already includes some references to
>> "insert", so maybe "--insert-via-partition-root"?  Although you could
>> argue that that's confusing when we're using COPY.
>
>
> --use-partitioned-table [partition-name, ...]  # if names are omitted it
> defaults to all partitioned tables

I like this idea since it allows using this feature for selected
tables e.g. hash tables. Otherwise, users will be forced to use this
option even when there is only  a single hash partitioned table and
many other list/range partitioned tables.

What we are trying to do here is dump the data in a partitioned table
as if it's not partitioned. Combine that with --data-only dumps, and
one could use it to load partitioned data into unpartitioned or
differently partitioned table. So, how about naming the option as

--unpartition-partitioned-table [partitioned-table-name, ] # if
names are omitted it defaults to all the partitioned tables.

That really says what dump is really doing without focusing on how the
data will be used like restoring/inserting/copying etc.

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

[HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative

2017-08-03 Thread Daniel Gustafsson

In https://postgr.es/m/69db7657-3f9d-4d30-8a4b-e06034251...@yesql.se I
presented a WIP patch for adding support for the Apple Secure Transport SSL
library on macOS as, an alternative to OpenSSL.  That patch got put on the
backburner for a bit, but I’ve now found the time to make enough progress to
warrant a new submission for discussions on this (and hopefully help hacking).

It is a drop-in replacement for the OpenSSL code, and supports all the same
features and options, except for two things: compression is not supported and
the CRL cannot be loaded from a plain PEM file.  A Keychain must be used for
that instead.


Current state
=
The frontend and backend are implemented more or less fully, the two main
things missing being the CRL support (further details below) and loading DH
files (to support the GUC added in c0a15e07cd).  All non-CRL tests but one are
passing.  The failing test is in the frontend and it also fails when running
against an OpenSSL backend, but I can’t find where the logic is flawed and
could do with some help there.


Threads
===
Just like the CFLocaleCopyCurrent() call referenced in postmaster.c, the Secure
Transport APIs makes the process multithreaded.  To keep this out of the
postmaster, and contained in the backend, I’ve moved all functionality to
open_server and left init empty.  I could definitely need some clues on how to
properly handle this, or if it’s a complete showstopper.


Keychains
=
The frontend has support for using PEM files for certificates and keys.  It can
also look up the key for the certificate in a Keychain, or both certificate and
key in a Keychain.  The root certificate is still just read from a PEM file.
The existence of an sslcert config trumps a keychain, but when a keychain is
used I’m currently using the sslcert parameter (awaiting a discussion on how to
properly do this) for the certificate label required to search the keychain.

There is a new frontend parameter called “keychain” with which a path to a
custom Keychain file can be passed.  If set, this Keychain will be searched as
well as the default.  If not, only the default user Keychain is used.  There is
nothing that modifies the Keychain in this patch, it can read identities
(certificates and its key) but not alter them in any way.

The backend is only supporting PEM files at this point.

Once we have support for Keychains, we can however use them for additionally
supporting other Secure Transport features like OCSP etc.

“keychain” is obviously a very Secure Transport specific name, and I personally
think we should avoid that.  Any new configuration added here should take
future potential implementation into consideration such that avoid the need for
lots of backend specific knobs.  “sslcertstore” comes to mind as an
alternative, but we’d also need parameters to point into the certstore for
finding what we need.  Another option would be to do a URL based scheme
perhaps.


Certificate Revocation
==
Secure Transport supports loading CRL lists into Keychain files, the command
line certtool can for example do that.  When doing the trust evaluation on the
connection, a policy can be added which enables revocation checking via CRL.  I
have however been unable to figure out how to load a CRL list programmatically,
and as far as I can tell there is no API for this.  The certtool utility is
using the low-level CSSM APIs for this it seems, but adding that level of
complexity doesn’t seem worth it for us to maintain (I did a test and it turned
big, ugly and messy).

Unless someone knows how to implement this, we’d be limited to requiring the
use of a Keychain file for CRL support.  This would break drop-in replacement
support, but supporting certificate revocation seems more important.


Platform Support

I’ve tested this on 10.11 El Capitan and 10.12 Sierra, which are the systems I
have access to.  Supporting prairiedog and dromedary in the buildfarm will
probably be too hard (if at all possible), but down to 10.9 Mavericks should be
quite possible (if someone has the required systems to test on).  It adds a
dependency on Core Foundation on top of Secure Transport, no other macOS APIs
are used.


Testing
===
In order to test this we need to provide an alternative to the openssl calls in
src/test/ssl/Makefile for Secure Transport.  On top of that, code to generate
Keychains is required.  The certtool application can do all the Keychain
generations (I think) but this is still left open.  The main thing to figure
out here is how to avoid having to type in the Keychain password in a modal GUI
that Secure Transport pops up.  Since a Keychain can be passwordless it should
be doable, but the right certtool incantations for that is still evading me.
I’ve included a show-and-tell patch for this which I’ve used locally for
testing during hacking.


Documentation
=
I have started fiddling with this a little, but to avoid spending time on

Re: [HACKERS] Macros bundling RELKIND_* conditions

2017-08-03 Thread Joe Conway

On 08/02/2017 10:52 PM, Ashutosh Bapat wrote:
> On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
>  wrote:
>> Alvaro Herrera wrote:
>>> I think pg_class is a reasonable place to put more generic relkind lists
>>> alongside a matching error message for each, rather than specialized
>>> "does this relkind have storage" macros.  What about something like a
>>> struct list in pg_class.h,
>>
>> I just noticed that this doesn't help at all with the initial problem
>> statement, which is that some of the relkind checks failed to notice
>> that partitioned tables needed to be added to the set.  Maybe it still
>> helps because you have something to grep for, as Tom proposed elsewhere.
> 
> Having something like relkind_i_t_T, though correct, doesn't make the
> test readable. That's another improvement because of using such
> macros. The macro name tells the purpose of the test, which is what a
> reader would be interested in, rather than the actual values used.

+1

Joe

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



signature.asc
Description: OpenPGP digital signature

90 matches

Mail list logo