Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

[Masahiko Sawada]

On Fri, Nov 17, 2017 at 11:00 AM, Michael Paquier
 wrote:
> On Thu, Nov 16, 2017 at 8:17 PM, Masahiko Sawada  
> wrote:
>> On Thu, Nov 16, 2017 at 1:11 PM, Michael Paquier
>>  wrote:
>>> +   /*
>>> +* Quick exit if session is not keeping around a non-exclusive backup
>>> +* already started.
>>> +*/
>>> +   if (sessionBackupState != SESSION_BACKUP_NON_EXCLUSIVE)
>>> +   return;
>>> I think that it would be more solid to use SESSION_BACKUP_NONE for the
>>> comparison, and complete the assertion after the quick exit as follows
>>> as this code path should never be taken for an exclusive backup:
>>
>> Agreed.
>
> I have spent some time doing an extra lookup with tests involving one
> and two sessions doing backups checking for failure code paths while
> waiting for archives:
> - One session with non-exclusive backup.
> - One session with exclusive backup.
> - One session is exclusive and the second is non-exclusive
> - Both are exclusive.
> Also double-checked on the way the logic around the cleanup callback
> and sessionBackupState during startup, and those look clean to me. One
> thing that was bothering me is that the callback
> nonexclusive_base_backup_cleanup is called after do_pg_start_backup()
> finishes. But between the moment sessionBackupState is set and the
> callback is registered there is no CHECK_FOR_INTERRUPTS or even elog()
> calls so even if the process starting a non-exclusive backup is tried
> to be terminated between the moment the session lock is set and the
> callback is registered things are handled correctly.
>
> So I think that we are basically safe for backups running with the SQL
> interface.

Thank you for double checking!

> However, things are not completely clean for base backups taken using
> the replication protocol. While monitoring more the code, I have
> noticed that perform_base_backup() calls do_pg_stop_backup() *without*
> taking any cleanup action. So if a base backup is interrupted, say
> with SIGTERM, while do_pg_stop_backup() is called and before the
> session lock is updated then it is possible to finish with
> inconsistent in-memory counters. Oops.

Good catch! I'd missed this case.

> No need to play with breakpoints and signals in this case, using
> something like that is enough to create inconsistent counters.
> --- a/src/backend/replication/basebackup.c
> +++ b/src/backend/replication/basebackup.c
> @@ -327,6 +327,8 @@ perform_base_backup(basebackup_options *opt, DIR 
> *tblspcdir)
> }
> PG_END_ENSURE_ERROR_CLEANUP(base_backup_cleanup, (Datum) 0);
>
> +   elog(ERROR, "base backups don't decrement counters here, stupid!");
> +
> endptr = do_pg_stop_backup(labelfile->data, !opt->nowait, );
>
> A simple fix for this one is to call do_pg_stop_backup() before
> PG_END_ENSURE_ERROR_CLEANUP(). By doing so, we also make the callback
> cleanup logic consistent with what is done for the SQL equivalent
> where the callback is removed after finishing going through
> do_pg_stop_backup(). A comment would be adapted here, say something
> like "finish the backup while still holding the cleanup callback to
> avoid inconsistent in-memory data should the this call fail before
> sessionBackupState is updated."
>
> For the start phase, the current logic is fine, because in the case of
> the SQL interface the cleanup callback is registered after finishing
> do_pg_start_backup().
>
> What do you think?

I agree with your approach. It makes sense to me.

Attached updated patch. Please review it.

Regards,

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


fix_do_pg_abort_backup_v7.patch
Description: Binary data

Re: no library dependency in Makefile?

[高增琦]

2017-11-20 2:25 GMT+08:00 Tom Lane :

> =?UTF-8?B?6auY5aKe55Cm?=  writes:
> > I very much look forward to hearing everyone's views on this issue.
> > If the solution mentioned before is ok, I will start to complete it.
>
> Please don't top-post, it makes the flow of the conversation very hard
> to follow.
>

Sorry, my fault, I am so anxious.


> > Recently, I found 'psql' is not rebuilt automatically after
> > editing 'fe_utils/psqlscan.l'.
> > ...
> > It's OK for 'libpq' since 'libpq' is a dynamic library.
> > For a static library such as 'libpgfeutils.a', should we
> > add dependency rule in Makefile?
>
> Hm.  I think what you're saying is that when we copied the makefile
> patterns used for libpq.so to use for libpgport and libpgfeutils,
> we did the wrong thing because those are static not dynamic libraries.
> We don't have to relink psql if libpq.so gets some non-API-relevant
> changes, but we do need to relink it if libpgport.a does, so I suppose
> you are right.  However, this doesn't seem like the right way to
> fix it:
>
> > # add STLIBS as normal prerequisite
> > psql: $(OBJS) $(STLIBS) | submake-libpq submake-libpgport
> submake-libpgfeutils
> >   $(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
>
> Your point is that the static libraries should be treated as normal
> dependencies not order-only ones, so building that behavior like
> this seems pretty bizarre.
>
> I think what we want is something more like
>
> ../../src/port/libpgport.a: submake-libpgport
>
> ../../src/fe_utils/libpgfeutils.a: submake-libpgfeutils
>
> psql: $(OBJS) ../../src/port/libpgport.a 
> ../../src/fe_utils/libpgfeutils.a
> | submake-libpq
> $(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS)
> -o $@$(X)
>
> where of course the library file names need to be wrapped up in macros,
> but this is what it'd look like after macro expansion.  (I'm not sure
> this is right in detail, but my point is that we don't want order-only
> dependencies for these libraries.)
>
> regards, tom lane
>

Thank you, I will try it this way.


-- 
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com

Allowing SSL connection of v11 client to v10 server with SCRAM channel binding

[Michael Paquier]

HI all,

When a client connects during a SCRAM exchange, it has multiple ways
to let the server know what the client supports or not when using
channel binding:
- "n" -> client doesn't support channel binding.
- "y" -> client does support channel binding but thinks the server does not.
- "p" -> client requires channel binding.

On a v10 client, we just need to use the "n" flag because the client
does not support channel binding. This way, a v10 client can connect
to a v10 or v11 server with or without SSL, and this even if the
server has published the SASL mechanism SCRAM-SHA-256-PLUS, which is
used to define channel binding use during SCRAM authentication.

With a v11 client though, things are more fancy:
- If the server publishes the SCRAM-PLUS mechanism, then the client
replies with a "p" message. We are here in the context of an SSL
connection. This is the case of a v11 client, v11 server.
- If using an SSL connection, and the server did not publish
SCRAM-PLUS, then the client uses "y". This is the case of a v11 client
and v10 server.
- For a non-SSL connection, "n" is used. (The server would not have
sent the -PLUS mechanism anyway). This happens for all combinations
without SSL.

When using "n" or "y", the data sent by the client to the server about
the use of channel binding is a base64-encoded string of respectively
"n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
v10 server is able to allow connections with "n,,", but not with
"y,,":
https://www.postgresql.org/message-id/887b6fb7-15fe-239e-2aad-5911d2b09...@2ndquadrant.com

When trying to connect to a v11 client based on current HEAD to a v10
server using SSL, then the connection would fail. The attached patch,
for REL_10_STABLE, allows a server to accept as well as input "eSws",
which is a combination that can now happen. This way, a v10 server
accepts connections from a v11 and newer client with SSL.

Thoughts?
-- 
Michael
diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 9161c885e1..e7423cba51 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -1033,10 +1033,12 @@ read_client_final_message(scram_state *state, char *input)
 
 	/*
 	 * Read channel-binding.  We don't support channel binding, so it's
-	 * expected to always be "biws", which is "n,,", base64-encoded.
+	 * expected to always be "biws", which is "n,,", or 'eSws', which is
+	 * "y,,", base64-encoded.
 	 */
 	channel_binding = read_attr_value(, 'c');
-	if (strcmp(channel_binding, "biws") != 0)
+	if (strcmp(channel_binding, "biws") != 0 &&
+		strcmp(channel_binding, "eSws") != 0)
 		ereport(ERROR,
 (errcode(ERRCODE_PROTOCOL_VIOLATION),
  (errmsg("unexpected SCRAM channel-binding attribute in client-final-message";

Re: [HACKERS] [PATCH] A hook for session start

[Tom Lane]

Michael Paquier  writes:
> On Mon, Nov 20, 2017 at 12:11 PM, Tom Lane  wrote:
>> The stuff related to AuxProcType is in miscadmin.h, so one possibility
>> is to put the new enum there.  But I could see inventing a whole new
>> header for this, "utils/proctype.h" or so.

> I am on board for a new, dedicated, header, miscadmin.h having already
> a lot of things. So this API could just be located in its own file say
> in src/backend/utils/misc?

Maybe I'm missing something, but it seems like the only .c infrastructure
the header would need is a backend-global variable "MyProcType" or so.
That could perfectly well live in globals.c.

The model I'm imagining at the moment is that we generalize AuxProcType
to classify all PG processes not just "aux" processes.  The new header
would need to contain the enum typedef, an extern for the global
variable, and a bunch of test macros --- we'd move AmBootstrapProcess(),
IsAutoVacuumWorkerProcess(), and the rest of those into that header.
Whatever else pgstat is doing could probably stay in pgstat.c.  The
other infrastructure needed is for each kind of process to set MyProcType
as soon as possible during process start, but that's inherently something
not very centralizable.

Alternatively, we keep AuxProcType as-is, and invent a new enum that
classifies everything else, with one of its values being "AuxProc".
I'm not sure that sort of two-level scheme has any advantage, but
it's hard to tell for sure without doing the work to make a patch.

regards, tom lane

Re: [HACKERS] [PATCH] A hook for session start

[Michael Paquier]

On Mon, Nov 20, 2017 at 12:11 PM, Tom Lane  wrote:
> This is really about consolidating a whole bunch of ad-hoc stuff.
> I don't think pgstat has any particular pride of place here.  It
> should be one consumer of a common API.
>
> The stuff related to AuxProcType is in miscadmin.h, so one possibility
> is to put the new enum there.  But I could see inventing a whole new
> header for this, "utils/proctype.h" or so.

I am on board for a new, dedicated, header, miscadmin.h having already
a lot of things. So this API could just be located in its own file say
in src/backend/utils/misc?
-- 
Michael

Re: [HACKERS] [PATCH] A hook for session start

[Tom Lane]

Michael Paquier  writes:
> I was the one suggesting to Fabrizio to look at how backend types are
> evaluated in pgstat.c after an off-list discussion. Agreed that this
> result is fragile as this makes two places dependent on the process
> types. Why not simply moving all the business of pgstat_bestart()
> evaluating which st_backendType to use into a common routine and have
> pgstat.c and have this utility use this API? pgstat.h already includes
> BackendType as an enum to use so this could live with a routine
> available in pgstat.c. Or would it be cleaner to have a new thing
> dedicated to process-related information, say as
> src/backend/utils/misc/process_info.c? It is indeed strange to have a
> session-related feature depend on something with statistics.

This is really about consolidating a whole bunch of ad-hoc stuff.
I don't think pgstat has any particular pride of place here.  It
should be one consumer of a common API.

The stuff related to AuxProcType is in miscadmin.h, so one possibility
is to put the new enum there.  But I could see inventing a whole new
header for this, "utils/proctype.h" or so.

regards, tom lane

Re: [HACKERS] CUBE seems a bit confused about ORDER BY

[Tomas Vondra]

On 10/30/2017 03:04 PM, Alexander Korotkov wrote:
> On Sun, Oct 29, 2017 at 1:30 AM, Alexander Korotkov
> > wrote:
...
> 
> Thus, I heard no objection to my approach.  Attached patch changes ~>
> operator in the proposed way and fixes KNN-GiST search for that.  I'm
> going to register this patch to the nearest commitfest.
> 

Seems fine to me, although perhaps it should be split into two parts.
One with the cube_coord_llur fixes, and then g_cube_distance changes
adding support for negative coordinates.

regards

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

RE: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)

[Badrul Chowdhury]

>> I spent a little more time looking at this patch today.  I think that the 
>> patch
>> should actually send NegotiateProtocolVersion when *either* the requested
>> version is differs from the latest one we support *or* an unsupported 
>> protocol
>> option is present.  Otherwise, you only find out about unsupported protocol
>> options if you also request a newer minor version, which isn't good, because 
>> it
>> makes it hard to add new protocol options *without* bumping the protocol
>> version.

It makes sense from a maintainability point of view.

>> Here's an updated version with that change and a proposed commit message.

I have tested the new patch and it works great. The comments look good as well.

Thanks,
Badrul

>> -Original Message-
>> From: Robert Haas [mailto:robertmh...@gmail.com]
>> Sent: Wednesday, November 15, 2017 1:12 PM
>> To: Badrul Chowdhury 
>> Cc: Tom Lane ; Satyanarayana Narlapuram
>> ; Craig Ringer
>> ; Peter Eisentraut ; Magnus
>> Hagander ; PostgreSQL-development > hack...@postgresql.org>
>> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq
>> PGRES_COPY_BOTH - version compatibility)
>> 
>> On Mon, Oct 30, 2017 at 9:19 PM, Badrul Chowdhury
>>  wrote:
>> > Thank you for the comprehensive review! We are very much in the early
>> stages of contributing to the PG community and we clearly have lots to learn,
>> but we look forward to becoming proficient and active members of the pg
>> community.
>> >
>> > Regarding the patch, I have tested it extensively by hand and it works 
>> > great.
>> 
>> I spent a little more time looking at this patch today.  I think that the 
>> patch
>> should actually send NegotiateProtocolVersion when *either* the requested
>> version is differs from the latest one we support *or* an unsupported 
>> protocol
>> option is present.  Otherwise, you only find out about unsupported protocol
>> options if you also request a newer minor version, which isn't good, because 
>> it
>> makes it hard to add new protocol options *without* bumping the protocol
>> version.
>> 
>> Here's an updated version with that change and a proposed commit message.
>> 
>> --
>> Robert Haas
>> EnterpriseDB:
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent
>> erprisedb.com=02%7C01%7Cbachow%40microsoft.com%7Ce37b69223
>> a144d1e5b7408d52c6d8171%7C72f988bf86f141af91ab2d7cd011db47%7C1
>> %7C0%7C636463771208784375=1%2FDylQIfS2rI2RwIVyZnDCUbzRQJe
>> V4YM8J496QkpiQ%3D=0
>> The Enterprise PostgreSQL Company

Re: [HACKERS] GnuTLS support

[Tomas Vondra]

Hi,

On 11/02/2017 11:33 PM, Andreas Karlsson wrote:
> On 09/18/2017 07:04 PM, Jeff Janes wrote:> You fixed the first issue,
> but I still get the second one:
>>
>> be-secure-gnutls.c: In function 'get_peer_certificate':
>> be-secure-gnutls.c:667: error: 'GNUTLS_X509_CRT_LIST_SORT' undeclared
>> (first use in this function)
>> be-secure-gnutls.c:667: error: (Each undeclared identifier is reported
>> only once
>> be-secure-gnutls.c:667: error: for each function it appears in.)
> 
> Thanks again for testing the code. I have now rebased the patch and
> fixed the second issue. I tested that it works on CentOS 6.
> 
> Work which remains:
> 
> - sslinfo
> - pgcrypto
> - Documentation
> - Decide if what I did with the config is a good idea
> 

I don't want to be the annoying guy, but this patch no longer applies
due to 642bafa0c5f9f08d106a14f31429e0e0c718b603 touching the tests :-(

BTW regarding the config, I believe you've kept is static (no hiding of
sections based on configure parameters), and you've separated the
openssl and gnutls options, right? Seems fine to me. The one thing is
that it was proposed to rename the openssl-specific options to start
with openssl_ instead of ssl_.

One question though. What happens when you do

  ./configure --with-openssl --with-gnutls

If I get it right we ignore gnutls and use openssl (as it's the first
checked in #ifdefs). Shouldn't we enforce in configure that only one TLS
implementation is enabled? Either by some elaborate check, or by
switching to something like

 --with-ssl=(openssl|gnutls)


regards

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

RE: Speed up the removal of WAL files

[Tsunakawa, Takayuki]

Thanks, it seems to require a bit more consideration about 
RemoveOldXLogFiles().  Let me continue this next month.


Regards
Takayuki Tsunakawa


> -Original Message-
> From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> Sent: Saturday, November 18, 2017 10:37 PM
> To: Fujii Masao
> Cc: Tsunakawa, Takayuki/綱川 貴之; Kyotaro HORIGUCHI;
> pgsql-hack...@postgresql.org
> Subject: Re: Speed up the removal of WAL files
> 
> On Sat, Nov 18, 2017 at 2:57 AM, Fujii Masao  wrote:
> > On Fri, Nov 17, 2017 at 5:20 PM, Tsunakawa, Takayuki
> >  wrote:
> >> From: Kyotaro HORIGUCHI [mailto:horiguchi.kyot...@lab.ntt.co.jp]
> >>> The orinal code recycles some of the to-be-removed files, but the
> >>> patch removes all the victims.  This may impact on performance.
> >>
> >> Yes, I noticed it after submitting the patch and was wondering what to
> do.  Thinking simply, I think it would be just enough to replace
> durable_unlink/durable_rename in RemoveXLogFile() with unlink/rename, and
> sync the pg_wal directory once in RemoveNonParentXlogFiles() and
> RemoveOldXlogFiles().  This will benefit the failover time when fast
> promotion is not performed.  What do you think?
> 
> Note that durable_rename() also flushes the origin file to make sure that
> it does not show up again after a crash.
> 
> > It seems not good idea to just replace durable_rename() with rename()
> > in RemoveOldXlogFiles()->RemoveXlogFiles()->InstallXLogFileSegment().
> > Because that change seems to be able to cause the following problem.
> 
> If archiving is enabled, RemoveOldXlogFiles() would create a .ready flag
> for all segments that have reappeared. Oops, it archived again.

Re: [HACKERS] Custom compression methods

[Tomas Vondra]

On 11/15/2017 02:13 PM, Robert Haas wrote:
> On Wed, Nov 15, 2017 at 4:09 AM, Ildus Kurbangaliev
>  wrote:
>> So in the next version of the patch I can just unlink the options from
>> compression methods and dropping compression method will not affect
>> already compressed tuples. They still could be decompressed.
> 
> I guess I don't understand how that can work.  I mean, if somebody
> removes a compression method - i.e. uninstalls the library - and you
> don't have a way to make sure there are no tuples that can only be
> uncompressed by that library - then you've broken the database.
> Ideally, there should be a way to add a new compression method via an
> extension ... and then get rid of it and all dependencies thereupon.
> 

I share your confusion. Once you do DROP COMPRESSION METHOD, there must
be no remaining data compressed with it. But that's what the patch is
doing already - it enforces this using dependencies, as usual.

Ildus, can you explain what you meant? How could the data still be
decompressed after DROP COMPRESSION METHOD, and possibly after removing
the .so library?

regards

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

Re: [HACKERS] Custom compression methods

[Tomas Vondra]

Hi,

On 11/14/2017 02:23 PM, Ildus Kurbangaliev wrote:
>
> ...
>
> Attached version 4 of the patch. Fixed pg_upgrade and few other bugs.
> 

I did a review of this today, and I think there are some things that
need improvement / fixing.

Firstly, some basic comments from just eye-balling the diff, then some
bugs I discovered after writing an extension adding lz4.

1) formatRelOptions/freeRelOptions are no longer needed (I see Ildar
already pointer that out)

2) There's unnecessary whitespace (extra newlines) on a couple of
places, which is needlessly increasing the size of the patch. Small
difference, but annoying.

3) tuptoaster.c

Why do you change 'info' from int32 to uint32? Seems unnecessary.

Adding new 'att' variable in toast_insert_or_update is confusing, as
there already is 'att' in the very next loop. Technically it's correct,
but I'd bet it'll lead to some WTF?! moments later. I propose to just
use TupleDescAttr(tupleDesc,i) on the two places where it matters,
around line 808.

There are no comments for init_compression_options_htab and
get_compression_options_info, so that needs to be fixed. Moreover, the
names are confusing because what we really get is not just 'options' but
the compression routines too.

4) gen_db_file_maps probably shouldn't do the fprints, right?

5) not sure why you modify src/tools/pgindent/exclude_file_patterns

6) I'm rather confused by AttributeCompression vs. ColumnCompression. I
mean, attribute==column, right? Of course, one is for data from parser,
the other one is for internal info. But can we make the naming clearer?

7) The docs in general are somewhat unsatisfactory, TBH. For example the
ColumnCompression has no comments, unlike everything else in parsenodes.
Similarly for the SGML docs - I suggest to expand them to resemble FDW
docs (https://www.postgresql.org/docs/10/static/fdwhandler.html) which
also follows the handler/routines pattern.

8) One of the unclear things if why we even need 'drop' routing. It
seems that if it's defined DropAttributeCompression does something. But
what should it do? I suppose dropping the options should be done using
dependencies (just like we drop columns in this case).

BTW why does DropAttributeCompression mess with att->attisdropped in
this way? That seems a bit odd.

9) configure routines that only check if (options != NIL) and then error
out (like tsvector_configure) seem a bit unnecessary. Just allow it to
be NULL in CompressionMethodRoutine, and throw an error if options is
not NIL for such compression method.

10) toast_compress_datum still does this:

if (!ac && (valsize < PGLZ_strategy_default->min_input_size ||
valsize > PGLZ_strategy_default->max_input_size))

which seems rather pglz-specific (the naming is a hint). Why shouldn't
this be specific to compression, exposed either as min/max constants, or
wrapped in another routine - size_is_valid() or something like that?

11) The comments in toast_compress_datum probably need updating, as it
still references to pglz specifically. I guess the new compression
methods do matter too.

12) get_compression_options_info organizes the compression info into a
hash table by OID. The hash table implementation assumes the hash key is
at the beginning of the entry, but AttributeCompression is defined like
this:

typedef struct
{
CompressionMethodRoutine *routine;
List  *options;
Oid cmoptoid;
} AttributeCompression;

Which means get_compression_options_info is busted, will never lookup
anything, and the hash table will grow by adding more and more entries
into the same bucket. Of course, this has extremely negative impact on
performance (pretty much arbitrarily bad, depending on how many entries
you've already added to the hash table).

Moving the OID to the beginning of the struct fixes the issue.

13) When writing the experimental extension, I was extremely confused
about the regular varlena headers, custom compression headers, etc. In
the end I stole the code from tsvector.c and whacked it a bit until it
worked, but I wouldn't dare to claim I understand how it works.

This needs to be documented somewhere. For example postgres.h has a
bunch of paragraphs about varlena headers, so perhaps it should be
there? I see the patch tweaks some of the constants, but does not update
the comment at all.

Perhaps it would be useful to provide some additional macros making
access to custom-compressed varlena values easier. Or perhaps the
VARSIZE_ANY / VARSIZE_ANY_EXHDR / VARDATA_ANY already support that? This
part is not very clear to me.



regards

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


pg_lz4.tgz
Description: application/compressed-tar

Re: [HACKERS] [PATCH] A hook for session start

[Andrew Dunstan]

On 11/19/2017 04:49 PM, Tom Lane wrote:
> Andrew Dunstan  writes:
>> I think this:
>> #define IsClientBackend() \
>>     (MyBackendId != InvalidBackendId &&    \
>>  !IsAutoVacuumLauncherProcess() &&    \
>>  !IsAutoVacuumWorkerProcess() && \
>>  !am_walsender && \
>>  !IsBackgroundWorker)
>> probably belongs somewhere more central. Surely this isn't the only
>> place that we want to be able to run such a test?
> Hm.  It also seems awfully awkward.  Perhaps it's not being used anyplace
> performance-critical, but regardless of speed it seems like a modularity
> violation, in that client backends have to be explicitly aware of
> everything that isn't a "client backend".
>
> Maybe it's time to invent something corresponding to AuxProcType
> for non "aux" processes, or else to fold all types of Postgres
> processes into the same enum.



Yes, agreed, The above is pretty ugly and likely to be quite fragile.

cheers

andrew

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

Re: [HACKERS] [PATCH] A hook for session start

[Tom Lane]

Andrew Dunstan  writes:
> I think this:

> #define IsClientBackend() \
>     (MyBackendId != InvalidBackendId &&    \
>  !IsAutoVacuumLauncherProcess() &&    \
>  !IsAutoVacuumWorkerProcess() && \
>  !am_walsender && \
>  !IsBackgroundWorker)

> probably belongs somewhere more central. Surely this isn't the only
> place that we want to be able to run such a test?

Hm.  It also seems awfully awkward.  Perhaps it's not being used anyplace
performance-critical, but regardless of speed it seems like a modularity
violation, in that client backends have to be explicitly aware of
everything that isn't a "client backend".

Maybe it's time to invent something corresponding to AuxProcType
for non "aux" processes, or else to fold all types of Postgres
processes into the same enum.

regards, tom lane

Re: [HACKERS] [PATCH] A hook for session start

[Andrew Dunstan]

On 11/16/2017 10:38 PM, Fabrízio de Royes Mello wrote:
> Hi all,
>
> Attached new version of the patch fixing issues about installcheck and
> some assertions reported before (based on Michael Paquier code):
>
> https://www.postgresql.org/message-id/flat/30479.1510800078%40sss.pgh.pa.us#30479.1510800...@sss.pgh.pa.us
> https://www.postgresql.org/message-id/flat/20171116121823.GI4628%40tamriel.snowman.net#20171116121823.gi4...@tamriel.snowman.net
>
> Regards,



I think this:

#define IsClientBackend() \
    (MyBackendId != InvalidBackendId &&    \
 !IsAutoVacuumLauncherProcess() &&    \
 !IsAutoVacuumWorkerProcess() && \
 !am_walsender && \
 !IsBackgroundWorker)


probably belongs somewhere more central. Surely this isn't the only
place that we want to be able to run such a test?

cheers

andrew

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

Re: [HACKERS] [PATCH] Incremental sort

[Thomas Munro]

On Wed, Nov 15, 2017 at 7:42 AM, Alexander Korotkov
 wrote:
> Sure, please find rebased patch attached.

+ /*
+  * Check if first "skipCols" sort values are equal.
+  */
+ static bool
+ cmpSortSkipCols(IncrementalSortState *node, TupleTableSlot *a,
+ TupleTableSlot *b)
+ {
+ int n, i;
+
+ Assert(IsA(node->ss.ps.plan, IncrementalSort));
+
+ n = ((IncrementalSort *) node->ss.ps.plan)->skipCols;
+
+ for (i = 0; i < n; i++)
+ {
+ Datum datumA, datumB, result;
+ bool isnullA, isnullB;
+ AttrNumber attno = node->skipKeys[i].attno;
+ SkipKeyData *key;
+
+ datumA = slot_getattr(a, attno, );
+ datumB = slot_getattr(b, attno, );
+
+ /* Special case for NULL-vs-NULL, else use standard comparison */
+ if (isnullA || isnullB)
+ {
+ if (isnullA == isnullB)
+ continue;
+ else
+ return false;
+ }
+
+ key = >skipKeys[i];
+
+ key->fcinfo.arg[0] = datumA;
+ key->fcinfo.arg[1] = datumB;
+
+ /* just for paranoia's sake, we reset isnull each time */
+ key->fcinfo.isnull = false;
+
+ result = FunctionCallInvoke(>fcinfo);
+
+ /* Check for null result, since caller is clearly not expecting one */
+ if (key->fcinfo.isnull)
+ elog(ERROR, "function %u returned NULL", key->flinfo.fn_oid);
+
+ if (!DatumGetBool(result))
+ return false;
+ }
+ return true;
+ }

Is there some reason not to use ApplySortComparator for this?  I think
you're missing out on lower-overhead comparators, and in any case it's
probably good code reuse, no?

Embarrassingly, I was unaware of this patch and started prototyping
exactly the same thing independently[1].  I hadn't got very far and
will now abandon that, but that's one thing I did differently.  Two
other things that may be different: I had a special case for groups of
size 1 that skipped the sorting, and I only sorted on the suffix
because I didn't put tuples with different prefixes into the sorter (I
was assuming that tuplesort_reset was going to be super efficient,
though I hadn't got around to writing that).  I gather that you have
determined empirically that it's better to be able to sort groups of
at least MIN_GROUP_SIZE than to be able to skip the comparisons on the
leading attributes, but why is that the case?

[1] 
https://github.com/macdice/postgres/commit/ab0f8aff9c4b25d5598aa6b3c630df864302f572

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

Re: percentile value check can be slow

[jotpe]

On 19.11.2017 18:49, David Fetter wrote:

On Sun, Nov 19, 2017 at 01:23:42PM +0100, Tomas Vondra wrote:

Hi,

On 11/19/2017 03:10 AM, David Fetter wrote:

On Sat, Nov 18, 2017 at 11:05:47PM +0100, Tomas Vondra wrote:

Hi,

...

Is 'recognizer' an established definition I should know? Is it the same
as 'validator' or is it something new/different?


I borrowed it from http://langsec.org/

I'm not entirely sure what you mean by a validator, but a recognizer
is something that gives a quick and sure read as to whether the input
is well-formed. In general, it's along the lines of a tokenizer, a
parser, and something that does very light post-parse analysis for
correctness of form.

For the case that started the thread, a recognizer would check
something along the lines of

 CHECK('[0,1]' @> ALL(input_array))


OK, thanks. From what I understand, recognizer is more about recognizing
if a string is valid within a given formal language (essentially, if
it's a well-formed program). That may not be the right term for checks
on parameter values.


There are two hard problems in computer science: naming things, cache
coherency, and off-by-one.


OTOH we already have "validators" on a number of places - functions
checking various parameters, e.g. reloptions for FDWs, etc.

But I guess the naming can be solved later ...


Indeed.


Way Bigger Lift, As Far As I Can Tell, But More Fun For Users:
 Allow optional CHECK constraints in CREATE AGGREGATE for direct
 arguments.



How will any of the approaches deal with something like

 select percentile_cont((select array_agg(v) from p))
within group (order by a) from t;

In this case the the values are unknown after the parse analysis, so I
guess it does not really address that.


It doesn't.  Does it make sense to do a one-shot execution for cases
like that?  It might well be worth it to do the aggregate once in
advance as a throw-away if the query execution time is already going
to take awhile.  Of course, you can break that one by making p a JOIN
to yet another thing...


FWIW while I won't stand in the way of improving this, I wonder if this
is really worth the additional complexity. If you get errors like this
with a static list of values, you will fix the list and you're done. If
the list is dynamic (computed in the query itself), you'll still get the
error much later during query execution.

So if you're getting many failures like this for the "delayed error
reporting" to be an issue, perhaps there's something wrong in you stack
and you should address that instead?


I'd like to think that getting something to fail quickly and cheaply
when it can will give our end users a better experience.  Here,
"cheaply" refers to their computing resources and time.


The trouble is, this increases execution time for everyone, including
people who carefully construct the parameter values. That seems rather
undesirable.


I may be wrong but I'm pretty sure that a check for well-formed direct
parameters will not impose a significant cost on aggregates.

It occurs to me that this particular aggregate could take an array of
a domain defined along the lines of:

 CREATE DOMAIN float4_0_1_closed AS float4
 NOT NULL
 CHECK(VALUE >= 0.0 AND VALUE <= 1.0);

Then the check would happen much earlier without adding a bunch of
potentially expensive machinery.


Clearly, not having this happen in this case bothered Johannes
enough to wade in here.


No. He was surprised the error is reported after significant amount
of time, but he does not seem to claim failing faster would be
valuable to him. That is your assumption, and I have my doubts about
it.


My mistake.  I shouldn't have guessed when there was a better
alternative.


I already wrote that: I did not know about the complexity that is needed 
to precheck the parameters. I thought maybe it could be done easily.

If it's too hard to change that, I wouldn't want that improvement.


Johannes, could you help us understand your thinking in reporting
this?


When executing this query was wanted to see whats happening with wrong 
fractions. And I expected to fail this query quick, but it seams the 
postgresql has to read a lot until it checks (in the end) that on 
parameter was out the valid area.


I thougt, maybe there are there are technical forces to do it that way, 
or it can just be improved to fail fast.


Best regards.

Re: [HACKERS] [PATCH] Improve geometric types

[Emre Hasegeli]

> dist_pl is changed to take the smaller distance of both ends of
> the segment. It seems absorbing error, so it might be better
> taking the mean of the two distances. If you have a firm reason
> for the change, it is better to be written there, or it might be
> better left alone.

I am sorry for not paying enough attention to this before.  The
distance functions are meant to return the minimum distance.  Getting
the maximum is just wrong in here.  Can you think of any particular
error it would absorb?  Otherwise I will put this change back to the
patch for fixes.

Re: [HACKERS] [WIP] Zipfian distribution in pgbench

[Alik Khilazhev]

Hello Fabien,

Sorry for not respondingfor long time. 

> Two typos:
> - "usinng" -> "using"
> - "a rejection method used" -> "a rejection method is used"
> 
> I'm not sure of "least_recently_used_i", this naming style is not used in 
> pgbench. "least_recently_used" would be ok.
> 
> "..nb_cells.. != ZIPF_CACHE_SIZE", ISTM that "<" is more logical,
> even if the result is the same?

Fixed.

> Maybe the cache overflow could be counted, to allow for a possible warning 
> message in the final report?
Also done. But one question: Should it be done by printing to stdout or stderr ?

> When/if the pgbench tap test patch get through, coverage tests should
> be added.

I have added few tests and I am not sure if they OK or not. It was my first 
experience with perl and tests in PostgreSQL. 


pgbench-zipf-09v.patch
Description: Binary data

—
Thanks and Regards,
Alik Khilazhev
Postgres Professional:
http://www.postgrespro.com
The Russian Postgres Company

Re: Patch to add dependency between client executes and static libraries

[Tom Lane]

=?UTF-8?B?6auY5aKe55Cm?=  writes:
> The attached patch trying to add dependency between client executes
> and static libraries.

If this is intended to solve the problem described in

then it should be added to that thread rather than starting a new one.

But please see the comment I just posted in that thread --- I don't
think we want to continue using order-only dependencies for these
libraries.

regards, tom lane

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

On Thu, Nov 16, 2017 at 10:37:40PM +0100, Dmitry Dolgov wrote:
> I had hard time parsing this, but from your examples I assume you're talking
> about passing little bit different arguments to `fetch` function (am I
> right?).

Yes, I meant to pass the following arguments:

Datum source, SubscriptingRefState *state, bool *isNull


I think it is time to mark the patch as "Ready for Commiter". I've
marked it.

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

Re: [HACKERS] Bug in to_timestamp().

[Arthur Zakirov]

On Sat, Nov 18, 2017 at 05:48:26PM -0500, Tom Lane wrote:
> This patch needs a rebase over the formatting.c fixes that have gone
> in over the last couple of days.
> 
> Looking at the rejects, I notice that in your changes to parse_format(),
> you seem to be making it rely even more heavily on remembering state about
> the previous input.  I recommend against that --- it was broken before,
> and it's a pretty fragile approach.  Backslashes are not that hard to
> deal with in-line.

I can continue to work on a better approach. Though, the patch was made a long 
time
ago I'll refresh my memory. The main intent was to fix the following
behaviour:

=# SELECT TO_TIMESTAMP('2016-06-13 15:43:36', '/MM/DD  HH24:MI:SS');
to_timestamp 

2016-06-13 05:43:36-07   <— incorrect time

> select to_timestamp('97/Feb/16', 'YY:Mon:DD')
> select to_timestamp('97/Feb/16', 'YY Mon DD')
> select to_timestamp('97 Feb 16', 'YY/Mon/DD')
> 
> (Well, Oracle thinks these mean 2097 where we think 1997, but the point is
> you don't get an error.)  I see from your regression test additions that
> you want to make some of these throw an error, and I think that any such
> proposal is dead in the water.  Nobody is going to consider it an
> improvement if it both breaks working PG apps and disagrees with Oracle.
> 
>   regards, tom lane

If I understand correctly, these queries don't throw an error and the patch 
tried to follow Oracle's rules.
Only queries with FX field throw an error. For example:

=# SELECT to_timestamp('97/Feb/16', 'FXYY:Mon:DD');
ERROR:  unexpected character "/", expected ":"

>From Oracle's documentation [1]:

> FX - Requires exact matching between the character data and the format model.

I agree that compatibility breaking is not good and a fu
ure patch may only try to fix wrong output date and time as in Amul's first 
email.

1 - 
https://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#i34924

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

Re: percentile value check can be slow

[Tomas Vondra]

Hi,

On 11/19/2017 03:10 AM, David Fetter wrote:
> On Sat, Nov 18, 2017 at 11:05:47PM +0100, Tomas Vondra wrote:
>> Hi,
>>
>> ...
>>
>> Is 'recognizer' an established definition I should know? Is it the same
>> as 'validator' or is it something new/different?
> 
> I borrowed it from http://langsec.org/
> 
> I'm not entirely sure what you mean by a validator, but a recognizer
> is something that gives a quick and sure read as to whether the input
> is well-formed. In general, it's along the lines of a tokenizer, a
> parser, and something that does very light post-parse analysis for
> correctness of form.
> 
> For the case that started the thread, a recognizer would check
> something along the lines of 
> 
> CHECK('[0,1]' @> ALL(input_array))
> 

OK, thanks. From what I understand, recognizer is more about recognizing
if a string is valid within a given formal language (essentially, if
it's a well-formed program). That may not be the right term for checks
on parameter values.

OTOH we already have "validators" on a number of places - functions
checking various parameters, e.g. reloptions for FDWs, etc.

But I guess the naming can be solved later ...

>>> Way Bigger Lift, As Far As I Can Tell, But More Fun For Users:
>>> Allow optional CHECK constraints in CREATE AGGREGATE for direct
>>> arguments.
>>>
>>
>> How will any of the approaches deal with something like
>>
>> select percentile_cont((select array_agg(v) from p))
>>within group (order by a) from t;
>>
>> In this case the the values are unknown after the parse analysis, so I
>> guess it does not really address that.
> 
> It doesn't.  Does it make sense to do a one-shot execution for cases
> like that?  It might well be worth it to do the aggregate once in
> advance as a throw-away if the query execution time is already going
> to take awhile.  Of course, you can break that one by making p a JOIN
> to yet another thing...
> 
>> FWIW while I won't stand in the way of improving this, I wonder if this
>> is really worth the additional complexity. If you get errors like this
>> with a static list of values, you will fix the list and you're done. If
>> the list is dynamic (computed in the query itself), you'll still get the
>> error much later during query execution.
>>
>> So if you're getting many failures like this for the "delayed error
>> reporting" to be an issue, perhaps there's something wrong in you stack
>> and you should address that instead?
> 
> I'd like to think that getting something to fail quickly and cheaply
> when it can will give our end users a better experience.  Here,
> "cheaply" refers to their computing resources and time.

The trouble is, this increases execution time for everyone, including
people who carefully construct the parameter values. That seems rather
undesirable.

>
> Clearly, not having this happen in this case bothered Johannes
> enough to wade in here.
> 

No. He was surprised the error is reported after significant amount of
time, but he does not seem to claim failing faster would be valuable to
him. That is your assumption, and I have my doubts about it.

regards

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

Patch to add dependency between client executes and static libraries

[高增琦]

Hi, all

The attached patch trying to add dependency between client executes
and static libraries.

As described in message:
https://www.postgresql.org/message-id/flat/CAFmBtr1GDF%2BCpw%2B7SZF8jWGeazOd%3D%3DivRAg3rWhLaRXYCv83Vg%40mail.gmail.com#CAFmBtr1GDF+Cpw+7SZF8jWGeazOd==ivrag3rwhlarxycv8...@mail.gmail.com

I add a STLIBS variable in Makefiles as normal prerequisite.
Then, for example, if you update something in psqlscan.l, then make will
rebuild psql automatically.

Since I don't know if the la
​c​
k of dependency is problem or not,
this patch maybe useless.

Thanks,


--
GaoZengqi
pgf...@gmail.com
zengqi...@gmail.com


0001-add-dependency-between-client-executes-and-static-li.patch
Description: Binary data