Re: [HACKERS] IPv6 link-local addresses and init data type

[Haribabu Kommi]

On Fri, Nov 4, 2016 at 6:33 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 6/7/16 2:43 PM, Peter Eisentraut wrote:
> > On 6/7/16 1:19 AM, Haribabu Kommi wrote:
> >> How about the following case, Do we treat them as same or different?
> >>
> >> select 'fe80::%eth1'::inet = 'fe80::%ETH1'::inet;
> >>
> >> fe80::%2/64 is only treated as the valid address but not other way as
> >> fe80::/64%2.
> >> Do we need to throw an error in this case or just ignore.
> >
> > I suspect questions like these are already solved in someone else's IP
> > address type/object/class implementation, and we could look there.
>
> I have been doing some testing and reviewed the RFCs again.  I'm having
> some second thoughts about this.
>
> I tried the IP address parsing modules in Perl, Python, Ruby, and they
> all reject zone IDs.
>
> The Java class java.net.Inet6Address accepts zone IDs, but only numbers
> and names actually present on the local host.
>

Thanks for the review and analyzing other modules behavior.


> RFC 4007 specifies that the zone IDs "should" not be used for global
> addresses or the loopback address.  The presented patch does not check
> for that.
>

I will add the check.


>
> The original message in this thread mentioned an address "::1%0", so
> that is not even valid.  0 is supposed to be the default zone, so one
> could argue that that can be silently accepted.
>

Yes, I agree that default zone is the main use case of the original thread.
>From the RFC 4007, the default zone is used for the global addresses,
This may be the main use case with zone id. How about currently just
ignoring it and store the actual IP address with the attached patch and
handle the rest of the actual zone id support later once the it gets
properly standardized?


> A zone ID is only meaningful inside a node.  So storing it in a table
> without an associated node is meaningless.  (compare: storing a time
> with time zone without a date)
>
> There are also questions about comparing addresses with zone IDs, such
> as in the example at the very top.  We don't have a good answer on
> whether those addresses should be equal.  (My OS seems to think that
> interface names are case-insensitive.)  Also, since there is no node
> associated with the address, it's questionable what equal really means
> anyway.
>
> So I'm having doubts that the proposed change to allow any string to be
> appended to any address is really sound.  This could invite a lot of
> shenanigans, where equality comparisons or range operations are
> subverted, for example.


Currently there is lack of information to decide for the above problems from
the RFC, may be lets wait for the standardization to be clear and then
support zone id by adding minimal support for default zone?

Regards,
Hari Babu
Fujitsu Australia


0001-INET-ipv6-default-zone-support.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] Remove the comment on the countereffectiveness of large shared_buffers on Windows

[amul sul]

On Mon, Nov 7, 2016 at 10:46 AM, Tsunakawa, Takayuki
 wrote:
>
> The third paragraph may be redundant, I'm a bit inclined to leave it for 
> kindness and completeness.  The attached revised patch just correct the 
> existing typo (large -> larger).
>

I am not agree to having this paragraph either, I'll leave the
decision to committer.

> I'll change the status to needs review.

The new status of this patch is: Ready for Committer

Regards,
Amul Sul


-- 
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 the comment on the countereffectiveness of large shared_buffers on Windows

[Tsunakawa, Takayuki]

From: amul sul [mailto:sula...@gmail.com]
> IMHO, I think we could remove third paragraph completely and generalised
> starting of second paragraph, somewhat looks likes as
> follow:
> 
> 
> -If you have a dedicated database server with 1GB or more of RAM,
> a
> -reasonable starting value for shared_buffers
> is 25%
> -of the memory in your system.  There are some workloads where even
> +A reasonable starting value for
> shared_buffers is 25%
> +   of the RAM in your system.  There are some workloads where even
>  large settings for shared_buffers are
> effective, but
>  because PostgreSQL also relies on 
the
>  operating system cache, it is unlikely that an allocation of more
> than

The third paragraph may be redundant, I'm a bit inclined to leave it for 
kindness and completeness.  The attached revised patch just correct the 
existing typo (large -> larger).

I'll change the status to needs review.

Regards
Takayuki Tsunakawa





win_shrdbuf_perf_v2.patch
Description: win_shrdbuf_perf_v2.patch

-- 
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] [RFC] Should we fix postmaster to avoid slow shutdown?

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Ashutosh Bapat
> I am not sure if following condition is a good idea in ServerLoop()
> 1650 if (pmState == PM_WAIT_DEAD_END || ClosedSockets)
> 
> There are no sockets to listen on, so select in the else condition is going
> to sleep for timeout determined based on the sequence expected.
> Just before we close sockets in pmdie() it sets AbortStartTime, which
> determines the timeout for the sleep here. So, it doesn't make sense to
> ignore it. Instead may be we should change the default 60s sleep to 100ms
> sleep in DetermineSleepTime().

That sounds better.  I modified cleaned ServerLoop() by pushing the existing 
100ms logic into DetermineSleepTime().


> While the postmaster is terminating children, a new connection request may
> arrive. We should probably close listening sockets before terminating
> children in pmdie().

I moved ClosePostmasterSocket() call before terminating children in the 
immediate shutdown case.  I didn't change the behavior of smart and fast 
shutdown modes, because they may take a long time to complete due to 
checkpointing etc.  Users will want to know what's happening during shutdown or 
after pg_ctl stop times out, by getting the message "FATAL:  the database 
system is shutting down" when they try to connect to the database.  The 
immediate shutdown or crash should better be as fast as possible.


> Otherwise this patch looks good to me. It applies and compiles cleanly.
> make check-world doesn't show any failures.

Thank you for reviewing and testing.  The revised patch is attached.

Regards
Takayuki Tsunakawa



02_close_listen_ports_early_v2.patch
Description: 02_close_listen_ports_early_v2.patch

-- 
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] Let's get rid of SPI_push/SPI_pop

[Pavel Stehule]

2016-11-07 2:16 GMT+01:00 Tom Lane :

> The intent of SPI_push/SPI_pop seems to be to draw a boundary line between
> nested layers of SPI callers.  Which is fine, but the SPI_connect and
> SPI_finish calls of the inner layer would suffice for that.  AFAICS,
> the only thing that SPI_push/SPI_pop buy for us is the ability to detect
> a missing SPI_connect or SPI_finish in an inner function layer.  And
> that seems pretty useless, because any such bug in a function would be
> immediately detected in simple testing that calls it without any outer
> level of SPI calls.
>
> As against that, we have the risk of forgotten SPI_push/SPI_pop calls that
> go undetected for years, as just seen in commit fc8b81a29.  We've had that
> type of bug before too, cf 0d4899e44.  And then there's the fact that we
> put conditional SPI_push/SPI_pop calls into various places, eg deac9488d,
> which it seems to me largely destroys whatever debugging value the concept
> did have.
>
> So I think we should just delete these functions and adjust SPI_connect
> and SPI_finish so that they just push/pop a context level unconditionally.
> (Which will make them simpler, not more complicated.)
>
> We can provide do-nothing macros by these names to avoid an API break
> for third-party extensions.
>
> Comments, objections?
>

cannot be there some performance impacts?

Regards

Pavel


>
> 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_hba_file_settings view patch

[Michael Paquier]

On Mon, Nov 7, 2016 at 12:36 PM, Haribabu Kommi
 wrote:
> The added regression test fails for the cases where the server is loaded
> with
> different pg_hba.conf rules during installcheck verification. Updated patch
> is
> attached with removing those tests.

That's not a full review as I just glanced at this patch a couple of seconds...

 #include "utils/guc.h"
+#include "utils/jsonb.h"
 #include "utils/lsyscache.h"
You don't need to include this header when using arrays.

Implementing a test case is possible as well using the TAP
infrastructure. You may want to look at it and help folks testing the
patch more easily with a set of configurations in pg_hba.conf that
cover a maximum of code paths in your patch.
-- 
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] pg_hba_file_settings view patch

[Haribabu Kommi]

On Fri, Oct 28, 2016 at 4:55 PM, Haribabu Kommi 
wrote:

>
>
> On Fri, Oct 28, 2016 at 4:17 AM, Alvaro Herrera 
> wrote:
>
>> Greg Stark wrote:
>>
>> > The fundamental problem is that the pga_hba.conf file has some bits of
>> > complex structure that aren't easily captured by linear arrays. The
>> > problem I struggled with most was the keywords like "all", "samerole",
>> > and "replication". A simple array of text makes it awkward to
>> > distinguish those keywords from the quoted text values with the same
>> > content. And then there are the ldap options which naturally would be
>> > a data type like json or htab.
>>
>> Hmm I thought we had decided that such keywords would live in separate
>> arrays, i.e. you have one array for plain names and another array for
>> keyword stuff.  Then it's not ambiguous anymore.
>
>
>
> Thanks for all your opinions. Here I attached updated patch with the change
> in column datatype from JSONB to TEXT array. Rest of the code changes
> are same to the earlier patch.
>

The added regression test fails for the cases where the server is loaded
with
different pg_hba.conf rules during installcheck verification. Updated patch
is
attached with removing those tests.

Regards,
Hari Babu
Fujitsu Australia


pg_hba_rules_3.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] macaddr 64 bit (EUI-64) datatype support

[Haribabu Kommi]

On Mon, Nov 7, 2016 at 8:40 AM, Shay Rojansky  wrote:

> > 1. Does everyone agrees that renaming the existing datatype without
>> > changing the OID?
>> >
>> >
>> > As I said before, Npgsql for one loads data types by name, not by OID.
>> > So this would definitely cause breakage.
>>
>> Why would that cause breakage?
>
>
> Well, the first thing Npgsql does when it connects to a new database, is
> to query pg_type. The type names are used to associate entries with type
> handlers, avoiding the hard-coding of OIDs in code. So if the type name
> "macaddr" suddenly has a new meaning and its wire representation is
> different breakage will occur. It is possible to release new versions of
> Npgsql which will look at the PostgreSQL version and say "we know that in
> PostgreSQL < 10 macaddr means this, but in >= 10 it means that". But that
> doesn't seem like a good solution, plus old versions of Npgsql from before
> this change won't work.
>

The new datatype that is going to replace the existing one works with both
6 and 8 byte
MAC address and stores it a variable length format. This doesn't change the
wire format.
I don't see any problem with the existing applications. The new datatype
can recv and send
8 byte MAC address also.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Typo in comment in contrib/postgres_fdw/deparse.c

[Etsuro Fujita]

On 2016/11/04 22:04, Robert Haas wrote:

On Fri, Nov 4, 2016 at 7:20 AM, Etsuro Fujita
 wrote:

I found another typo in postgres_fdw.c.  Attached is a patch for fixing
that.



OK, committed that, too.


Thanks again!

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] Push down more full joins in postgres_fdw

[Etsuro Fujita]

On 2016/11/04 19:55, Etsuro Fujita wrote:

Attached is an updated version of the patch.


I noticed that I have included an unrelated regression test in the 
patch.  Attached is a patch with the test removed.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 109,114  typedef struct deparse_expr_cxt
--- 109,116 
  /* Handy macro to add relation name qualification */
  #define ADD_REL_QUALIFIER(buf, varno)	\
  		appendStringInfo((buf), "%s%d.", REL_ALIAS_PREFIX, (varno))
+ #define SS_TAB_ALIAS_PREFIX	"s"
+ #define SS_COL_ALIAS_PREFIX	"c"
  
  /*
   * Functions to determine whether an expression can be evaluated safely on
***
*** 167,172  static void appendConditions(List *exprs, deparse_expr_cxt *context);
--- 169,180 
  static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
  	RelOptInfo *joinrel, bool use_alias, List **params_list);
  static void deparseFromExpr(List *quals, deparse_expr_cxt *context);
+ static void deparseRangeTblRef(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
+    bool make_subquery, List **params_list);
+ static void appendSubselectAlias(StringInfo buf, int tabno, int ncols);
+ static void getSubselectAliasInfo(Var *node, RelOptInfo *foreignrel,
+ 	  int *tabno, int *colno);
+ static bool isSubqueryExpr(Var *node, RelOptInfo *foreignrel, int *tabno, int *colno);
  static void deparseAggref(Aggref *node, deparse_expr_cxt *context);
  static void appendGroupByClause(List *tlist, deparse_expr_cxt *context);
  static void appendAggOrderBy(List *orderList, List *targetList,
***
*** 1155,1165  deparseLockingClause(deparse_expr_cxt *context)
--- 1163,1182 
  	StringInfo	buf = context->buf;
  	PlannerInfo *root = context->root;
  	RelOptInfo *rel = context->scanrel;
+ 	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) rel->fdw_private;
  	int			relid = -1;
  
  	while ((relid = bms_next_member(rel->relids, relid)) >= 0)
  	{
  		/*
+ 		 * Ignore relation if it appears in a lower subquery, because in that
+ 		 * case we would have already considered locking for the relation
+ 		 * while deparsing the lower subquery.
+ 		 */
+ 		if (bms_is_member(relid, fpinfo->subquery_rels))
+ 			continue;
+ 
+ 		/*
  		 * Add FOR UPDATE/SHARE if appropriate.  We apply locking during the
  		 * initial row fetch, rather than later on as is done for local
  		 * tables. The extra roundtrips involved in trying to duplicate the
***
*** 1347,1364  deparseFromExprForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
  
  	if (foreignrel->reloptkind == RELOPT_JOINREL)
  	{
- 		RelOptInfo *rel_o = fpinfo->outerrel;
- 		RelOptInfo *rel_i = fpinfo->innerrel;
  		StringInfoData join_sql_o;
  		StringInfoData join_sql_i;
  
  		/* Deparse outer relation */
  		initStringInfo(_sql_o);
! 		deparseFromExprForRel(_sql_o, root, rel_o, true, params_list);
  
  		/* Deparse inner relation */
  		initStringInfo(_sql_i);
! 		deparseFromExprForRel(_sql_i, root, rel_i, true, params_list);
  
  		/*
  		 * For a join relation FROM clause entry is deparsed as
--- 1364,1385 
  
  	if (foreignrel->reloptkind == RELOPT_JOINREL)
  	{
  		StringInfoData join_sql_o;
  		StringInfoData join_sql_i;
  
  		/* Deparse outer relation */
  		initStringInfo(_sql_o);
! 		deparseRangeTblRef(_sql_o, root,
! 		   fpinfo->outerrel,
! 		   fpinfo->make_outerrel_subquery,
! 		   params_list);
  
  		/* Deparse inner relation */
  		initStringInfo(_sql_i);
! 		deparseRangeTblRef(_sql_i, root,
! 		   fpinfo->innerrel,
! 		   fpinfo->make_innerrel_subquery,
! 		   params_list);
  
  		/*
  		 * For a join relation FROM clause entry is deparsed as
***
*** 1414,1419  deparseFromExprForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
--- 1435,1585 
  }
  
  /*
+  * Append operand relation of foreign join to buf.
+  */
+ static void
+ deparseRangeTblRef(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
+    bool make_subquery, List **params_list)
+ {
+ 	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
+ 
+ 	Assert(foreignrel->reloptkind == RELOPT_BASEREL ||
+ 		   foreignrel->reloptkind == RELOPT_JOINREL);
+ 	Assert(fpinfo->local_conds == NIL);
+ 
+ 	if (make_subquery)
+ 	{
+ 		List	   *tlist;
+ 		List	   *retrieved_attrs;
+ 
+ 		tlist = make_tlist_from_pathtarget(foreignrel->reltarget);
+ 		Assert(tlist != NIL);
+ 		appendStringInfoChar(buf, '(');
+ 		deparseSelectStmtForRel(buf, root, foreignrel, tlist,
+ fpinfo->remote_conds, NIL,
+ _attrs, params_list);
+ 		appendStringInfoChar(buf, ')');
+ 		appendSubselectAlias(buf, fpinfo->relation_index,
+ 			 list_length(foreignrel->reltarget->exprs));
+ 	}
+ 	else
+ 		deparseFromExprForRel(buf, root, foreignrel, true, params_list);
+ }
+ 
+ /*
+  * Add a subselect alias to a subquery-in-FROM.
+  *
+  * The 

Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

[Andreas Karlsson]

Thanks for the review! I have fixed all your feedback in the attached patch.

On 11/03/2016 04:24 PM, Michael Banck wrote:

It does not update the documentation, I think at least section 18.9
"Secure TCP/IP Connections with SSL" needs updating as it says: "The
files server.key, server.crt, root.crt, and root.crl (or their
configured alternative names) are only examined during server start; so
you must restart the server for changes in them to take effect".


Changed this.


However see below for segfaults during testing.


Fixed, this was due to me not setting SSL_Context to NULL after freeing it.


[...]


I am not sure this '!!' operator is according to project policy, it
seems to be not used elsewhere in the codebase except for imported or
auto-generated code. At least there should be a comment here, methinks.


I changed the code to compare with '\0' instead.


[...]
Missing semicolon at the end of the line.
[...]
Opening braces should be on the next line.
[...]
Same.


Fixed.


[...]


All the delarations above this one are global variables for GUCs (see
postmaster.h, lines 16-31).  So ISTM this static variable declaration
should be introduced by a comment in order to logically seperate it from
the previous ones, like the following static variables are:


Fixed.


[...]


This hunk baffled me at first because two lines below your added
secure_destroy() declaration, the same line is already present.  I did
some digging and it turns out we had a secure_destroy() in the ancient
past, but its implementation got removed in 2008 in 4e8162865 as there
were no (more) users of it, however, the declaration was kept on until
now.

So this hunk should be removed I guess.


Removed.

Andreas
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 787cfce..5e78d81 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2288,8 +2288,8 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
 The files server.key, server.crt,
 root.crt, and root.crl
 (or their configured alternative names)
-are only examined during server start; so you must restart
-the server for changes in them to take effect.
+are examined when reloading the configuration, or when spawning the backend
+process on Windows systems.

   
 
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 668f217..8449a53 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -77,12 +77,14 @@ static DH  *generate_dh_parameters(int prime_len, int generator);
 static DH  *tmp_dh_cb(SSL *s, int is_export, int keylength);
 static int	verify_cb(int, X509_STORE_CTX *);
 static void info_cb(const SSL *ssl, int type, int args);
-static void initialize_ecdh(void);
+static SSL_CTX *initialize_context(void);
+static bool initialize_ecdh(SSL_CTX *context);
 static const char *SSLerrmessage(unsigned long ecode);
 
 static char *X509_NAME_to_cstring(X509_NAME *name);
 
 static SSL_CTX *SSL_context = NULL;
+static bool SSL_initialized = false;
 
 /*  */
 /*		 Hardcoded values		*/
@@ -157,14 +159,12 @@ KWbuHn491xNO25CQWMtem80uKw+pTnisBRF/454n1Jnhub144YRBoN8CAQI=\n\
 /*
  *	Initialize global SSL context.
  */
-void
+int
 be_tls_init(void)
 {
-	struct stat buf;
+	SSL_CTX *context;
 
-	STACK_OF(X509_NAME) *root_cert_list = NULL;
-
-	if (!SSL_context)
+	if (!SSL_initialized)
 	{
 #ifdef HAVE_OPENSSL_INIT_SSL
 		OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL);
@@ -173,177 +173,29 @@ be_tls_init(void)
 		SSL_library_init();
 		SSL_load_error_strings();
 #endif
-
-		/*
-		 * We use SSLv23_method() because it can negotiate use of the highest
-		 * mutually supported protocol version, while alternatives like
-		 * TLSv1_2_method() permit only one specific version.  Note that we
-		 * don't actually allow SSL v2 or v3, only TLS protocols (see below).
-		 */
-		SSL_context = SSL_CTX_new(SSLv23_method());
-		if (!SSL_context)
-			ereport(FATAL,
-	(errmsg("could not create SSL context: %s",
-			SSLerrmessage(ERR_get_error();
-
-		/*
-		 * Disable OpenSSL's moving-write-buffer sanity check, because it
-		 * causes unnecessary failures in nonblocking send cases.
-		 */
-		SSL_CTX_set_mode(SSL_context, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
-
-		/*
-		 * Load and verify server's certificate and private key
-		 */
-		if (SSL_CTX_use_certificate_chain_file(SSL_context,
-			   ssl_cert_file) != 1)
-			ereport(FATAL,
-	(errcode(ERRCODE_CONFIG_FILE_ERROR),
-  errmsg("could not load server certificate file \"%s\": %s",
-		 ssl_cert_file, SSLerrmessage(ERR_get_error();
-
-		if (stat(ssl_key_file, ) != 0)
-			ereport(FATAL,
-	(errcode_for_file_access(),
-	 errmsg("could not access private key file \"%s\": %m",
-			ssl_key_file)));
-
-		if (!S_ISREG(buf.st_mode))
-			ereport(FATAL,
-	(errcode(ERRCODE_CONFIG_FILE_ERROR),
-	 

Re: [HACKERS] Mention column name in error messages

[Franck Verrot]

On Sun, Nov 6, 2016 at 1:13 PM, Tom Lane  wrote:
>
> > The original intent of that patch tried to cover the case where we insert
> > records
> > made of dozens columns sharing the same type definition, and trying to
> > understand
> > what is going on, at a glance, when we debugged something like this:
> > ...
> > Relying on the cursor seems to be of little help I'm afraid.
>
> Well, it would be an improvement over what we've got now.  Also, a feature
> similar to what I suggested would help in localizing many types of errors
> that have nothing to do with coercion to a target column type.
>

Yes, it's a neat improvement in any case.

-- 
Franck Verrot

[HACKERS] Let's get rid of SPI_push/SPI_pop

[Tom Lane]

The intent of SPI_push/SPI_pop seems to be to draw a boundary line between
nested layers of SPI callers.  Which is fine, but the SPI_connect and
SPI_finish calls of the inner layer would suffice for that.  AFAICS,
the only thing that SPI_push/SPI_pop buy for us is the ability to detect
a missing SPI_connect or SPI_finish in an inner function layer.  And
that seems pretty useless, because any such bug in a function would be
immediately detected in simple testing that calls it without any outer
level of SPI calls.

As against that, we have the risk of forgotten SPI_push/SPI_pop calls that
go undetected for years, as just seen in commit fc8b81a29.  We've had that
type of bug before too, cf 0d4899e44.  And then there's the fact that we
put conditional SPI_push/SPI_pop calls into various places, eg deac9488d,
which it seems to me largely destroys whatever debugging value the concept
did have.

So I think we should just delete these functions and adjust SPI_connect
and SPI_finish so that they just push/pop a context level unconditionally.
(Which will make them simpler, not more complicated.)

We can provide do-nothing macros by these names to avoid an API break
for third-party extensions.

Comments, objections?

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] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> On Sun, Nov 6, 2016 at 6:30 PM, MauMau  wrote:
> > Sorry, I may have had to send this to pgsql-hackers.  I just replied
> > to all, which did not include pgsql-hackers but pgsql-bugs because
> > this discussion was on pgsql-bugs.  CommitFest app doesn't seem to
> > reflect the mails on pgsql-bugs, so I'm re-submitting this here on
> > pgsql-hackers.
> 
> No problem, I still see a unique thread so that's not an issue seen from
> here.

You are right.  A while after I sent the second mail, I noticed the CommitFest 
app collected both of my mails.  I was just impatient.



> So you see the same behavior with the patch I sent and your refactoring,
> right? If yes, backpatching the one-liner is the safest bet to me. We could
> keep the refactoring for HEAD if it makes sense.

Yes.  And It's fine to me that your patch will be applied to previous releases 
and my patch to HEAD only.  This is a good (rare?) chance to reduce the 
Windows-specific code, so I want to take advantage of it.




> Something is wrong with the format of your patch by the way. My Windows
> and even OSX environments recognize it as a binary file, though I can read
> it in any editor and I cannot apply it cleanly with a simple patch command.
> Could you send it again and double-check?

Ouch, the Git shell included in GitHub Desktop for Windows produced the diff in 
UTF-16 and CR/LF line terminators.  I haven't found how to fix it, so I 
generated the attached patch on Linux.  Please check it.


> > To reproduce the OP's problem, I modified pg_ctl.c to disable
> > SECURITY_SERVICE_RID when spawning postgres.exe.
> 
> So basically you allocated a SID to drop via AllocateAndInitializeSid,
> called _CreateRestrictedToken and let the process being spawned? I think
> that this is the patch attached (win32-disable-service-rid.patch). Could
> you confirm? I want to be sure that we are testing the same things.

Yes, I did the same.

Regards
Takayuki Tsunakawa



win32-security-service-v4.patch
Description: win32-security-service-v4.patch

-- 
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] Effective storage of duplicates in B-tree index.

[Peter Geoghegan]

On Mon, Jul 4, 2016 at 2:30 AM, Heikki Linnakangas  wrote:
> I think we should pack the TIDs more tightly, like GIN does with the varbyte
> encoding. It's tempting to commit this without it for now, and add the
> compression later, but I'd like to avoid having to deal with multiple
> binary-format upgrades, so let's figure out the final on-disk format that we
> want, right from the beginning.

While the idea of duplicate storage is pretty obviously compelling,
there could be other, non-obvious benefits. I think that it could
bring further benefits if we could use duplicate storage to change
this property of nbtree (this is from the README):

"""
Lehman and Yao assume that the key range for a subtree S is described
by Ki < v <= Ki+1 where Ki and Ki+1 are the adjacent keys in the parent
page.  This does not work for nonunique keys (for example, if we have
enough equal keys to spread across several leaf pages, there *must* be
some equal bounding keys in the first level up).  Therefore we assume
Ki <= v <= Ki+1 instead.  A search that finds exact equality to a
bounding key in an upper tree level must descend to the left of that
key to ensure it finds any equal keys in the preceding page.  An
insertion that sees the high key of its target page is equal to the key
to be inserted has a choice whether or not to move right, since the new
key could go on either page.  (Currently, we try to find a page where
there is room for the new key without a split.)

"""

If we could *guarantee* that all keys in the index are unique, then we
could maintain the keyspace as L originally described.

The practical benefits to this would be:

* We wouldn't need to take the extra step described above -- finding a
bounding key/separator key that's fully equal to our scankey would no
longer necessitate a probably-useless descent to the left of that key.
(BTW, I wonder if we could get away with not inserting a downlink into
parent when a leaf page split finds an identical IndexTuple in parent,
*without* changing the keyspace invariant I mention -- if we're always
going to go to the left of an equal-to-scankey key in an internal
page, why even have more than one?)

* This would make suffix truncation of internal index tuples easier,
and that's important.

The traditional reason why suffix truncation is important is that it
can keep the tree a lot shorter than it would otherwise be. These
days, that might not seem that important, because even if you have
twice the number of internal pages than strictly necessary, that still
isn't that many relative to typical main memory size (and even CPU
cache sizes, perhaps).

The reason I think it's important these days is that not having suffix
truncation makes our "separator keys" overly prescriptive about what
part of the keyspace is owned by each internal page. With a pristine
index (following REINDEX), this doesn't matter much. But, I think that
we get much bigger problems with index bloat due to the poor fan-out
that we sometimes see due to not having suffix truncation, *combined*
with the page deletion algorithms restriction on deleting internal
pages (it can only be done for internal pages with *no* children).

Adding another level or two to the B-Tree makes it so that your
workload's "sparse deletion patterns" really don't need to be that
sparse in order to bloat the B-Tree badly, necessitating a REINDEX to
get back to acceptable performance (VACUUM won't do it). To avoid
this, we should make the internal pages represent the key space in the
least restrictive way possible, by applying suffix truncation so that
it's much more likely that things will *stay* balanced as churn
occurs. This is probably a really bad problem with things like
composite indexes over text columns, or indexes with many NULL values.

-- 
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] macaddr 64 bit (EUI-64) datatype support

[Shay Rojansky]

>
> > 1. Does everyone agrees that renaming the existing datatype without
> > changing the OID?
> >
> >
> > As I said before, Npgsql for one loads data types by name, not by OID.
> > So this would definitely cause breakage.
>
> Why would that cause breakage?


Well, the first thing Npgsql does when it connects to a new database, is to
query pg_type. The type names are used to associate entries with type
handlers, avoiding the hard-coding of OIDs in code. So if the type name
"macaddr" suddenly has a new meaning and its wire representation is
different breakage will occur. It is possible to release new versions of
Npgsql which will look at the PostgreSQL version and say "we know that in
PostgreSQL < 10 macaddr means this, but in >= 10 it means that". But that
doesn't seem like a good solution, plus old versions of Npgsql from before
this change won't work.

Re: [HACKERS] Mention column name in error messages

[Tom Lane]

Franck Verrot  writes:
> On Sat, Nov 5, 2016 at 11:13 AM, Tom Lane  wrote:
>> The cases that are successfully annotated by the current patch seem to
>> mostly already have error cursor information, which really is good enough
>> IMO --- you can certainly figure out which column corresponds to the
>> textual spot that the cursor is pointing at.

> The original intent of that patch tried to cover the case where we insert
> records
> made of dozens columns sharing the same type definition, and trying to
> understand
> what is going on, at a glance, when we debugged something like this:
> ...
> Relying on the cursor seems to be of little help I'm afraid.

Well, it would be an improvement over what we've got now.  Also, a feature
similar to what I suggested would help in localizing many types of errors
that have nothing to do with coercion to a target column type.

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] Mention column name in error messages

[Franck Verrot]

On Sat, Nov 5, 2016 at 11:13 AM, Tom Lane  wrote:
>
> The cases that are successfully annotated by the current patch seem to
> mostly already have error cursor information, which really is good enough
> IMO --- you can certainly figure out which column corresponds to the
> textual spot that the cursor is pointing at.



The original intent of that patch tried to cover the case where we insert
records
made of dozens columns sharing the same type definition, and trying to
understand
what is going on, at a glance, when we debugged something like this:


# create table probes (
id int,
pin_1 varchar(2),
pin_2 varchar(2),
...
pin_19 varchar(2),
pin_20 varchar(2));
CREATE TABLE

# insert into probes (
pin_1,
pin_2,
...
pin_19,
pin_20)
  values (  );
INSERT 0 1

# insert into probes (
pin_1,
pin_2,
...
pin_19,
pin_20)
  values (  );
ERROR:  value too long for type character varying(2)


Relying on the cursor seems to be of little help I'm afraid.


Thanks for having looked into that, very useful to try understanding all
the mechanisms that are involved to make that happen.

Franck

-- 
Franck Verrot

Re: [HACKERS] Add support for SRF and returning composites to pl/tcl

[Tom Lane]

I wrote:
> I got the code to a state that I liked (attached), and started reviewing
> the docs, and then it occurred to me to wonder why you'd chosen to use
> Tcl lists to represent composite output values.  The precedent established
> by input argument handling is that composites are transformed to Tcl
> arrays.  So shouldn't we use an array to represent a composite result,
> too?

After further nosing around I see that the return-a-tuple-as-a-list
concept is already embedded in pltcl_trigger_handler.  So the
inconsistency is already there, and it's not necessarily this patch's
job to fix it.  Still seems like we might want to allow using an array
directly rather than insisting on conversion to a list, but that's a
task for a separate patch.

We should, however, make some attempt to ensure that the list-to-tuple
conversion semantics are the same in both cases.  In particular I notice
some undocumented behavior around a magic ".tupno" element.  Will see
about cleaning that up.

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] Re: [HACKERS] Re: [PATCH] Tab completion for ALTER TYPE … RENAME VALUE …

[Artur Zakirov]

Hello,

2016-09-12 16:16 GMT+03:00 Dagfinn Ilmari Mannsåker :
>
> I've added it to the 2016-11 commit fest:
> https://commitfest.postgresql.org/11/795/
>
> - ilmari

I've tested your patch.

Patch was applied to the master. It seems there is no need to rebase
it. PostgreSQL was compiled without errors with the patch.

I've tested the patch with type:

CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green',
'blue', 'purple');

And the following completions work as expected:

=> ALTER TYPE rainbow RENAME 
ATTRIBUTE  TO VALUE

=> ALTER TYPE rainbow RENAME VALUE 
'blue''green'   'orange'  'purple'  'red' 'yellow'

It seems that the patch can be commited without any fixes. So I marked
it as "Ready for Committer".

-- 
Sincerely,
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres 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] Bug in to_timestamp().

[Artur Zakirov]

Thank you for your comments,

2016-11-04 20:36 GMT+02:00 Tom Lane :
>
> Artur Zakirov  writes:
> > I attached new version of the patch, which fix is_char_separator()
> > declaration too.
>
> I did some experimenting using
> http://rextester.com/l/oracle_online_compiler
>

>
> which makes it look a lot like Oracle treats separator characters in the
> pattern the same as spaces (but I haven't checked their documentation to
> confirm that).
>
> The proposed patch doesn't seem to me to be trying to follow
> these Oracle behaviors, but I think there is very little reason for
> changing any of this stuff unless we move it closer to Oracle.

Previous versions of the patch doesn't try to follow all Oracle
behaviors. It tries to fix Amul Sul's behaviors. Because I was
confused by dealing with spaces and separators by Oracle
to_timestamp() and there is not information about it in the Oracle
documentation:
https://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#g195443

I've thought better about it now and fixed the patch. Now parser
removes spaces after and before fields and insists that count of
separators in the input string should match count of spaces or
separators in the formatting string (but in formatting string we can
have more separators than in input string).

>
> Some other nitpicking:
>
> * I think the is-separator function would be better coded like
>
> static bool
> is_separator_char(const char *str)
> {
> /* ASCII printable character, but not letter or digit */
> return (*str > 0x20 && *str < 0x7F &&
> !(*str >= 'A' && *str <= 'Z') &&
> !(*str >= 'a' && *str <= 'z') &&
> !(*str >= '0' && *str <= '9'));
> }
>
> The previous way is neither readable nor remarkably efficient, and it
> knows much more about the ASCII character set than it needs to.

Fixed.

>
> * Don't forget the cast to unsigned char when using isspace() or other
>  functions.

Fixed.

>
> * I do not see the reason for throwing an error here:
>
> +   /* Previous character was a backslash */
> +   if (in_backslash)
> +   {
> +   /* After backslash should go non-space character */
> +   if (isspace(*str))
> +   ereport(ERROR,
> +   
> (errcode(ERRCODE_SYNTAX_ERROR),
> +errmsg("invalid escape 
> sequence")));
> +   in_backslash = false;
>
> Why shouldn't backslash-space be a valid quoting sequence?
>

Hm, truly. Fixed it.

I've attached the fixed patch.

--
Sincerely,
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2e64cc4..5a4e248 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -6159,7 +6159,8 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
to_timestamp and to_date
skip multiple blank spaces in the input string unless the
FX option is used. For example,
-   to_timestamp('2000JUN', ' MON') works, but
+   to_timestamp('2000JUN', ' MON') and
+   to_timestamp('2000JUN', 'MON') work, but
to_timestamp('2000JUN', 'FX MON') returns an error
because to_timestamp expects one space only.
FX must be specified as the first item in
@@ -6169,6 +6170,19 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
 
  
   
+   to_timestamp and to_date don't
+   skip multiple printable non letter and non digit characters in the input
+   string, but skip them in the formatting string. For example,
+   to_timestamp('2000-JUN', '/MON') and
+   to_timestamp('2000/JUN', '//MON') work, but
+   to_timestamp('2000//JUN', '/MON')
+   returns an error because count of the "/" character in the input string
+   doesn't match count of it in the formatting string.
+  
+ 
+
+ 
+  
Ordinary text is allowed in to_char
templates and will be output literally.  You can put a substring
in double quotes to force it to be interpreted as literal text
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index d4eaa50..d28ceec 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -169,6 +169,8 @@ struct FormatNode
 #define NODE_TYPE_END		1
 #define NODE_TYPE_ACTION	2
 #define NODE_TYPE_CHAR		3
+#define NODE_TYPE_SEPARATOR	4
+#define NODE_TYPE_SPACE		5
 
 #define SUFFTYPE_PREFIX		1
 #define SUFFTYPE_POSTFIX	2
@@ -951,6 +953,7 @@ typedef struct NUMProc
 static const KeyWord *index_seq_search(const char *str, const KeyWord *kw,
  const int *index);
 static const KeySuffix *suff_search(const char *str, const KeySuffix *suf, int type);
+static bool is_separator_char(const char 

[HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[Michael Paquier]

On Sun, Nov 6, 2016 at 6:30 PM, MauMau  wrote:
> Sorry, I may have had to send this to pgsql-hackers.  I just replied
> to all, which did not include pgsql-hackers but pgsql-bugs because
> this discussion was on pgsql-bugs.  CommitFest app doesn't seem to
> reflect the mails on pgsql-bugs, so I'm re-submitting this here on
> pgsql-hackers.

No problem, I still see a unique thread so that's not an issue seen from here.

> I reviewed and tested this patch after simplifying it like the
> attached one.  The file could be reduced by about 110 lines.  Please
> review and/or test it.  Though I kept the status "ready for
> committer", feel free to change it back based on the result.

So you see the same behavior with the patch I sent and your
refactoring, right? If yes, backpatching the one-liner is the safest
bet to me. We could keep the refactoring for HEAD if it makes sense.

Something is wrong with the format of your patch by the way. My
Windows and even OSX environments recognize it as a binary file,
though I can read it in any editor and I cannot apply it cleanly with
a simple patch command. Could you send it again and double-check?

> To reproduce the OP's problem, I modified pg_ctl.c to disable
> SECURITY_SERVICE_RID when spawning postgres.exe.

So basically you allocated a SID to drop via AllocateAndInitializeSid,
called _CreateRestrictedToken and let the process being spawned? I
think that this is the patch attached
(win32-disable-service-rid.patch). Could you confirm? I want to be
sure that we are testing the same things.
-- 
Michael
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 4b47602..56c7f2e 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -1738,7 +1738,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
 	HANDLE		origToken;
 	HANDLE		restrictedToken;
 	SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
-	SID_AND_ATTRIBUTES dropSids[2];
+	SID_AND_ATTRIBUTES dropSids[3];
 
 	/* Functions loaded dynamically */
 	__CreateRestrictedToken _CreateRestrictedToken = NULL;
@@ -1790,7 +1790,10 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
   0, [0].Sid) ||
 		!AllocateAndInitializeSid(, 2,
 	SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_POWER_USERS, 0, 0, 0, 0, 0,
-  0, [1].Sid))
+  0, [1].Sid) ||
+		!AllocateAndInitializeSid(, 1,
+  SECURITY_SERVICE_RID, 0, 0, 0, 0, 0, 0,
+  0, [2].Sid))
 	{
 		write_stderr(_("%s: could not allocate SIDs: error code %lu\n"),
 	 progname, (unsigned long) GetLastError());
@@ -1805,6 +1808,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
 			   0, NULL,
 			   );
 
+	FreeSid(dropSids[2].Sid);
 	FreeSid(dropSids[1].Sid);
 	FreeSid(dropSids[0].Sid);
 	CloseHandle(origToken);

-- 
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] commitfest 2016-11 status summary

[Michael Paquier]

On Fri, Nov 4, 2016 at 12:36 AM, Robert Haas  wrote:
> On Tue, Nov 1, 2016 at 3:48 PM, Haribabu Kommi  
> wrote:
> https://commitfest.postgresql.org/11/604/ - pgwin32_is_service not
> checking if SECURITY_SERVICE_SID is disabled

This is quite a particular fix in a particular context. This is as
well waiting some input from the reporter to be sure that the patch
proposed is fixing what his case. The patch should fix it, but that's
a matter to be sure now.

> https://commitfest.postgresql.org/11/620/ - Updating Windows
> environment variables

This one is not that particular. Any committer input is welcome. All
the patches proposed should be HEAD-only considering the lack of
complaints across the years.
-- 
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] BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[MauMau]

Hello,

Sorry, I may have had to send this to pgsql-hackers.  I just replied
to all, which did not include pgsql-hackers but pgsql-bugs because
this discussion was on pgsql-bugs.  CommitFest app doesn't seem to
reflect the mails on pgsql-bugs, so I'm re-submitting this here on
pgsql-hackers.

From: Michael Paquier
(Moved to next CF, with same status "Ready for committer").

I reviewed and tested this patch after simplifying it like the
attached one.  The file could be reduced by about 110 lines.  Please
review and/or test it.  Though I kept the status "ready for
committer", feel free to change it back based on the result.

I tested as follows.  First, I confirmed that pg_is_admin() still
works by running postgres.exe from the Administrator command line:

--
G:\>postgres
Execution of PostgreSQL by a user with administrative permissions is
not
permitted.
The server must be started under an unprivileged user ID to prevent
possible system security compromises.  See the documentation for
more information on how to properly start the server.

G:\>
--



Then, I added the following two elog() calls in postmaster.c so that
pg_is_admin() and pg_is_service() works fine.


--
maybe_start_bgworker();

elog(LOG, "pgwin32_is_admin = %d", pgwin32_is_admin());
elog(LOG, "pgwin32_is_service = %d", pgwin32_is_service());

status = ServerLoop();
--


To reproduce the OP's problem, I modified pg_ctl.c to disable
SECURITY_SERVICE_RID when spawning postgres.exe.  Without the patch,
starting the Windows service emit the following log, showing that
pg_is_service() misjudged that postgres is running as a Windows
service:

LOG:  pgwin32_is_admin = 0
LOG:  pgwin32_is_service = 1

With the patch, the log became correct:

LOG:  pgwin32_is_admin = 0
LOG:  pgwin32_is_service = 0


Regards
Takayuki Tsunakawa



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