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
Re: percentile value check can be slow
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
Re: percentile value check can be slow
On 19.11.2017 13:23, 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. 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. 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. Best Regards Johannes
Re: [HACKERS] Bug in to_timestamp().
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: [HACKERS] [PATCH] Generic type subscripting
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 writes: > On Sat, Nov 18, 2017 at 05:48:26PM -0500, Tom Lane wrote: >> ... 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. > Only queries with FX field throw an error. Ah, I see --- I'd missed that FX was relevant to this. Yeah, maybe it'd be okay to tighten up in that case, since that's making it act more like Oracle, which seems to be generally agreed to be a good thing. I don't much like the error message that you've got: +SELECT to_timestamp('97/Feb/16', 'FXYY:Mon:DD'); +ERROR: unexpected character "/", expected ":" +DETAIL: The given value did not match any of the allowed values for this field. The DETAIL message seems to have been copied-and-pasted into a context where it's not terribly accurate. I'd consider replacing it with something along the lines of "HINT: In FX mode, punctuation in the input string must exactly match the format string." This way the report will contain a pretty clear statement of the new rule that was broken. (BTW, it's a hint not a detail because it might be guessing wrong as to what the real problem is.) regards, tom lane
Re: percentile value check can be slow
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. Johannes, could you help us understand your thinking in reporting this? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: no library dependency in Makefile?
=?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. > 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
Re: Patch to add dependency between client executes and static libraries
=?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] [WIP] Zipfian distribution in pgbench
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: [HACKERS] [PATCH] Improve geometric types
> 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: percentile value check can be slow
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] Incremental sort
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, &isnullA); + datumB = slot_getattr(b, attno, &isnullB); + + /* Special case for NULL-vs-NULL, else use standard comparison */ + if (isnullA || isnullB) + { + if (isnullA == isnullB) + continue; + else + return false; + } + + key = &node->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(&key->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: [HACKERS] [PATCH] A hook for session start
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] A hook for session start
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] INSERT ON CONFLICT and partitioned tables
On 2 August 2017 at 00:56, Amit Langote wrote: > The patch's job is simple: Patch no longer applies and has some strange behaviours. > - Remove the check in the parser that causes an error the moment the > ON CONFLICT clause is found. Where is the check and test that blocks ON CONFLICT UPDATE? My understanding was the patch was intended to only remove the error in case of DO NOTHING. > - Fix leaf partition ResultRelInfo initialization code so that the call > ExecOpenIndices() specifies 'true' for speculative, so that the > information necessary for conflict checking will be initialized in the > leaf partition's ResultRelInfo Why? There is no caller that needs information. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH] A hook for session start
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] Custom compression methods
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] Custom compression methods
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: Speed up the removal of WAL files
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] GnuTLS support
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: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
>> 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&data=02%7C01%7Cbachow%40microsoft.com%7Ce37b69223 >> a144d1e5b7408d52c6d8171%7C72f988bf86f141af91ab2d7cd011db47%7C1 >> %7C0%7C636463771208784375&sdata=1%2FDylQIfS2rI2RwIVyZnDCUbzRQJe >> V4YM8J496QkpiQ%3D&reserved=0 >> The Enterprise PostgreSQL Company
Re: [HACKERS] CUBE seems a bit confused about ORDER BY
On 10/30/2017 03:04 PM, Alexander Korotkov wrote: > On Sun, Oct 29, 2017 at 1:30 AM, Alexander Korotkov > mailto:a.korot...@postgrespro.ru>> 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] GnuTLS support
On Mon, Nov 20, 2017 at 9:42 AM, Tomas Vondra wrote: > I don't want to be the annoying guy, but this patch no longer applies > due to 642bafa0c5f9f08d106a14f31429e0e0c718b603 touching the tests :-( Sorry about that. Something more specific needs to happen here as well for channel binding support with SCRAM. CheckSCRAMAuth() now assumes that the channel binding mechanism SCRAM-SHA-256-PLUS can be published to the client if SSL is used, because OpenSSL is the only implementation available. Does gnutls include an API to allow an application to fetch the bytes from the TLS finished message? I can see some references by glancing at the tarball of gnutls 3.6.1, but you would need something similar to OpenSSL's SSL_get_peer_finished(). If that cannot be achieved I think that it will be necessary to tweak auth.c to not publish the -PLUS mechanism if for example the result of be_tls_get_peer_finished() is NULL. No need for a new SSL-specific API. At the end it would prove to be more portable to do so for all the SSL implementations, MacOS stuff does not document an API to get the TLS finish message bytes. > 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) WIth potential patches coming to use macos' SSL implementation or Windows channel, there should really be only one implementation available at compile time. That's more simple as a first step as well. So +1 for the --with-ssl switch. -- Michael
Re: [HACKERS] [PATCH] A hook for session start
On Mon, Nov 20, 2017 at 7:56 AM, Andrew Dunstan wrote: > 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. I think that this is close to the concept of BackendType in pgstat.h. > Yes, agreed, The above is pretty ugly and likely to be quite fragile. 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. -- Michael
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Mon, Aug 21, 2017 at 9:46 AM, Michael Paquier wrote: > On Mon, Aug 21, 2017 at 6:21 AM, Daniel Gustafsson wrote: >> I think the intended use case of the GUC should drive the decision on >> fallback. >> If the GUC isn’t supposed to be a way to figure out if the server was built >> with SSL support, then not existing in non-SSL backends is fine. If, >> however, >> we want to allow using the GUC to see if the server has SSL support, then >> there >> needs to be a “None” or similar value for that case. > > Only GUCs related to debugging have their existence defined based on a > #define, so it seems to me that if Postgres is compiled without any > SSL support, this parameter should still be visible, but set to > "none". The last set of patches available here does not apply: https://www.postgresql.org/message-id/b5e2b87d-3e8a-4597-9a7f-8489b3b67...@yesql.se The SSL test refactoring is one cause. I think as well that this is crashing when attempting to use SCRAM authentication with the SSL brand of macos and SCRAM's channel binding. I am going to send a patch which allows handling of no support for channel bindings for a given SSL implementation, something needed as well by the gnutls patch. Please make sure that you define at least be_tls_get_peer_finished() and pgtls_get_finished() with a NULL result and a length of 0 as return results as, as far as I can see, macos does not give direct access to the TLS finish message bytes. At least that's not documented. -- Michael
Re: [HACKERS] [PATCH] A hook for session start
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] [PATCH] A hook for session start
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
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
Allowing SSL connection of v11 client to v10 server with SCRAM channel binding
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(&p, '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: 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
Re: Allowing SSL connection of v11 client to v10 server with SCRAM channel binding
Michael Paquier writes: > When trying to connect to a v11 client based on current HEAD to a v10 > server using SSL, then the connection would fail. That's bad ... > 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. This is not an acceptable fix. We have to maintain the ability to connect to unpatched older servers. If features added to HEAD have broken that, either we fix those features to be backwards compatible, or we revert them. regards, tom lane
Re: Allowing SSL connection of v11 client to v10 server with SCRAM channel binding
(Adding Heikki here because that concerns him as well) On Mon, Nov 20, 2017 at 2:04 PM, Tom Lane wrote: > Michael Paquier writes: >> 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. > > This is not an acceptable fix. We have to maintain the ability to connect > to unpatched older servers. If features added to HEAD have broken that, > either we fix those features to be backwards compatible, or we revert > them. Well, when doing the SASL exchange the client does not know what is the backend version. If we'd know that it would then be possible to enforce a "n" flag conditionally from the client. So in order to be backward-compatible we could send unconditionally a "n" flag from a v11 client even in an SSL context. But that would not actually be true because the client is able to support channel binding in this case, so we would make libpq not RFC-compliant. v10 has not been out for a long time, so this can plead in favor of a change, and SCRAM is not spread yet. It is really unfortunate that we did not notice that during at the moment SCRAM has been implemented. That's clearly a bug of v10. Honestly I am of the opinion to do a proper fix now and have things in a clean state which is RFC-compliant instead of maintaining for 10 years a backward-compatible change in libpq that would be only valid for 10.0 and 10.1 (assuming that the server-side fix is added in 10.2). Note that we need to fix the v10 server anyway with something like the patch upthread. The enforcement of this "n" flag can just happen in a v11-client. -- Michael
Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.
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, &endtli); > > 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: [HACKERS] [WIP] Zipfian distribution in pgbench
Hello Alik, Patch applies with "patch", but not with "git apply", probably because it is in CR-NL eol format. No big deal. Documentation has been switched from SGML to XML, so now tags must be explicitely closed, i.e. use x instead of x. Patch compiles with a warning: pgbench.c: In function ‘getZipfianRand’: pgbench.c:905:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] Declaration must move before assertion. The random_zipfian(1,100,2) call result is not tested. "1 times" -> "once". "array overflow on \random_zipfian" remove spurious "\". -- Fabien.
Re: [HACKERS] Proposal: generic WAL compression
Antonin Houska wrote: > Oleg Ivanov wrote: > > > In order to overcome that issue, I would like to propose the patch, which > > provides possibility to use another approach of the WAL record > > construction. > > After having spent several hours reviewing this patch I dare to send the > following comments: One more idea: I think the metadata (ALIGN_GAP) should be stored separate from the actual data so that you can use memcpy() instead of this loop: while (i < j) { charc = targetRegionAligned[i++]; writeToPtr(ptr, c); } -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at
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
Re: percentile value check can be slow
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
Re: percentile value check can be slow
On 19.11.2017 13:23, 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. 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. 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. Best Regards Johannes
Re: [HACKERS] Bug in to_timestamp().
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: [HACKERS] [PATCH] Generic type subscripting
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 writes: > On Sat, Nov 18, 2017 at 05:48:26PM -0500, Tom Lane wrote: >> ... 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. > Only queries with FX field throw an error. Ah, I see --- I'd missed that FX was relevant to this. Yeah, maybe it'd be okay to tighten up in that case, since that's making it act more like Oracle, which seems to be generally agreed to be a good thing. I don't much like the error message that you've got: +SELECT to_timestamp('97/Feb/16', 'FXYY:Mon:DD'); +ERROR: unexpected character "/", expected ":" +DETAIL: The given value did not match any of the allowed values for this field. The DETAIL message seems to have been copied-and-pasted into a context where it's not terribly accurate. I'd consider replacing it with something along the lines of "HINT: In FX mode, punctuation in the input string must exactly match the format string." This way the report will contain a pretty clear statement of the new rule that was broken. (BTW, it's a hint not a detail because it might be guessing wrong as to what the real problem is.) regards, tom lane
Re: percentile value check can be slow
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. Johannes, could you help us understand your thinking in reporting this? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: no library dependency in Makefile?
=?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. > 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
Re: Patch to add dependency between client executes and static libraries
=?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] [WIP] Zipfian distribution in pgbench
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: [HACKERS] [PATCH] Improve geometric types
> 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: percentile value check can be slow
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] Incremental sort
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, &isnullA); + datumB = slot_getattr(b, attno, &isnullB); + + /* Special case for NULL-vs-NULL, else use standard comparison */ + if (isnullA || isnullB) + { + if (isnullA == isnullB) + continue; + else + return false; + } + + key = &node->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(&key->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: [HACKERS] [PATCH] A hook for session start
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] A hook for session start
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] INSERT ON CONFLICT and partitioned tables
On 2 August 2017 at 00:56, Amit Langote wrote: > The patch's job is simple: Patch no longer applies and has some strange behaviours. > - Remove the check in the parser that causes an error the moment the > ON CONFLICT clause is found. Where is the check and test that blocks ON CONFLICT UPDATE? My understanding was the patch was intended to only remove the error in case of DO NOTHING. > - Fix leaf partition ResultRelInfo initialization code so that the call > ExecOpenIndices() specifies 'true' for speculative, so that the > information necessary for conflict checking will be initialized in the > leaf partition's ResultRelInfo Why? There is no caller that needs information. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH] A hook for session start
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] Custom compression methods
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] Custom compression methods
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: Speed up the removal of WAL files
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] GnuTLS support
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: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
>> 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&data=02%7C01%7Cbachow%40microsoft.com%7Ce37b69223 >> a144d1e5b7408d52c6d8171%7C72f988bf86f141af91ab2d7cd011db47%7C1 >> %7C0%7C636463771208784375&sdata=1%2FDylQIfS2rI2RwIVyZnDCUbzRQJe >> V4YM8J496QkpiQ%3D&reserved=0 >> The Enterprise PostgreSQL Company
Re: [HACKERS] CUBE seems a bit confused about ORDER BY
On 10/30/2017 03:04 PM, Alexander Korotkov wrote: > On Sun, Oct 29, 2017 at 1:30 AM, Alexander Korotkov > mailto:a.korot...@postgrespro.ru>> 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] GnuTLS support
On Mon, Nov 20, 2017 at 9:42 AM, Tomas Vondra wrote: > I don't want to be the annoying guy, but this patch no longer applies > due to 642bafa0c5f9f08d106a14f31429e0e0c718b603 touching the tests :-( Sorry about that. Something more specific needs to happen here as well for channel binding support with SCRAM. CheckSCRAMAuth() now assumes that the channel binding mechanism SCRAM-SHA-256-PLUS can be published to the client if SSL is used, because OpenSSL is the only implementation available. Does gnutls include an API to allow an application to fetch the bytes from the TLS finished message? I can see some references by glancing at the tarball of gnutls 3.6.1, but you would need something similar to OpenSSL's SSL_get_peer_finished(). If that cannot be achieved I think that it will be necessary to tweak auth.c to not publish the -PLUS mechanism if for example the result of be_tls_get_peer_finished() is NULL. No need for a new SSL-specific API. At the end it would prove to be more portable to do so for all the SSL implementations, MacOS stuff does not document an API to get the TLS finish message bytes. > 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) WIth potential patches coming to use macos' SSL implementation or Windows channel, there should really be only one implementation available at compile time. That's more simple as a first step as well. So +1 for the --with-ssl switch. -- Michael
Re: [HACKERS] [PATCH] A hook for session start
On Mon, Nov 20, 2017 at 7:56 AM, Andrew Dunstan wrote: > 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. I think that this is close to the concept of BackendType in pgstat.h. > Yes, agreed, The above is pretty ugly and likely to be quite fragile. 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. -- Michael
Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
On Mon, Aug 21, 2017 at 9:46 AM, Michael Paquier wrote: > On Mon, Aug 21, 2017 at 6:21 AM, Daniel Gustafsson wrote: >> I think the intended use case of the GUC should drive the decision on >> fallback. >> If the GUC isn’t supposed to be a way to figure out if the server was built >> with SSL support, then not existing in non-SSL backends is fine. If, >> however, >> we want to allow using the GUC to see if the server has SSL support, then >> there >> needs to be a “None” or similar value for that case. > > Only GUCs related to debugging have their existence defined based on a > #define, so it seems to me that if Postgres is compiled without any > SSL support, this parameter should still be visible, but set to > "none". The last set of patches available here does not apply: https://www.postgresql.org/message-id/b5e2b87d-3e8a-4597-9a7f-8489b3b67...@yesql.se The SSL test refactoring is one cause. I think as well that this is crashing when attempting to use SCRAM authentication with the SSL brand of macos and SCRAM's channel binding. I am going to send a patch which allows handling of no support for channel bindings for a given SSL implementation, something needed as well by the gnutls patch. Please make sure that you define at least be_tls_get_peer_finished() and pgtls_get_finished() with a NULL result and a length of 0 as return results as, as far as I can see, macos does not give direct access to the TLS finish message bytes. At least that's not documented. -- Michael
Re: [HACKERS] [PATCH] A hook for session start
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] [PATCH] A hook for session start
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
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
Allowing SSL connection of v11 client to v10 server with SCRAM channel binding
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(&p, '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: 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
Re: Allowing SSL connection of v11 client to v10 server with SCRAM channel binding
Michael Paquier writes: > When trying to connect to a v11 client based on current HEAD to a v10 > server using SSL, then the connection would fail. That's bad ... > 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. This is not an acceptable fix. We have to maintain the ability to connect to unpatched older servers. If features added to HEAD have broken that, either we fix those features to be backwards compatible, or we revert them. regards, tom lane
Re: Allowing SSL connection of v11 client to v10 server with SCRAM channel binding
(Adding Heikki here because that concerns him as well) On Mon, Nov 20, 2017 at 2:04 PM, Tom Lane wrote: > Michael Paquier writes: >> 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. > > This is not an acceptable fix. We have to maintain the ability to connect > to unpatched older servers. If features added to HEAD have broken that, > either we fix those features to be backwards compatible, or we revert > them. Well, when doing the SASL exchange the client does not know what is the backend version. If we'd know that it would then be possible to enforce a "n" flag conditionally from the client. So in order to be backward-compatible we could send unconditionally a "n" flag from a v11 client even in an SSL context. But that would not actually be true because the client is able to support channel binding in this case, so we would make libpq not RFC-compliant. v10 has not been out for a long time, so this can plead in favor of a change, and SCRAM is not spread yet. It is really unfortunate that we did not notice that during at the moment SCRAM has been implemented. That's clearly a bug of v10. Honestly I am of the opinion to do a proper fix now and have things in a clean state which is RFC-compliant instead of maintaining for 10 years a backward-compatible change in libpq that would be only valid for 10.0 and 10.1 (assuming that the server-side fix is added in 10.2). Note that we need to fix the v10 server anyway with something like the patch upthread. The enforcement of this "n" flag can just happen in a v11-client. -- Michael
Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.
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, &endtli); > > 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: [HACKERS] [WIP] Zipfian distribution in pgbench
Hello Alik, Patch applies with "patch", but not with "git apply", probably because it is in CR-NL eol format. No big deal. Documentation has been switched from SGML to XML, so now tags must be explicitely closed, i.e. use x instead of x. Patch compiles with a warning: pgbench.c: In function ‘getZipfianRand’: pgbench.c:905:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] Declaration must move before assertion. The random_zipfian(1,100,2) call result is not tested. "1 times" -> "once". "array overflow on \random_zipfian" remove spurious "\". -- Fabien.
Re: [HACKERS] Proposal: generic WAL compression
Antonin Houska wrote: > Oleg Ivanov wrote: > > > In order to overcome that issue, I would like to propose the patch, which > > provides possibility to use another approach of the WAL record > > construction. > > After having spent several hours reviewing this patch I dare to send the > following comments: One more idea: I think the metadata (ALIGN_GAP) should be stored separate from the actual data so that you can use memcpy() instead of this loop: while (i < j) { charc = targetRegionAligned[i++]; writeToPtr(ptr, c); } -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at