Re: meson and check-tests
Hi, On Sun, 2 Jun 2024 at 07:06, jian he wrote: > > On Sun, Jun 2, 2024 at 4:48 AM Tristan Partin wrote: > > > > On Fri May 31, 2024 at 12:02 PM CDT, Ashutosh Bapat wrote: > > > Hi Tristan, > > > Using make I can run only selected tests under src/test/regress using > > > TESTS=... make check-tests. I didn't find any way to do that with meson. > > > meson test --suite regress runs all the regression tests. > > > > > > We talked this off-list at the conference. It seems we have to somehow > > > avoid passing pg_regress --schedule argument and instead pass the list of > > > tests. Any idea how to do that? > > > > I think there are 2 solutions to this. > > > > 1. Avoid passing --schedule by default, which doesn't sound like a great > >solution. > > > > 2. Teach pg_regress to ignore the --schedule option if specific tests > >are passed instead. > > > > 3. Add a --no-schedule option to pg_regress which would override the > >previously added --schedule option. > > > > I personally prefer 2 or 3. > > > > 2: meson test -C build regress/regress --test-args my_specific_test > > 3: meson test -C build regress/regress --test-args "--no-schedule > > my_specific_test" > > > > Does anyone have an opinion? > > > > if each individual sql file can be tested separately, will > `test: test_setup`? > be aslo executed as a prerequisite? Yes, it is required to run at least two setup tests because regress tests require a database to be created: 1- The initdb executable is needed, and it is installed by the 'tmp_install' test for the tests. 2- Although the initdb executable exists, it is not enough by itself. Regress tests copies its database contents from the template directory, instead of running initdb for each test [1] (This is the default behaviour in the meson builds' regress tests). This template directory is created by the 'initdb_cache' test. [1] 252dcb3239 -- Regards, Nazir Bilal Yavuz Microsoft
walsender.c comment with no context is hard to understand
Hi, I was looking at this code comment and wondered what it meant. AFAICT over time code has been moved around causing comments to lose their original context, so now it is hard to understand what they are saying. ~~~ After a 2017 patch [1] the code in walsender.c function logical_read_xlog_page() looked like this: /* make sure we have enough WAL available */ flushptr = WalSndWaitForWal(targetPagePtr + reqLen); /* fail if not (implies we are going to shut down) */ if (flushptr < targetPagePtr + reqLen) return -1; ~~~ The same code in HEAD now looks like this: /* * Make sure we have enough WAL available before retrieving the current * timeline. This is needed to determine am_cascading_walsender accurately * which is needed to determine the current timeline. */ flushptr = WalSndWaitForWal(targetPagePtr + reqLen); /* * Since logical decoding is also permitted on a standby server, we need * to check if the server is in recovery to decide how to get the current * timeline ID (so that it also cover the promotion or timeline change * cases). */ am_cascading_walsender = RecoveryInProgress(); if (am_cascading_walsender) GetXLogReplayRecPtr(); else currTLI = GetWALInsertionTimeLine(); XLogReadDetermineTimeline(state, targetPagePtr, reqLen, currTLI); sendTimeLineIsHistoric = (state->currTLI != currTLI); sendTimeLine = state->currTLI; sendTimeLineValidUpto = state->currTLIValidUntil; sendTimeLineNextTLI = state->nextTLI; /* fail if not (implies we are going to shut down) */ if (flushptr < targetPagePtr + reqLen) return -1; ~~~ Notice how the "fail if not" comment has become distantly separated from the flushptr assignment it was once adjacent to, so that comment hardly makes sense anymore -- e.g. "fail if not" WHAT? Perhaps the comment should say something like it used to: /* Fail if there is not enough WAL available. This can happen during shutdown. */ == [1] https://github.com/postgres/postgres/commit/fca85f8ef157d4d58dea1fdc8e1f1f957b74ee78 Kind Regards, Peter Smith. Fujitsu Australia
Re: cannot drop intarray extension
On Mon, Jun 3, 2024 at 12:14 PM jian he wrote: > > hi. > > setup > drop table if exist test__int cascade; > create extension intarray; > > CREATE TABLE test__int( a int[] ); > CREATE INDEX text_idx on test__int using gist (a gist__intbig_ops(siglen = > 1)); > drop extension intarray cascade; > NOTICE: drop cascades to index text_idx > 2024-06-03 11:53:32.629 CST [41165] ERROR: cache lookup failed for > function 17758 > 2024-06-03 11:53:32.629 CST [41165] STATEMENT: drop extension intarray > cascade; > ERROR: cache lookup failed for function 17758 > > extension (ltree, pg_trgm) also have the same problem. drop table if exists t2 cascade; CREATE EXTENSION ltree; CREATE TABLE t2 (t ltree); create index tstidx on t2 using gist (t gist_ltree_ops(siglen=4)); drop extension ltree cascade; drop table if exists t3 cascade; CREATE EXTENSION pg_trgm; CREATE TABLE t3(t text COLLATE "C"); create index trgm_idx on t3 using gist (t gist_trgm_ops(siglen=1)); drop extension pg_trgm cascade; > extension hstore work as expected, no error. drop table if exists t1 cascade; create extension hstore; CREATE TABLE t1 (h hstore); create index hidx on t1 using gist(h gist_hstore_ops(siglen=1)); drop extension hstore cascade; on the master branch. i didn't test on other branches.
Re: pgsql: Add more SQL/JSON constructor functions
Peter Eisentraut writes: > On 29.05.24 18:44, Tom Lane wrote: >> Yeah, I too think this is a cast, and truncation is the spec-defined >> behavior for casting to varchar with a specific length limit. > The SQL standard says essentially that the output of json_serialize() is > some string that when parsed back in gives you an equivalent JSON value > as the input. That doesn't seem compatible with truncating the output. Maybe you should take this up with the SQL committee? If you don't like our current behavior, then either you have to say that RETURNING with a length-limited target type is illegal (which is problematic for the spec, since they have no such type) or that the cast behaves like an implicit cast, with errors for overlength input (which I find to be an unintuitive definition for a construct that names the target type explicitly). > If you want output truncation, you can of course use an actual cast. > But it makes sense that the RETURNING clause is separate from that. Are you trying to say that the RETURNING clause's specified type isn't the actual output type? I can't buy that either. Again, if you think our existing behavior isn't right, I think it's a problem for the SQL committee not us. regards, tom lane
Re: cannot drop intarray extension
Hi Jian On Mon, Jun 3, 2024 at 9:14 AM jian he wrote: > hi. > > setup > drop table if exist test__int cascade; > create extension intarray; > > CREATE TABLE test__int( a int[] ); > CREATE INDEX text_idx on test__int using gist (a gist__intbig_ops(siglen = > 1)); > drop extension intarray cascade; > NOTICE: drop cascades to index text_idx > 2024-06-03 11:53:32.629 CST [41165] ERROR: cache lookup failed for > function 17758 > Its a bug. > 2024-06-03 11:53:32.629 CST [41165] STATEMENT: drop extension intarray > cascade; > ERROR: cache lookup failed for function 17758 > > > backtrace info: > index_getprocinfo > #0 index_opclass_options (indrel=0x7faeca727b58, attnum=1, > attoptions=94372901674408, validate=false) > at > ../../Desktop/pg_src/src4/postgres/src/backend/access/index/indexam.c:1034 > #1 0x55d4e63a79cb in RelationGetIndexAttOptions > (relation=0x7faeca727b58, copy=false) > at > ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:5872 > #2 0x55d4e639d72d in RelationInitIndexAccessInfo > (relation=0x7faeca727b58) > at > ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:1569 > #3 0x55d4e639c5ac in RelationBuildDesc (targetRelId=24582, > insertIt=true) > at > ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:1207 > #4 0x55d4e639e9ce in RelationIdGetRelation (relationId=24582) > at > ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:2115 > #5 0x55d4e5a412fd in relation_open (relationId=24582, lockmode=8) > at > ../../Desktop/pg_src/src4/postgres/src/backend/access/common/relation.c:58 > #6 0x55d4e5ae6a06 in index_open (relationId=24582, lockmode=8) > at > ../../Desktop/pg_src/src4/postgres/src/backend/access/index/indexam.c:137 > #7 0x55d4e5be61b8 in index_drop (indexId=24582, concurrent=false, > concurrent_lock_mode=false) > at ../../Desktop/pg_src/src4/postgres/src/backend/catalog/index.c:2156 > > i guess it's because we first dropped the function g_intbig_options > then later we need it. > > >
cannot drop intarray extension
hi. setup drop table if exist test__int cascade; create extension intarray; CREATE TABLE test__int( a int[] ); CREATE INDEX text_idx on test__int using gist (a gist__intbig_ops(siglen = 1)); drop extension intarray cascade; NOTICE: drop cascades to index text_idx 2024-06-03 11:53:32.629 CST [41165] ERROR: cache lookup failed for function 17758 2024-06-03 11:53:32.629 CST [41165] STATEMENT: drop extension intarray cascade; ERROR: cache lookup failed for function 17758 backtrace info: index_getprocinfo #0 index_opclass_options (indrel=0x7faeca727b58, attnum=1, attoptions=94372901674408, validate=false) at ../../Desktop/pg_src/src4/postgres/src/backend/access/index/indexam.c:1034 #1 0x55d4e63a79cb in RelationGetIndexAttOptions (relation=0x7faeca727b58, copy=false) at ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:5872 #2 0x55d4e639d72d in RelationInitIndexAccessInfo (relation=0x7faeca727b58) at ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:1569 #3 0x55d4e639c5ac in RelationBuildDesc (targetRelId=24582, insertIt=true) at ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:1207 #4 0x55d4e639e9ce in RelationIdGetRelation (relationId=24582) at ../../Desktop/pg_src/src4/postgres/src/backend/utils/cache/relcache.c:2115 #5 0x55d4e5a412fd in relation_open (relationId=24582, lockmode=8) at ../../Desktop/pg_src/src4/postgres/src/backend/access/common/relation.c:58 #6 0x55d4e5ae6a06 in index_open (relationId=24582, lockmode=8) at ../../Desktop/pg_src/src4/postgres/src/backend/access/index/indexam.c:137 #7 0x55d4e5be61b8 in index_drop (indexId=24582, concurrent=false, concurrent_lock_mode=false) at ../../Desktop/pg_src/src4/postgres/src/backend/catalog/index.c:2156 i guess it's because we first dropped the function g_intbig_options then later we need it.
Re: Improve the connection failure error messages
Hi, just by visual inspection of the v4/v5 patch diffs, the latest v5 LGTM. == Kind Regards, Peter Smith. Fujitsu Australia
Re: Fix overflow hazard in interval rounding
Hi Andres, Sorry for such a late reply. On Tue, Feb 13, 2024 at 2:14 PM Andres Freund wrote: > Random, mildly related thought: I wonder if it's time to, again, look at > enabling -ftrapv in assert enabled builds.I had looked at that a few years > back, and fixed a number of instances, but not all I think. But I think we are > a lot closer to avoiding signed overflows everywhere, and it'd be nice to find > overflow hazards more easily. I agree that this would be very helpful. > Many places are broken even with -fwrapv > semantics (which we don't have on all compilers!). Trapping on such overflows > makes it far easier to find problems with tools like sqlsmith. Does this mean that some of our existing tests will panic when compiled with -ftrapv or -fwrapv? If so I'd be interested in resolving the remaining issues if you could point me in the right direction of how to set the flag. Thanks, Joe Koshakow
Re: pltcl crashes due to a syntax error
Erik Wienhold writes: > Tcl_GetVar returns null if errorInfo does not exist. Omitting econtext > from errcontext in that case looks like the proper fix to me. Yeah, that was the conclusion I came to last night while sitting in the airport, but I didn't have time to prepare a cleaned-up patch. The new bit of information that this bug report provides is that it's possible to get a TCL_ERROR result without Tcl having set errorInfo. That seems a tad odd, and it must happen only in weird corner cases, else we'd have heard of this decades ago. Not sure if it's worth trying to characterize those cases further, however. > Or just do away with throw_tcl_error and call ereport directly. I'd say this adds to the importance of having throw_tcl_error, because now it's even more complex than before, and there are multiple call sites. > Compare > that to pltcl_trigger_handler where the same case is handled like this: Hm, I wonder why that's not using throw_tcl_error. I guess because it wants to give its own primary message, but still ... regards, tom lane
Re: Schema variables - new implementation for Postgres 15
On 25.05.24 12:50, Pavel Stehule wrote: It looks odd - It is not intuitive, it introduces new inconsistency inside Postgres, or with solutions in other databases. No other database has a similar rule, so users coming from Oracle, Db2, or MSSQL, Firebird will be confused. Users that use PL/pgSQL will be confused. Do you have a description of what those other systems do? Maybe you posted it already earlier?
Re: Switch background worker on/off in runtime.
On 31.05.24 11:28, ISHAN CHHANGANI . wrote: Is it possible to switch on/off a background worker in runtime? worker.bgw_flags =BGWORKER_SHMEM_ACCESS; worker.bgw_start_time =BgWorkerStart_PostmasterStart; I want to switch off the worker based on some flag value, etc, either from the main process or the worker itself. Are there any already existing examples? I think this depends more on your exact use case. For example, the logical replication background workers have sophisticated logic to do this. There is a launcher, which is itself a background worker, which launches other per-subscription workers. And there are commands to disable subscriptions, which would among other things stop their corresponding background workers. That logic is specific to the needs of the logic replication system. You might get some ideas from that. But in general it will need a bit of custom logic.
Re: pgsql: Add more SQL/JSON constructor functions
On 29.05.24 18:44, Tom Lane wrote:
Amit Langote writes:
On Mon, May 27, 2024 at 7:10 PM Alvaro Herrera wrote:
On 2024-May-27, Alvaro Herrera wrote:
I just noticed this behavior, which looks like a bug to me:
select json_serialize('{"a":1, "a":2}' returning varchar(5));
json_serialize
{"a":
I think this function should throw an error if the destination type
doesn't have room for the output json. Otherwise, what good is the
serialization function?
This behavior comes from using COERCE_EXPLICIT_CAST when creating the
coercion expression to convert json_*() functions' argument to the
RETURNING type.
Yeah, I too think this is a cast, and truncation is the spec-defined
behavior for casting to varchar with a specific length limit. I see
little reason that this should work differently from
select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
json_serialize
{"a":
(1 row)
The SQL standard says essentially that the output of json_serialize() is
some string that when parsed back in gives you an equivalent JSON value
as the input. That doesn't seem compatible with truncating the output.
If you want output truncation, you can of course use an actual cast.
But it makes sense that the RETURNING clause is separate from that.
Re: Avoid an odd undefined behavior with memcmp (src/bin/pg_rewind/pg_rewind.c)
Em qua., 29 de mai. de 2024 às 22:41, Long Song escreveu: > > Hi Ranier, > > > > > IMO, I think that pg_rewind can have a security issue, > > if two files are exactly the same, they are considered different. > > Because use of structs with padding values is unspecified. > Logically you are right. But I don't understand what scenario > would require memcmp to compare ControlFileData. > In general, we read ControlFileData from a pg_control file > and then use members of ControlFileData directly. > So the two ControlFileData are not directly compared by byte. > Actually in pg_rewind there is a comparison using memcmp. > > > Fix by explicitly initializing with memset to avoid this. > And, even if there are scenarios that use memcmp comparisons, > your modifications are not complete. > There are three calls to the digestControlFile in the main() > of pg_rewind.c, and as your said(if right), these should do > memory initialization every time. > In fact, initializing structures with memset does not solve anything. Once the entire structure is populated again by a call to memcpy shortly thereafter. My concern now is that when the structure is saved to disk, what are the padding fields like? But enough noise. Thanks for taking a look. best regards, Ranier Vilela
Re: Fix possible dereference null pointer (PQprint)
Em sex., 31 de mai. de 2024 às 05:03, Daniel Gustafsson
escreveu:
> > On 27 May 2024, at 16:52, Ranier Vilela wrote:
>
> > In the function *PQprint*, the variable po->fieldName can be NULL.
>
> Yes.
>
> > See the checks a few lines up.
>
> Indeed, let's check it.
>
> for (numFieldName = 0;
> po->fieldName && po->fieldName[numFieldName];
> numFieldName++)
> ;
> for (j = 0; j < nFields; j++)
> {
> int len;
> const char *s = (j < numFieldName && po->fieldName[j][0]) ?
> po->fieldName[j] : PQfname(res, j);
>
> If po->fieldName is NULL then numFieldName won't be incremented and will
> remain
> zero. In the check you reference we check (j < numFieldName) which will
> check
> the j in the range 0..nFields for being less than zero. The code thus does
> seem quite correct to me.
>
You are completely correct. My bad.
Thank you Daniel.
best regards,
Ranier Vilela
Re: pltcl crashes due to a syntax error
On 2024-06-02 14:32 +0200, Pierre Forstmann wrote:
> I understand that Tcl_GetVar should not be used any more and should be
> replaced by Tcl_GetStringResult
> (but I know nothing about Tcl internals)
>
> Following patch :
> diff postgres/src/pl/tcl/pltcl.c.orig postgres/src/pl/tcl/pltcl.c
> 1373c1373,1376
> < econtext = utf_u2e(Tcl_GetVar(interp, "errorInfo",
> TCL_GLOBAL_ONLY));
> ---
> > /*
> >* econtext = utf_u2e(Tcl_GetVar(interp, "errorInfo",
> TCL_GLOBAL_ONLY));
> >*/
> > econtext = utf_u2e(Tcl_GetStringResult(interp));
>
> gives:
>
> pierre=# CREATE OR REPLACE PROCEDURE test_proc(INOUT a text)
> AS $$
> set aa [concat $1 "+" $1]
> return [list $aa $aa])
> $$
> LANGUAGE pltcl;
> CREATE PROCEDURE
> pierre=# CALL test_proc('abc');
> 2024-06-02 14:22:45.223 CEST [61649] ERROR: list element in braces
> followed by ")" instead of space
> 2024-06-02 14:22:45.223 CEST [61649] CONTEXT: list element in braces
> followed by ")" instead of space
> in PL/Tcl function "test_proc"
> 2024-06-02 14:22:45.223 CEST [61649] STATEMENT: CALL test_proc('abc');
> ERROR: list element in braces followed by ")" instead of space
> CONTEXT: list element in braces followed by ")" instead of space
> in PL/Tcl function "test_proc"
Tcl_GetStringResult is already used for emsg. Setting econtext to same
string is rather pointless. The problem is that Tcl_ListObjGetElements
does not set errorInfo if conversion fails. From the manpage:
"If listPtr is not already a list value, Tcl_ListObjGetElements will
attempt to convert it to one; if the conversion fails, it returns
TCL_ERROR and leaves an error message in the interpreter's result value
if interp is not NULL."
Tcl_GetVar returns null if errorInfo does not exist. Omitting econtext
from errcontext in that case looks like the proper fix to me.
Or just do away with throw_tcl_error and call ereport directly. Compare
that to pltcl_trigger_handler where the same case is handled like this:
/
* Otherwise, the return value should be a column name/value list
* specifying the modified tuple to return.
/
if (Tcl_ListObjGetElements(interp, Tcl_GetObjResult(interp),
_Objc, _Objv)
!= TCL_OK)
ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
errmsg("could not split return value from trigger: %s",
utf_u2e(Tcl_GetStringResult(interp);
--
Erik
Re: The xversion-upgrade test fails to stop server
02.06.2024 21:39, Andrew Dunstan wrote: Maybe uploading this log file too would help to understand what is the cause of the failure... Yeah, I'll fix that. Thank you, Andrew! Could you also take a look at: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sungazer=2024-04-21%2014%3A09%3A53 This log contains: test sto_using_select ... FAILED 27556 ms but I can't see ../snapshot_too_old/output_iso/regression.diff and .../snapshot_too_old/output_iso/log/postmaster.log in the log. Best regards, Alexander
Re: meson and check-tests
On 2024-06-02 Su 01:25, Tom Lane wrote: "Tristan Partin" writes: On Fri May 31, 2024 at 12:02 PM CDT, Ashutosh Bapat wrote: We talked this off-list at the conference. It seems we have to somehow avoid passing pg_regress --schedule argument and instead pass the list of tests. Any idea how to do that? I think there are 2 solutions to this. 1. Avoid passing --schedule by default, which doesn't sound like a great solution. 2. Teach pg_regress to ignore the --schedule option if specific tests are passed instead. 3. Add a --no-schedule option to pg_regress which would override the previously added --schedule option. I personally prefer 2 or 3. Just to refresh peoples' memory of what the Makefiles do: src/test/regress/GNUmakefile has check: all $(pg_regress_check) $(REGRESS_OPTS) --schedule=$(srcdir)/parallel_schedule $(MAXCONNOPT) $(EXTRA_TESTS) check-tests: all | temp-install $(pg_regress_check) $(REGRESS_OPTS) $(MAXCONNOPT) $(TESTS) $(EXTRA_TESTS) (and parallel cases for installcheck etc). AFAICS, meson.build has no equivalent to the EXTRA_TESTS add-on, nor does it have behavior equivalent to check-tests' substitution of $(TESTS) for --schedule. But I suggest that those behaviors have stood for a long time and so the appropriate thing to do is duplicate them as best we can, not invent something different. +1 cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Optimizing COPY with SIMD
Inspired by David Rowley's work [1] on optimizing JSON escape processing with SIMD, I noticed that the COPY code could potentially benefit from SIMD instructions in a few places, eg: (1) CopyAttributeOutCSV() has 2 byte-by-byte loops (2) CopyAttributeOutText() has 1 (3) CopyReadLineText() has 1 (4) CopyReadAttributesCSV() has 1 (5) CopyReadAttributesText() has 1 Attached is a quick POC patch that uses SIMD instructions for case (1) above. For sufficiently large attribute values, this is a significant performance win. For small fields, performance looks to be about the same. Results on an M1 Macbook Pro. == neilconway=# select count(*), avg(length(a))::int, avg(length(b))::int, avg(length(c))::int from short_strings; count | avg | avg | avg +-+-+- 524288 | 8 | 8 | 8 (1 row) neilconway=# select count(*), avg(length(a))::int, avg(length(b))::int, avg(length(c))::int from long_strings; count | avg | avg | avg ---+-+-+- 65536 | 657 | 657 | 657 (1 row) master @ 8fea1bd541: $ for i in ~/*.sql; do hyperfine --warmup 5 "./psql -f $i"; done Benchmark 1: ./psql -f /Users/neilconway/copy-out-bench-long-quotes.sql Time (mean ± σ): 2.027 s ± 0.075 s[User: 0.001 s, System: 0.000 s] Range (min … max):1.928 s … 2.207 s10 runs Benchmark 1: ./psql -f /Users/neilconway/copy-out-bench-long.sql Time (mean ± σ): 1.420 s ± 0.027 s[User: 0.001 s, System: 0.000 s] Range (min … max):1.379 s … 1.473 s10 runs Benchmark 1: ./psql -f /Users/neilconway/copy-out-bench-short.sql Time (mean ± σ): 546.0 ms ± 9.6 ms[User: 1.4 ms, System: 0.3 ms] Range (min … max): 539.0 ms … 572.1 ms10 runs master + SIMD patch: $ for i in ~/*.sql; do hyperfine --warmup 5 "./psql -f $i"; done Benchmark 1: ./psql -f /Users/neilconway/copy-out-bench-long-quotes.sql Time (mean ± σ): 797.8 ms ± 19.4 ms[User: 0.9 ms, System: 0.0 ms] Range (min … max): 770.0 ms … 828.5 ms10 runs Benchmark 1: ./psql -f /Users/neilconway/copy-out-bench-long.sql Time (mean ± σ): 732.3 ms ± 20.8 ms[User: 1.2 ms, System: 0.0 ms] Range (min … max): 701.1 ms … 763.5 ms10 runs Benchmark 1: ./psql -f /Users/neilconway/copy-out-bench-short.sql Time (mean ± σ): 545.7 ms ± 13.5 ms[User: 1.3 ms, System: 0.1 ms] Range (min … max): 533.6 ms … 580.2 ms10 runs == Implementation-wise, it seems complex to use SIMD when encoding_embeds_ascii is true (which should be uncommon). In principle, we could probably still use SIMD here, but it would require juggling between the SIMD chunk size and sizes returned by pg_encoding_mblen(). For now, the POC patch falls back to the old code path when encoding_embeds_ascii is true. Any feedback would be very welcome. Cheers, Neil [1] https://www.postgresql.org/message-id/caaphdvplxwmzvbckcdgfu9xqjgcdm7tfprdtxub9pvgpnuy...@mail.gmail.com 0002-Optimize-COPY-TO-.-FORMAT-CSV-using-SIMD-instruction.patch Description: Binary data 0001-Remove-inaccurate-comment.patch Description: Binary data copy-out-bench-short.sql Description: Binary data copy-out-bench-long.sql Description: Binary data copy-out-bench-long-quotes.sql Description: Binary data
Re: The xversion-upgrade test fails to stop server
On 2024-05-30 Th 11:00, Alexander Lakhin wrote: Hello Andrew, While reviewing recent buildfarm failures, I came across this one: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake=2024-05-23%2004%3A11%3A03 upgrade.crake/REL_16_STABLE/REL9_5_STABLE-ctl4.log waiting for server to shut down... failed pg_ctl: server does not shut down Looking at: https://github.com/PGBuildFarm/client-code/blob/05014d50e/PGBuild/Modules/TestUpgradeXversion.pm#L641 I see that ctl4.log is created after updating extensions and REL9_5_STABLE-update_extensions.log contains: You are now connected to database "contrib_regression_redis_fdw" as user "buildfarm". ALTER EXTENSION "hstore" UPDATE; ALTER EXTENSION You are now connected to database "contrib_regression_btree_gin" as user "buildfarm". ALTER EXTENSION "btree_gin" UPDATE; ALTER EXTENSION ... but I see no corresponding server log file containing these commands in the failure log. When running the same test locally, I find these in inst/upgrade_log. Maybe uploading this log file too would help to understand what is the cause of the failure... Yeah, I'll fix that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: POC: GROUP BY optimization
Hi! On Sun, Jun 2, 2024 at 10:55 AM jian he wrote: > > On Fri, May 31, 2024 at 8:12 AM Alexander Korotkov > wrote: > > > > I've revised some grammar including the sentence you've proposed. > > > > -static List *groupclause_apply_groupingset(PlannerInfo *root, List *force); > +static List *preprocess_groupclause(PlannerInfo *root, List *force); > > changing preprocess_groupclause the second argument > from "force" to "gset" would be more intuitive, I think. Probably, but my intention is to restore preprocess_groupclause() as it was before 0452b461bc with minimal edits to support incremental sort. I'd rather avoid refactoring if this area for now. > `elog(ERROR, "Order of group-by clauses doesn't correspond incoming > sort order");` > > I think this error message makes people wonder what "incoming sort order" is. > BTW, "correspond", generally people use "correspond to". Thank you. On the second thought, I think it would be better to turn this into an assertion like the checks before. > I did some minor cosmetic changes, mainly changing foreach to foreach_node. > Please check the attachment. I would avoid refactoring of preprocess_groupclause() for the reason described above. But I picked the grammar fix for PlannerInfo's comment. -- Regards, Alexander Korotkov Supabase v4-0003-Rename-PathKeyInfo-to-GroupByOrdering.patch Description: Binary data v4-0004-Restore-preprocess_groupclause.patch Description: Binary data v4-0005-Teach-group_keys_reorder_by_pathkeys-about-redund.patch Description: Binary data v4-0001-Fix-asymmetry-in-setting-EquivalenceClass.ec_sort.patch Description: Binary data v4-0002-Add-invariants-check-to-get_useful_group_keys_ord.patch Description: Binary data
Re: pltcl crashes due to a syntax error
I can also reproduce in Alma Linux 8.10 with tcl-devel 8.6.8
gdb says:
(gdb) p interp
$1 = (Tcl_Interp *) 0x288a500
(gdb) p *interp
$2 = {resultDontUse = 0x288a6d8 "", freeProcDontUse = 0x0,
errorLineDontUse = 1}
(gdb) p msg
No symbol "msg" in current context.
(gdb) p emsg
$3 = 0x27ebe48 "list element in braces followed by \")\" instead of space"
Involved PG source code is:
**
* throw_tcl_error - ereport an error returned from the Tcl interpreter
**/
static void
throw_tcl_error
(Tcl_Interp
*interp, const char *proname
)
{
/*
* Caution is needed here because Tcl_GetVar could overwrite the
* interpreter result (even though it's not really supposed to), and we
* can't control the order of evaluation of ereport arguments. Hence, make
* real sure we have our own copy of the result string before invoking
* Tcl_GetVar.
*/
char *emsg;
char *econtext;
emsg = pstrdup
(utf_u2e
(Tcl_GetStringResult(interp)));
econtext = utf_u2e
(Tcl_GetVar(interp,
"errorInfo", TCL_GLOBAL_ONLY));
ereport
(ERROR
,
(errcode
(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
errmsg
("%s", emsg),
errcontext
("%s\nin PL/Tcl function \"%s\"",
econtext, proname
)));
}
I understand that Tcl_GetVar should not be used any more and should be
replaced by Tcl_GetStringResult
(but I know nothing about Tcl internals)
Following patch :
diff postgres/src/pl/tcl/pltcl.c.orig postgres/src/pl/tcl/pltcl.c
1373c1373,1376
< econtext = utf_u2e(Tcl_GetVar(interp, "errorInfo",
TCL_GLOBAL_ONLY));
---
> /*
>* econtext = utf_u2e(Tcl_GetVar(interp, "errorInfo",
TCL_GLOBAL_ONLY));
>*/
> econtext = utf_u2e(Tcl_GetStringResult(interp));
gives:
pierre=# CREATE OR REPLACE PROCEDURE test_proc(INOUT a text)
AS $$
set aa [concat $1 "+" $1]
return [list $aa $aa])
$$
LANGUAGE pltcl;
CREATE PROCEDURE
pierre=# CALL test_proc('abc');
2024-06-02 14:22:45.223 CEST [61649] ERROR: list element in braces
followed by ")" instead of space
2024-06-02 14:22:45.223 CEST [61649] CONTEXT: list element in braces
followed by ")" instead of space
in PL/Tcl function "test_proc"
2024-06-02 14:22:45.223 CEST [61649] STATEMENT: CALL test_proc('abc');
ERROR: list element in braces followed by ")" instead of space
CONTEXT: list element in braces followed by ")" instead of space
in PL/Tcl function "test_proc"
PF
Le sam. 1 juin 2024 à 06:36, a.kozhemyakin a
écrit :
> Hello hackers,
>
> When executing the following query on master branch:
>
> CREATE EXTENSION pltcl;
> CREATE or replace PROCEDURE test_proc5(INOUT a text)
> LANGUAGE pltcl
> AS $$
> set aa [concat $1 "+" $1]
> return [list $aa $aa])
> $$;
>
> CALL test_proc5('abc');
> CREATE EXTENSION
> CREATE PROCEDURE
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>
> The connection to the server was lost. Attempting reset: Failed.
>
>
> Core was generated by `postgres: postgres postgres [loca'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0 __strlen_sse2 () at ../sysdeps/x86_64/multiarch/strlen-sse2.S:142
> 142 ../sysdeps/x86_64/multiarch/strlen-sse2.S: No such file or
> directory.
> (gdb) bt
> #0 __strlen_sse2 () at ../sysdeps/x86_64/multiarch/strlen-sse2.S:142
> #1 0x7f5f1353ba6a in utf_u2e (src=0x0) at pltcl.c:77
> #2 0x7f5f1353c9f7 in throw_tcl_error (interp=0x55ec24bdaf70,
> proname=0x55ec24b6b140 "test_proc5") at pltcl.c:1373
> #3 0x7f5f1353ed64 in pltcl_func_handler
> (fcinfo=fcinfo@entry=0x7ffdbfb407a0,
> call_state=call_state@entry=0x7ffdbfb405d0,
> pltrusted=pltrusted@entry=true) at pltcl.c:1029
> #4 0x7f5f1353ee8d in pltcl_handler (fcinfo=0x7ffdbfb407a0,
> pltrusted=pltrusted@entry=true) at pltcl.c:765
> #5 0x7f5f1353f1ef in pltcl_call_handler (fcinfo=) at
> pltcl.c:698
>
Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
Hi, At some point this patch switched from rdtsc to rdtscp, which imo largely negates the point of it. What lead to that? Greetings, Andres Freund
Re: POC: GROUP BY optimization
On Fri, May 31, 2024 at 8:12 AM Alexander Korotkov wrote:
>
> I've revised some grammar including the sentence you've proposed.
>
-static List *groupclause_apply_groupingset(PlannerInfo *root, List *force);
+static List *preprocess_groupclause(PlannerInfo *root, List *force);
changing preprocess_groupclause the second argument
from "force" to "gset" would be more intuitive, I think.
`elog(ERROR, "Order of group-by clauses doesn't correspond incoming
sort order");`
I think this error message makes people wonder what "incoming sort order" is.
BTW, "correspond", generally people use "correspond to".
I did some minor cosmetic changes, mainly changing foreach to foreach_node.
Please check the attachment.
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index d6a1fb8e..801d9f6d 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -2852,15 +2852,13 @@ preprocess_groupclause(PlannerInfo *root, List *force)
{
Query *parse = root->parse;
List *new_groupclause = NIL;
- ListCell *sl;
- ListCell *gl;
+ ListCell *gl;
/* For grouping sets, we need to force the ordering */
- if (force)
+ if (force != NIL)
{
- foreach(sl, force)
+ foreach_int(ref, force)
{
- Index ref = lfirst_int(sl);
SortGroupClause *cl = get_sortgroupref_clause(ref, parse->groupClause);
new_groupclause = lappend(new_groupclause, cl);
@@ -2879,10 +2877,8 @@ preprocess_groupclause(PlannerInfo *root, List *force)
*
* This code assumes that the sortClause contains no duplicate items.
*/
- foreach(sl, parse->sortClause)
+ foreach_node(SortGroupClause, sc, parse->sortClause)
{
- SortGroupClause *sc = lfirst_node(SortGroupClause, sl);
-
foreach(gl, parse->groupClause)
{
SortGroupClause *gc = lfirst_node(SortGroupClause, gl);
@@ -2908,10 +2904,8 @@ preprocess_groupclause(PlannerInfo *root, List *force)
* implemented using incremental sort. Also, give up if there are any
* non-sortable GROUP BY items, since then there's no hope anyway.
*/
- foreach(gl, parse->groupClause)
+ foreach_node(SortGroupClause,gc, parse->groupClause)
{
- SortGroupClause *gc = lfirst_node(SortGroupClause, gl);
-
if (list_member_ptr(new_groupclause, gc))
continue; /* it matched an ORDER BY item */
if (!OidIsValid(gc->sortop)) /* give up, GROUP BY can't be sorted */
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index af8de421..2ba297c1 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -426,7 +426,7 @@ struct PlannerInfo
* items to be proven redundant, implying that there is only one group
* containing all the query's rows. Hence, if you want to check whether
* GROUP BY was specified, test for nonempty parse->groupClause, not for
- * nonempty processed_groupClause. Optimiser chooses specific order of
+ * nonempty processed_groupClause. Optimizer chooses specific order of
* group-by clauses during the upper paths generation process, attempting
* to use different strategies to minimize number of sorts or engage
* incremental sort. See preprocess_groupclause() and
