Re: [HACKERS] pgbench regression test failure
Fabien COELHO writes: >> [...] After another week of buildfarm runs, we have a few more cases of >> 3 rows of output, and none of more than 3 or less than 1. So I went >> ahead and pushed your patch. I'm still suspicious of these results, but >> we might as well try to make the buildfarm green pending investigation >> of how this is happening. > Yep. I keep the issue of pgbench tap test determinism in my todo list, > among other things. > I think that it should be at least clearer under which condition (load ? > luck ? bug ?) the result may be 1 or 3 when 2 are expected, which needs > some thinking. skink blew up real good just now: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2017-09-23%2010%3A50%3A01 the critical bit being # Failed test 'pgbench progress stderr /(?^:progress: 1\b)/' # at /home/andres/build/buildfarm/HEAD/pgsql.build/../pgsql/src/test/perl/TestLib.pm line 369. # 'starting vacuum...end. # progress: 2.6 s, 6.9 tps, lat 0.000 ms stddev 0.000, lag 0.000 ms, 18 skipped # progress: 3.0 s, 0.0 tps, lat -nan ms stddev -nan, lag -nan ms, 0 skipped # progress: 4.0 s, 1.0 tps, lat 2682.730 ms stddev 0.000, lag 985.509 ms, 0 skipped # ' # doesn't match '(?^:progress: 1\b)' # Failed test 'transaction count for 001_pgbench_log_1.15981 (5)' # at t/001_pgbench_with_server.pl line 438. # Failed test 'transaction count for 001_pgbench_log_1.15981.1 (4)' # at t/001_pgbench_with_server.pl line 438. # Looks like you failed 3 tests of 233. That's exceeded my patience with this test case, so I've removed it for the moment. We can put it back as soon as we figure some way to make it more robust. (BTW, the "-nan" bits suggest an actual pgbench bug, independently of anything else.) Possibly you can duplicate skink's issue by running things under valgrind. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] user-defined numeric data types triggering ERROR: unsupported type
Tomas Vondra writes: >>> [ scalarineqsel may fall over when used by extension operators ] > What about using two-pronged approach: > 1) fall back to mid bucket in back branches (9.3 - 10) > 2) do something smarter (along the lines you outlined) in PG11 Sure. We need to test the fallback case anyway. >> [ sketch of a more extensible design ] > Sounds reasonable to me, I guess - I can't really think about anything > simpler giving us the same flexibility. Actually, on further thought, that's still too simple. If you look at convert_string_to_scalar() you'll see it's examining all three values concurrently (the probe value, of one datatype, and two bin boundary values of possibly a different type). The reason is that it's stripping off whatever common prefix those strings have before trying to form a numeric equivalent. While certainly convert_string_to_scalar is pretty stupid in the face of non-ASCII sort orders, the prefix-stripping is something I really don't want to give up. So the design I sketched of considering each value totally independently isn't good enough. We could, perhaps, provide an API that lets an operator estimation function replace convert_to_scalar in toto, but that seems like you'd still end up duplicating code in many cases. Not sure about how to find a happy medium. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?
Andrew Dunstan writes: > On 09/22/2017 11:19 PM, Tom Lane wrote: >> Yeah, I was considering the same thing over dinner, though I'd phrase >> it oppositely: keep a list of enum type OIDs created in the current >> transaction, so that we could whitelist them. This could maybe become >> a problem if someone created a zillion enums in one xact, though. > I see what you're saying, but my idea was slightly different. We would > only add to the hashtable I had in mind at the bottom AddEnumLabel(). > Any other value, whether added in the current transaction or not, should > be safe, AIUI. Oh, I see: a list of enum values we need to blacklist, not whitelist. Yes, that's a significantly better idea than mine, because in common use-cases that would be empty or have a very small number of entries. In particular that fixes the "pg_restore -1" scenario, because no matter how many enums you're restoring, pg_dump doesn't use ALTER TYPE ADD VALUE. (Well, it does in --binary-upgrade mode, but those scripts are run in transaction-per-statement mode so it's fine.) > Maybe we should also keep a cache of whitelisted enums > created in the current transaction. What for? Wouldn't be any faster to search, in fact likely slower because it could get large in common use-cases. >> The immediate question is do we care to design/implement such a thing >> post-RC1. I'd have to vote "no". I think the most prudent thing to >> do is revert 15bc038f9 and then have another go at it during the v11 >> cycle. > Sadly I agree. We've made decisions like this in the past, and I have > generally been supportive of them. I think this is the first time I have > been on the receiving end of one so late in the process :-( Unless you want to try writing a patch for this in the next day or two, I think we have to do that. But now that I see the plan clearly, maybe we could get away with a post-RC1 fix. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?
Robert Haas writes: > Peter, with respect, it's time to let this argument go. We're > scheduled to wrap a GA release in just over 72 hours. FWIW, the release is a week from Monday, not Monday. (Or if it is Monday, somebody else is wrapping it.) We have some other embarrassingly critical things to fix, like bug #14825, so I can certainly sympathize with an argument that there's not enough committer bandwidth left to deal with this; but not with an argument that it's too late to change behavior period. The big concern I have here is that this feels a lot like something that we'll regret at leisure, if it's not right in the first release. I'd much rather be restrictive in v10 and then loosen the rules later, than be lax in v10 and then have to argue about whether to break backwards compatibility in order to gain saner behavior. > We have never canonicalized collations before and therefore it is not > essential that we do that now. Actually, we try; see initdb.c's check_locale_name(). It's not our fault that setlocale(3) fails to play along on many platforms. > I simply do not buy the theory that this cannot be changed later. OK, so you're promising not to whine when we break backwards compatibility on this point in v11? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?
Andrew Dunstan writes: > On 09/22/2017 05:46 PM, Tom Lane wrote: >> I'm not sure if that qualifies as a stop-ship problem, but it ain't >> good, for sure. We need to look at whether we should revert 15bc038f9 >> or somehow revise its rules. > I wonder if we wouldn't be better > doing this more directly, keeping a per-transaction hash of unsafe enum > values (which will almost always be empty). It might even speed up the > check. Yeah, I was considering the same thing over dinner, though I'd phrase it oppositely: keep a list of enum type OIDs created in the current transaction, so that we could whitelist them. This could maybe become a problem if someone created a zillion enums in one xact, though. The immediate question is do we care to design/implement such a thing post-RC1. I'd have to vote "no". I think the most prudent thing to do is revert 15bc038f9 and then have another go at it during the v11 cycle. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?
bal...@obiserver.hu writes:
> PostgreSQL version: 10beta4
> testdb=# begin;
> BEGIN
> testdb=# create type enumtype as enum ('v1', 'v2');
> CREATE TYPE
> testdb=# alter type enumtype owner to testrole;
> ALTER TYPE
> testdb=# create table testtable (enumcolumn enumtype not null default 'v1');
> ERROR: unsafe use of new value "v1" of enum type enumtype
> HINT: New enum values must be committed before they can be used.
Hmm, that's pretty annoying. It's happening because check_safe_enum_use
insists that the enum's pg_type entry not be updated-in-transaction.
We thought that the new rules instituted by 15bc038f9 would be generally
more convenient to use than the old ones --- but this example shows
that, for example, pg_dump scripts involving enums often could not
be restored inside a single transaction.
I'm not sure if that qualifies as a stop-ship problem, but it ain't
good, for sure. We need to look at whether we should revert 15bc038f9
or somehow revise its rules.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Rethinking autovacuum.c memory handling
I notice that autovacuum.c calls autovacuum_do_vac_analyze, and
thereby vacuum(), in TopTransactionContext. This doesn't seem
like a terribly great idea, because it doesn't correspond to what
happens during a manually-invoked vacuum. TopTransactionContext
will go away when vacuum() commits the outer transaction, whereas
in non-autovac usage, we call vacuum() in a PortalHeapMemory
context that is not a child of TopTransactionContext and is not
at risk of being reset multiple times during the vacuum(). This'd
be a hazard if autovacuum_do_vac_analyze or vacuum did any palloc's
before getting to the main loop. More generally, I'm not aware of
other cases where we invoke a function in a context that we know
that function will destroy as it executes.
I don't see any live bug associated with this in HEAD, but this behavior
requires a rather ugly (and memory-leaking) workaround in the proposed
patch to allow multiple vacuum target rels.
What I think we should do instead is invoke autovacuum_do_vac_analyze
in the PortalContext that do_autovacuum has created, which we already
have a mechanism to reset once per table processed in do_autovacuum.
The attached patch does that, and also modifies perform_work_item()
to use the same approach. Right now perform_work_item() has a
copied-and-pasted MemoryContextResetAndDeleteChildren(PortalContext)
call in its error recovery path, but that seems a bit out of place
given that perform_work_item() isn't using PortalContext otherwise.
Comments, objections?
regards, tom lane
PS: I was disappointed to find out that perform_work_item() isn't
exercised at all in the standard regression tests.
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index b745d89..1a32433 100644
*** a/src/backend/postmaster/autovacuum.c
--- b/src/backend/postmaster/autovacuum.c
*** do_autovacuum(void)
*** 2444,2451
*/
PG_TRY();
{
/* have at it */
- MemoryContextSwitchTo(TopTransactionContext);
autovacuum_do_vac_analyze(tab, bstrategy);
/*
--- 2444,2453
*/
PG_TRY();
{
+ /* Use PortalContext for any per-table allocations */
+ MemoryContextSwitchTo(PortalContext);
+
/* have at it */
autovacuum_do_vac_analyze(tab, bstrategy);
/*
*** do_autovacuum(void)
*** 2482,2487
--- 2484,2492
}
PG_END_TRY();
+ /* Make sure we're back in AutovacMemCxt */
+ MemoryContextSwitchTo(AutovacMemCxt);
+
did_vacuum = true;
/* the PGXACT flags are reset at the next end of transaction */
*** perform_work_item(AutoVacuumWorkItem *wo
*** 2614,2619
--- 2619,2627
autovac_report_workitem(workitem, cur_nspname, cur_datname);
+ /* clean up memory before each work item */
+ MemoryContextResetAndDeleteChildren(PortalContext);
+
/*
* We will abort the current work item if something errors out, and
* continue with the next one; in particular, this happens if we are
*** perform_work_item(AutoVacuumWorkItem *wo
*** 2622,2630
*/
PG_TRY();
{
! /* have at it */
! MemoryContextSwitchTo(TopTransactionContext);
switch (workitem->avw_type)
{
case AVW_BRINSummarizeRange:
--- 2630,2639
*/
PG_TRY();
{
! /* Use PortalContext for any per-work-item allocations */
! MemoryContextSwitchTo(PortalContext);
+ /* have at it */
switch (workitem->avw_type)
{
case AVW_BRINSummarizeRange:
*** perform_work_item(AutoVacuumWorkItem *wo
*** 2668,2673
--- 2677,2685
}
PG_END_TRY();
+ /* Make sure we're back in AutovacMemCxt */
+ MemoryContextSwitchTo(AutovacMemCxt);
+
/* We intentionally do not set did_vacuum here */
/* be tidy */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Shaky coding for vacuuming partitioned relations
Somebody inserted this into vacuum.c's get_rel_oids(): tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for relation %u", relid); apparently without having read the very verbose comment two lines above, which points out that we're not taking any lock on the target relation. So, if that relation is concurrently being dropped, you're likely to get "cache lookup failed for relation " rather than anything more user-friendly. A minimum-change fix would be to replace the elog() with an ereport that produces the same "relation does not exist" error you'd have gotten from RangeVarGetRelid, had the concurrent DROP TABLE committed a few microseconds earlier. But that feels like its's band-aiding around the problem. What I'm wondering about is changing the RangeVarGetRelid call to take ShareUpdateExclusiveLock rather than no lock. That would protect the syscache lookup, and it would also make the find_all_inheritors call a lot more meaningful. If we're doing a VACUUM, the ShareUpdateExclusiveLock would be dropped as soon as we close the caller's transaction, and then we'd acquire the same or stronger lock inside vacuum_rel(). So that seems fine. If we're doing an ANALYZE, then the lock would continue to be held and analyze_rel would merely be acquiring it an extra time, so we'd actually be removing a race-condition failure scenario for ANALYZE. This would mean a few more cycles in lock management, but since this only applies to a manual VACUUM or ANALYZE that specifies a table name, I'm not too concerned about that. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgbench regression test failure
Fabien COELHO writes: >> It could be as simple as putting the check-for-done at the bottom of the >> loop not the top, perhaps. > I agree that it is best if tests should work in all reasonable conditions, > including a somehow overloaded host... > I'm going to think about it, but I'm not sure of the best approach. In the > mean time, ISTM that the issue has not been encountered (yet), so this is > not a pressing issue. Maybe under -T > --aggregate-interval pgbench could > go on over the limit if the log file has not been written at all, but that > would be some kind of kludge for this specific test... After another week of buildfarm runs, we have a few more cases of 3 rows of output, and none of more than 3 or less than 1. So I went ahead and pushed your patch. I'm still suspicious of these results, but we might as well try to make the buildfarm green pending investigation of how this is happening. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] !USE_WIDE_UPPER_LOWER compile errors in v10+
Peter Eisentraut writes:
> On 9/21/17 17:38, Tom Lane wrote:
>> Meanwhile, I see that Peter has posted a fix for the immediate problem.
>> I propose that Peter should apply his fix in HEAD and v10, and then
> done
>> I'll rip out the !USE_WIDE_UPPER_LOWER code paths in HEAD only.
And done.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix bool/int type confusion
Peter Eisentraut writes: > On 9/21/17 14:58, Tom Lane wrote: >> I've just been going through their git commit log to see what else has >> changed since tzcode2017b, and I note that there are half a dozen other >> portability-ish fixes. I think that some of them affect only code we >> don't use, but I'm not sure that that's the case for all. So I'm a bit >> inclined to go with plan B, that is sync with their current HEAD now. > I suppose it might be good to do this after 10.0 final is wrapped? I went and did it already. I'm not particularly worried about the impending 10.0 wrap --- the changes are minor as such things go, and we've generally not worried about giving previous tzcode syncs more than a few days' buildfarm testing before shipping them. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands
"Bossart, Nathan" writes: > [ 0001-error_on_duplicate_columns_in_analyze_v6.patch ] I've pushed (and back-patched) the 0001 patch, with some significant changes: * The list_concat_unique implementation is O(N^2), and seems a bit obscure as well. Perhaps there will never be so many column list entries that the speed is a big issue, but I'm not sure. I replaced the list with a bitmap to avoid the risk. * I did not see the point of having "ANALYZE t (x,x,x,nosuchcolumn)" complain about nosuchcolumn rather than the duplicated x entries, especially not if it meant you couldn't say which column was duplicated. So I folded the test into the primary loop and made the error look more like what you get for a nonexistent column. * I didn't like the test case at all: it was repetitive and it actually failed to exhibit the originally problematic behavior with unpatched code, which is generally a minimum expectation for a regression test. (I think that's because you used an empty table, for which ANALYZE will not try to make any pg_statistic entries.) So I rewrote that, and made it use an existing table rather than create a whole new one just for this. I'll take a look at the updated 0002 tomorrow, but if anyone living in a different timezone wants to review it meanwhile, feel free. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] !USE_WIDE_UPPER_LOWER compile errors in v10+
I wrote: > Noah Misch writes: >> Perhaps it is time to require HAVE_WCSTOMBS and HAVE_TOWLOWER, removing >> USE_WIDE_UPPER_LOWER? Every buildfarm fossil has both. > +1 ... if nothing else, there's the problem that untested code is likely > to be broken. You just proved it *is* broken, of course, but my point > is that even if we repaired the immediate damage we could have little > confidence in it staying fixed. Further notes about that: * The Single Unix Spec v2 (a/k/a POSIX 1997), which has been our minimum portability spec for quite awhile, requires wcstombs() and towlower(), and further requires the latter to be declared in . * Surveying the buildfarm, I agree with your conclusion that every active member has wcstombs() and towlower(). gaur/pademelon is the lone member that lacks ; it declares towlower() in instead. It's not so surprising that that system adheres to a pre-1997 idea of where to put that, because its /usr/include files mostly date from 1996 ... Meanwhile, I see that Peter has posted a fix for the immediate problem. I propose that Peter should apply his fix in HEAD and v10, and then I'll rip out the !USE_WIDE_UPPER_LOWER code paths in HEAD only. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Arrays of domains
I wrote:
> Here's a rebased-up-to-HEAD version of this patch set. The only
> actual change is removal of a no-longer-needed hunk in pl_exec.c.
I see the patch tester is complaining that this broke, due to commit
4bd199465. The fix is trivial (s/GETARG_ANY_ARRAY/GETARG_ANY_ARRAY_P/)
but for convenience here's an updated patch set.
regards, tom lane
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index 9d75e86..d7db32e 100644
*** a/src/backend/optimizer/prep/preptlist.c
--- b/src/backend/optimizer/prep/preptlist.c
*** expand_targetlist(List *tlist, int comma
*** 306,314
new_expr = coerce_to_domain(new_expr,
InvalidOid, -1,
atttype,
COERCE_IMPLICIT_CAST,
-1,
- false,
false);
}
else
--- 306,314
new_expr = coerce_to_domain(new_expr,
InvalidOid, -1,
atttype,
+ COERCION_IMPLICIT,
COERCE_IMPLICIT_CAST,
-1,
false);
}
else
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index e79ad26..5a241bd 100644
*** a/src/backend/parser/parse_coerce.c
--- b/src/backend/parser/parse_coerce.c
***
*** 34,48
static Node *coerce_type_typmod(Node *node,
Oid targetTypeId, int32 targetTypMod,
! CoercionForm cformat, int location,
! bool isExplicit, bool hideInputCoercion);
static void hide_coercion_node(Node *node);
static Node *build_coercion_expression(Node *node,
CoercionPathType pathtype,
Oid funcId,
Oid targetTypeId, int32 targetTypMod,
! CoercionForm cformat, int location,
! bool isExplicit);
static Node *coerce_record_to_complex(ParseState *pstate, Node *node,
Oid targetTypeId,
CoercionContext ccontext,
--- 34,49
static Node *coerce_type_typmod(Node *node,
Oid targetTypeId, int32 targetTypMod,
! CoercionContext ccontext, CoercionForm cformat,
! int location,
! bool hideInputCoercion);
static void hide_coercion_node(Node *node);
static Node *build_coercion_expression(Node *node,
CoercionPathType pathtype,
Oid funcId,
Oid targetTypeId, int32 targetTypMod,
! CoercionContext ccontext, CoercionForm cformat,
! int location);
static Node *coerce_record_to_complex(ParseState *pstate, Node *node,
Oid targetTypeId,
CoercionContext ccontext,
*** coerce_to_target_type(ParseState *pstate
*** 110,117
*/
result = coerce_type_typmod(result,
targettype, targettypmod,
! cformat, location,
! (cformat != COERCE_IMPLICIT_CAST),
(result != expr && !IsA(result, Const)));
if (expr != origexpr)
--- 111,117
*/
result = coerce_type_typmod(result,
targettype, targettypmod,
! ccontext, cformat, location,
(result != expr && !IsA(result, Const)));
if (expr != origexpr)
*** coerce_type(ParseState *pstate, Node *no
*** 355,361
result = coerce_to_domain(result,
baseTypeId, baseTypeMod,
targetTypeId,
! cformat, location, false, false);
ReleaseSysCache(baseType);
--- 355,362
result = coerce_to_domain(result,
baseTypeId, baseTypeMod,
targetTypeId,
! ccontext, cformat, location,
! false);
ReleaseSysCache(baseType);
*** coerce_type(ParseState *pstate, Node *no
*** 417,436
result = build_coercion_expression(node, pathtype, funcId,
baseTypeId, baseTypeMod,
! cformat, location,
! (cformat != COERCE_IMPLICIT_CAST));
/*
* If domain, coerce to the domain type and relabel with domain
! * type ID. We can skip the internal length-coercion step if the
! * selected coercion function was a type-and-length coercion.
*/
if (targetTypeId != baseTypeId)
result = coerce_to_domain(result, baseTypeId, baseTypeMod,
targetTypeId,
! cformat, location, true,
! exprIsLengthCoercion(result,
! NULL));
}
else
{
--- 418,434
result = build_coercion_expression(node, pathtype, funcId,
baseTypeId, baseTypeMod,
! ccontext, cformat, location);
/*
* If domain, coerce to the domain type and relabel with domain
! * type ID, hiding the previous coercion node.
*/
if (targetTypeId != baseTypeId)
result = coerce_to_domain(result, baseTypeId, baseTypeMod,
targetTypeId,
! ccontext, cformat, location,
! true);
}
else
{
*** coerce_type(ParseSta
Re: [HACKERS] [COMMITTERS] pgsql: Fix bool/int type confusion
Peter Eisentraut writes: > On 9/21/17 11:46, Tom Lane wrote: >> This also means that the portability problem is purely hypothetical; >> given valid tz reference data the code wouldn't ever try to increment >> "hit" to 2 anyway. It's even more hypothetical for us, because we don't >> use leap-second-aware zones. > It's not so much that there is an actual portability problem. The > problem is that the compiler issues a warning for this with stdbool. Understood. >> Therefore, what I would like to do is revert this commit (0ec2e908b), >> and then either leave the problem to be fixed by our next regular sync >> with a released tzcode version, or else force a sync with their git tip >> to absorb their fix now. In either case we should keep all our live >> branches in sync. I'm willing to do the code-sync work either way. > That makes sense. There is no urgency on the stdbool patch set, so > waiting for this to be resolved properly around November seems reasonable. I've just been going through their git commit log to see what else has changed since tzcode2017b, and I note that there are half a dozen other portability-ish fixes. I think that some of them affect only code we don't use, but I'm not sure that that's the case for all. So I'm a bit inclined to go with plan B, that is sync with their current HEAD now. As far as I've been able to tell, they don't have any special code QA process that they apply before a release. They push out releases based on the need to update the tzdata files, and the tzcode just rides along with that in whatever state it is in. So there's not really any reliability gain from waiting for an official code release --- it just is a bit easier to document what we synced to. That being the case, absorbing their changes in smaller chunks rather than bigger ones seems easier. I don't have the sync process totally automated, but it's not that painful as long as there are not too many changes at once. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Fix bool/int type confusion
Peter Eisentraut writes: > Fix bool/int type confusion > Using ++ on a bool variable doesn't work well when stdbool.h is in use. > The original BSD code appears to use int here, so use that instead. I'm fairly unhappy with this approach to fixing this problem, because localtime.c is not Postgres-maintained code; we try to keep it in sync with the upstream copy maintained by the IANA timezone crew. Patching it like this will interfere with the next sync attempt, and patching it only in master will cause back-patching of that sync patch to fail. I checked the tz list archives and discovered that this problem was already reported to them back in May: http://mm.icann.org/pipermail/tz/2017-May/024995.html Although indeed their previous coding had had the variable as "int", their response was not to change it back to int, but to get rid of the loop incrementing it, because that was intended to support the possibility of 2 leap seconds on the same day, which according to them is a widespread misunderstanding of the applicable standard. So the code in their git repo still has the variable as bool, but there's no ++ operator on it anymore. This also means that the portability problem is purely hypothetical; given valid tz reference data the code wouldn't ever try to increment "hit" to 2 anyway. It's even more hypothetical for us, because we don't use leap-second-aware zones. They've not made a new tzcode release since May, due to lack of political activity in this sphere this year, although I gather that one is likely by November or so. However, we have precedent for sometimes syncing with their git tip to absorb code bug fixes, cf commit 7afafe8af. Therefore, what I would like to do is revert this commit (0ec2e908b), and then either leave the problem to be fixed by our next regular sync with a released tzcode version, or else force a sync with their git tip to absorb their fix now. In either case we should keep all our live branches in sync. I'm willing to do the code-sync work either way. Comments? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] user-defined numeric data types triggering ERROR: unsupported type
Tomas Vondra writes: > [ scalarineqsel may fall over when used by extension operators ] I concur with your thought that we could have ineq_histogram_selectivity fall back to a "mid bucket" default if it's working with a datatype it is unable to convert_to_scalar. But I think if we're going to touch this at all, we ought to have higher ambition than that, and try to provide a mechanism whereby an extension that's willing to work a bit harder could get that additional increment of estimation accuracy. I don't care for this way to do that: > * Make convert_numeric_to_scalar smarter, so that it checks if there is > an implicit cast to numeric, and fail only if it does not find one. because it's expensive, and it only works for numeric-category cases, and it will fail outright for numbers outside the range of "double". (Notice that convert_numeric_to_scalar is *not* calling the regular cast function for numeric-to-double.) Moreover, any operator ought to know what types it can accept; we shouldn't have to do more catalog lookups to figure out what to do. Now that scalarltsel and friends are just trivial wrappers for a common function, we could imagine exposing scalarineqsel_wrapper as a non-static function, with more arguments (and perhaps a better-chosen name ;-)). The idea would be for extensions that want to go this extra mile to provide their own selectivity estimation functions, which again would just be trivial wrappers for the core function, but would provide additional knowledge through additional arguments. The additional arguments I'm envisioning are a couple of C function pointers, one function that knows how to convert the operator's left-hand input type to scalar, and one function that knows how to convert the right-hand type to scalar. (Identical APIs of course.) Passing a NULL would imply that the core code must fall back on its own devices for that input. Now the thing about convert_to_scalar is that there are several different conversion conventions already (numeric, string, timestamp, ...), and there probably could be more once extension types are coming to the party. So I'm imagining that the API for these conversion functions could be something like bool convert(Datum value, Oid valuetypeid, int *conversion_convention, double *converted_value); The conversion_convention output would make use of some agreed-on constants, say 1 for numeric, 2 for string, etc etc. In the core code, if either converter fails (returns false) or if they don't return the same conversion_convention code, we give up and use the mid-bucket default. If they both produce results with the same conversion_convention, then we can treat the converted_values as commensurable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] !USE_WIDE_UPPER_LOWER compile errors in v10+
Noah Misch writes: > Perhaps it is time to require HAVE_WCSTOMBS and HAVE_TOWLOWER, removing > USE_WIDE_UPPER_LOWER? Every buildfarm fossil has both. +1 ... if nothing else, there's the problem that untested code is likely to be broken. You just proved it *is* broken, of course, but my point is that even if we repaired the immediate damage we could have little confidence in it staying fixed. I think the USE_WIDE_UPPER_LOWER split was originally my code, so I'm willing to take care of removing it if there's consensus that that's what to do. I'm not sure that we need to treat this as a v10 open item, though. The premise of removing !USE_WIDE_UPPER_LOWER is that nobody cares anymore, therefore it shouldn't matter to users whether we remove it in v10. There's an argument that having only two states of the relevant code, not three, in the live back branches is worth something for maintenance --- but should that outweigh the risk of breaking something post-rc1? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Windows warnings from VS 2017
Andrew Dunstan writes:
> The speed of memset is hardly going to be the dominating factor in a
> 'CREATE DATABASE' command, so we could certainly afford to change to
> plain memset calls here.
Another thought is that it may be time for our decennial debate about
whether MemSet is worth the electrons it's printed on. I continue to
think that any modern compiler+libc ought to do an equivalent or better
optimization given just a plain memset(). If that seems to be true
for recent MSVC, we could consider putting an #if into c.h to define
MemSet as just memset for MSVC. Maybe later that could be extended
to other compilers.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands
Michael Paquier writes: > On Thu, Sep 21, 2017 at 9:08 AM, Tom Lane wrote: >> Um ... so? With Nathan's proposed behavior, there are two cases depending >> on just when the unexpected schema change happens: >> 1. *None* of the work gets done. >> 2. The work before the troublesome relation gets done, and the work after >> doesn't. > You may be missing one which is closer to what autovacuum does: > 3) Issue a warning for the troublesome relation, and get the work done > a maximum. Well, we could certainly discuss whether the behavior on detecting a conflict ought to be "error" or "warning and continue". But I do not buy the value of "it might be one or the other depending on timing". regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] compress method for spgist - 2
Alexander Korotkov writes: > On Thu, Sep 21, 2017 at 2:06 AM, Darafei "Komяpa" Praliaskouski < > m...@komzpa.net> wrote: >> What is rationale behind this circle? > I would prefer to rather forbid any geometries with infs and nans. > However, then upgrade process will suffer. User with such geometries would > get errors during dump/restore, pg_upgraded instances would still contain > invalid values... Yeah, that ship has sailed unfortunately. >> It seems to me that any circle with radius of any Infinity should become a >> [-Infinity .. Infinity, -Infinity .. Infinity] box.Then you won't have >> NaNs, and index structure shouldn't be broken. > We probably should produce [-Infinity .. Infinity, -Infinity .. Infinity] > box for any geometry containing inf or nan. Hm, we can do better in at least some cases, eg for a box ((0,1),(1,inf)) there's no reason to give up our knowledge of finite bounds for the other three boundaries. But certainly for a NaN circle radius what you suggest seems the most sensible thing to do. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands
Michael Paquier writes: > On Thu, Sep 21, 2017 at 8:18 AM, Tom Lane wrote: >> "Bossart, Nathan" writes: >>> I agree that the patch might be simpler without this, but the user-visible >>> behavior is the reason I had included it. In short, my goal was to avoid >>> errors halfway through a long-running VACUUM statement because the user >>> misspelled a relation/column name or the relation/column was dropped. >> I don't particularly buy that argument, because it's not the case that >> the preceding processing was wasted when that happens. We've done and >> committed the vacuuming work for the earlier relations. > I think that the problem can be seen differently though: the next > relations on the list would not be processed as well. For example in > parallel of a manual VACUUM triggered by a cron job, say that a rogue > admin removes a column for a relation to be VACUUM-ed. The relations > processed before the relation redefined would have been vacuumed and > the transaction doing the vacuum committed, but the ones listed after > would not have been updated in this nightly VACUUM. Um ... so? With Nathan's proposed behavior, there are two cases depending on just when the unexpected schema change happens: 1. *None* of the work gets done. 2. The work before the troublesome relation gets done, and the work after doesn't. I think it'll be much easier to understand if the behavior is always (2). And I don't see any particular advantage to (1) anyway, especially not for an unattended vacuum script. Keep in mind that there were not-entirely-unjustified complaints upthread about whether we needed to add any complexity here at all. I'd just as soon keep the added complexity to a minimum, especially when it's in service of behaviors that are not clearly improvements. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Windows warnings from VS 2017
Andrew Dunstan writes: > It's also warning that it will copy 16 bytes to a 13 byte structure at > lines 518, 1293 and 1294 of src/backend/commands/dbcommands.c. I haven't > seen any ill effects of this so far, but it seems to indicate that > something is possibly amiss on this compiler with the MemSet macros. That's weird. Is it too stupid to figure out that the if() inside MemSet evaluates to constant false in these calls? It seems hard to see how it would realize that the loop will write 16 bytes if it doesn't propagate the constant value forward. However ... on some other compilers, I've noticed that the compiler seems more likely to make "obvious" deductions of that sort if the variables in question are marked const. Does it help if you do - void *_vstart = (void *) (start); \ - int _val = (val); \ - Size_len = (len); \ + void * const _vstart = (void *) (start); \ + const int _val = (val); \ + const Size _len = (len); \ I don't think there's any strong reason not to just do s/MemSet/memset/ in these calls and nearby ones, but it would be good to understand just what's wrong here. And why it's only showing up in that file; seems nearly certain that we have similar coding elsewhere. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Windows warnings from VS 2017
Andrew Dunstan writes: > On 09/20/2017 06:13 PM, Michael Paquier wrote: >> Those are around for some time, see here: >> https://www.postgresql.org/message-id/CAB7nPqTkW=b_1jvvywd_g0wrkot+4ufqjggrv8osqbuzzxg...@mail.gmail.com >> But there has been no actual agreement about how to fix them.. > Oh. Missed that. > My solution was going to be slightly different. I was going to enclose > the #ifdef'd code in a bare block and move the rte declaration inside > that block. Of the various solutions proposed in the previous thread, I think the most salable alternative is probably ilmari's: get rid of the variable and write the assert like Assert(planner_rt_fetch(rel->relid, root)->rtekind == RTE_SUBQUERY); That's a pretty minimal change and it doesn't add any cycles to the non-Assert case. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] compress method for spgist - 2
=?UTF-8?Q?Darafei_=22Kom=D1=8Fpa=22_Praliaskouski?= writes:
> If it happens because NaN > Infinity, then the check should be not for
> isnan, but for if (low>high){swap(high, low)}.
Yeah, the same idea had occurred to me. It'd still need a comment, but
at least it's slightly more apparent what we're trying to ensure.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands
"Bossart, Nathan" writes: > On 9/20/17, 2:29 PM, "Tom Lane" wrote: >> I think that the way this ought to work is you process the VacuumRelation >> structs purely sequentially, each in its own transaction, so you don't >> need leaps of faith about what to do if a relation changes between the >> first time you look at it and when you actually come to process it. > How might this work for VACUUM statements with no tables specified? It > seems like we must either handle the case when the relation changes before > it is processed or hold a lock for the duration of the vacuuming. I don't see a need to change the way that vacuum-with-no-table operates. It essentially collects a list of relation OIDs that exist at the start of vacuuming, and then vacuums all the ones that still exist when their turn comes. Since no information beyond the bare OID is saved across the preceding transactions, there's not really any schema-change risk involved. We could possibly adapt that concept to the inheritance/partitioning cases for vacuum with a table name or list of names: when we first come to a VacuumRelation, collect a list of child table OIDs, and then process each one unless it's disappeared by the time its turn comes. But in any case, we should not be doing any checks on a particular relation until we've got it open and locked with intent to vacuum. >> The idea of "fast failing" because some later VacuumRelation struct might >> contain an error seems like useless complication, both in terms of the >> implementation and the user-visible behavior. > I agree that the patch might be simpler without this, but the user-visible > behavior is the reason I had included it. In short, my goal was to avoid > errors halfway through a long-running VACUUM statement because the user > misspelled a relation/column name or the relation/column was dropped. I don't particularly buy that argument, because it's not the case that the preceding processing was wasted when that happens. We've done and committed the vacuuming work for the earlier relations. > It's true that the tests become mostly pointless if the relations or > columns change before they are processed, but this seems like it would be > a rarer issue in typical use cases. Nonetheless, we'd have to explain this behavior to people, and I think it's mostly useless complication. With what I'm proposing, if vacuum complains about the third table in the list, you know it has done the ones before that. What what you want to do, maybe it did the ones before that, or maybe it didn't. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Varying results when using merge joins over postgres_fdw vs hash joins
Peter Eisentraut writes: > On 9/20/17 12:06, Tom Lane wrote: >> I'm tempted to propose that we invent some kind of "unknown" >> collation, which the planner would have to be taught to not equate to any >> other column collation (not even other instances of "unknown"), and that >> postgres_fdw's IMPORT ought to label remote columns with that collation >> unless specifically told to do otherwise. Then it's on the user's head >> if he tells us to do the wrong thing; but we won't produce incorrect >> plans by default. > OID 0 might already work that way, depending on the details. No, OID 0 means "column is not collatable". I'm pretty sure there are some asserts that will trip if we use that collation OID for a column of a collatable data type --- and even if there are not, I think conflating the two cases would be a bad idea. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] compress method for spgist - 2
Nikita Glukhov writes: > On 20.09.2017 23:19, Alexander Korotkov wrote: >> On Wed, Sep 20, 2017 at 11:07 PM, Tom Lane > <mailto:t...@sss.pgh.pa.us>> wrote: >>> Maybe I'm missing something, but it appears to me that it's >>> impossible for bbox->low.x to be NaN unless circle->center.x and/or >>> circle->radius is a NaN, in which case bbox->high.x would also have been >>> computed as a NaN, >>> making the swap entirely useless. > It is possible for bbox->low.x to be NaN when circle->center.x is and > circle->radius are both +Infinity. Without this float-order-preserving > swapping > one regression test for KNN with ORDER BY index will be totally broken > (you can > try it: https://github.com/glukhovn/postgres/tree/knn). If that's the reasoning, not having a comment explaining it is inexcusable. Do you really think people will understand what the code is doing? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PSA: don't be in a hurry to update to XCode 9.0
It seems to install some libraries that depend on
/usr/lib/system/libsystem_darwin.dylib, which doesn't exist.
Googling suggests it will be there in macOS 10.13,
but on Sierra or earlier, you're outta luck.
It looks like the only directly-affected PG dependency is
libxml; if you leave out --with-libxml you can still build.
I found this out the hard way on longfin's host, so I've
temporarily removed --with-libxml from that animal's
configuration to restore it to service. I trust I'll be
able to re-enable that after 10.13 comes out.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] compress method for spgist - 2
Darafei Praliaskouski writes:
> I have some questions about the circles example though.
> * What is the reason for isnan check and swap of box ordinates for circle?
> It wasn't in the code previously.
I hadn't paid any attention to this patch previously, but this comment
excited my curiosity, so I went and looked:
+ bbox->high.x = circle->center.x + circle->radius;
+ bbox->low.x = circle->center.x - circle->radius;
+ bbox->high.y = circle->center.y + circle->radius;
+ bbox->low.y = circle->center.y - circle->radius;
+
+ if (isnan(bbox->low.x))
+ {
+ double tmp = bbox->low.x;
+ bbox->low.x = bbox->high.x;
+ bbox->high.x = tmp;
+ }
Maybe I'm missing something, but it appears to me that it's impossible for
bbox->low.x to be NaN unless circle->center.x and/or circle->radius is a
NaN, in which case bbox->high.x would also have been computed as a NaN,
making the swap entirely useless. Likewise for the Y case. There may be
something useful to do about NaNs here, but this doesn't seem like it.
> How about removing circle from the scope of this patch, so it is smaller and
> cleaner?
Neither of those patches would be particularly large, and since they'd need
to touch adjacent code in some places, the diffs wouldn't be independent.
I think splitting them is just make-work.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands
"Bossart, Nathan" writes: > [ 0001-vacuum_multiple_tables_v18.patch ] I started to look at this, but soon became pretty desperately unhappy with the way that it makes a bunch of tests on the relations and then releases all locks on them. That either makes the tests pointless, or requires you to invent completely bizarre semantics, like this: * Check that all specified columns exist so that we can fast-fail * commands with multiple tables. If the column disappears before we * actually process it, we will emit a WARNING and skip it later on. I think that the way this ought to work is you process the VacuumRelation structs purely sequentially, each in its own transaction, so you don't need leaps of faith about what to do if a relation changes between the first time you look at it and when you actually come to process it. The idea of "fast failing" because some later VacuumRelation struct might contain an error seems like useless complication, both in terms of the implementation and the user-visible behavior. It looks like some of this stuff might be the fault of the partitioned-tables patch more than your own changes, but no time like the present to try to improve matters. As for the other patch, I wonder if it might not be better to silently ignore duplicate column names. But in either case, we don't need a pre-check, IMO. I'd just leave it to the lookup loop in do_analyze_rel to notice if it's looked up the same column number twice. That would be more likely to lead to a back-patchable fix, too. 0002 as it stands is useless as a back-patch basis, since it proposes modifying code that doesn't even exist in the back branches. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Varying results when using merge joins over postgres_fdw vs hash joins
Peter Geoghegan writes: > I would like postgres_fdw to be taught about collation versioning, so > that postgres_fdw's IMPORT could automatically do the right thing when > ICU is in use. Maybe it's too early to discuss that, because we don't > even support alternative collation provider collations as the > database/cluster default collation just yet. FWIW, postgres_fdw + > collations was one of the issues that made me believe in the long term > strategic importance of ICU. TBH, the more I learn about ICU, the less faith I have in the proposition that it's going to fix anything at all for us in this area. It seems to be just about as squishy as glibc in terms of locale identification, if not worse. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Varying results when using merge joins over postgres_fdw vs hash joins
Corey Huinker writes:
> We had difficulty finding the place in the code were LC_COLLATE gets
> recombobulated into a recognized collation.
That's because it isn't. The DB's default collation boils down to
"call strcoll(), having set LC_COLLATE to whatever pg_database says".
Non-default collations go through strcoll_l(), which might not even
exist on a given platform. So they're entirely separate code paths.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Varying results when using merge joins over postgres_fdw vs hash joins
Peter Geoghegan writes: > I think that Corey describes a user hostile behavior. I feel that we > should try to do better here. It is that. I'm tempted to propose that we invent some kind of "unknown" collation, which the planner would have to be taught to not equate to any other column collation (not even other instances of "unknown"), and that postgres_fdw's IMPORT ought to label remote columns with that collation unless specifically told to do otherwise. Then it's on the user's head if he tells us to do the wrong thing; but we won't produce incorrect plans by default. This is, of course, not at all a back-patchable fix. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Varying results when using merge joins over postgres_fdw vs hash joins
Corey Huinker writes: >> The collation column is empty here, which means that collation for >> str* columns is default collation i.e. C. This isn't true, since the >> default ncollation on the foreign server is different from the default >> collation of local database. AFAIU, import foreign schema should have >> set appropriate collation of the foreign table. > That's what we saw. The query inside IMPORT FOREIGN SCHEMA assumes a NULL > collation means default, without asking the server what that default is. No, it's not NULL, it's pg_catalog.default. The problem is exactly that that means something else on the remote server than it does locally. I'm not sure whether there's a way to fix this that doesn't break other cases. We could retrieve the pg_database.datcollate string from the remote, but that doesn't necessarily match up with any collation name we know about locally. One pretty common failure mode would be that the datcollate string isn't a canonical spelling (eg, "en_US.UTF-8" where the name we know about is "en_US.utf8"). In general, datcollate is handled through other code paths than collation names, so it might easily be that it doesn't match anything in the remote's pg_collation catalog either :-(. Another point is that when the servers' default collations do match, users would likely not thank us for replacing "default" with something else. Even if we picked a functionally equivalent collation, it would impede query optimization because the planner wouldn't know it was equivalent. Perhaps, rather than trying to fix this automatically, we should leave it to the user. We could invent another import option that says what to translate "default" to, with the default being, uh, "default". regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/subscription/t/005_encoding.pl is broken
Michael Paquier writes: > On Wed, Sep 20, 2017 at 12:33 AM, Tom Lane wrote: >> That would indicate that something isn't ever retrying the worker >> start; but if that's the case, how is it that we get through the >> other subscription tests with my random-failure patch in place? > I have been able to dig into this issue further, and the problem is > indeed in the wait logic of 005_encoding.pl. It is important to wait > for the initial sync of the subscriber to happen. There is no need to > incorporate the additional wait query in wait_for_caught_up() as well. > Please see the attached which fixes the stability problems for me even > after forcing failures in launcher.c. Ah, that makes sense. Pushed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions
Alexander Korotkov writes:
> On Wed, Sep 20, 2017 at 4:25 PM, Tom Lane wrote:
>> Yes. We don't allow out-of-line values, but we do allow compressed and
>> short-header values.
> BTW, this comment looks still invalid for me...
>> #define SIGLENINT 4 /* >122 => key will *toast*, so very slow!!! */
I haven't done the math to see if 122 is exactly the right value,
but at some point index_form_tuple would decide that the tuple was
uncomfortably big and try to compress it. So the comment is right
in general terms.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error: dsa_area could not attach to a segment that has been freed
Craig Ringer writes: > On 20 September 2017 at 16:55, Thomas Munro > wrote: >> There is a kind of garbage collection for palloc'd memory and >> also for other resources like file handles, but if you're using a big >> long lived DSA area you have nothing like that. > We need, IMO, a DSA-backed heirachical MemoryContext system. Perhaps the ResourceManager subsystem would help here. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions
Andrey Borodin writes:
> You mentioned that probably there cannot be TOASTed values in the index.
> I need to settle this question in more deterministic way.
> Can you point where to look at the code or who to ask:
> Can there be TOASTed values in index tuples?
Yes. We don't allow out-of-line values, but we do allow compressed and
short-header values.
If you don't believe me, look at index_form_tuple().
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/subscription/t/002_types.pl hanging on particular environment
Craig Ringer writes: > On 19 September 2017 at 18:04, Petr Jelinek > wrote: >> If you are asking why they are not identified by the >> BackgroundWorkerHandle, then it's because it's private struct and can't >> be shared with other processes so there is no way to link the logical >> worker info with bgworker directly. > I really want BackgroundWorkerHandle to be public, strong +1 from me. I'm confused about what you think that would accomplish. AFAICS, the point of BackgroundWorkerHandle is to allow the creator/requestor of a bgworker to verify whether or not the slot in question is still "owned" by its request. This is necessarily not useful to any other process, since they didn't make the request. The thought I had in mind upthread was to get rid of logicalrep slots in favor of expanding the underlying bgworker slot with some additional fields that would carry whatever extra info we need about a logicalrep worker. Such fields could also be repurposed for additional info about other kinds of bgworkers, when (not if) we find out we need that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Allow GiST opcalsses without compress\decompres functions
Andrey Borodin writes:
> [ 0001-Allow-uncompressed-GiST-4.patch ]
Pushed, with a bit more work on the documentation and some minor
cosmetic changes.
I did not like the fact that the new code paths added by the patch
were untested, so I went ahead and removed the no-longer-needed
no-op functions in the core GiST opclasses. There's still room
to improve the contrib opclasses, but that's much more tedious
(you need to write an extension update script) so I didn't feel
like messing with those now.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] sync process names between ps and pg_stat_activity
Michael Paquier writes: > On Fri, Sep 1, 2017 at 5:33 AM, Tom Lane wrote: >> Peter Eisentraut writes: >>> As an aside, is there a reason why the archiver process is not included >>> in pg_stat_activity? >> It's not connected to shared memory. > Do you think that monitoring would be a reason sufficient to do so? My > personal opinion on the matter is that people are more and more going > to move on with pull (*) models (aka pg_receivewal and such with > replication slots) instead of push (*) models (use of > archive_command), so that monitoring of the archiver becomes less and > less useful in the long-term. And there is also pg_stat_archiver that > covers largely the gap for archive failures. Meh. I think keeping it separate from shared memory is a good thing for reliability reasons. > Still, one reason that could be used to connect it to shared memory is > to control the interval of time used for archive attempts, which is > now a interval hardcoded of 1s in pgarch_ArchiverCopyLoop(). Here more > flexibility would be definitely useful. AFAIR, GUC reloads work regardless of that. If we wanted to make the interval configurable, this would not prevent us from doing so. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: issue: record or row variable cannot be part of multiple-item INTO list
"David G. Johnston" writes: > On Tue, Sep 19, 2017 at 2:12 PM, Tom Lane wrote: >> Notice the parens/comma positioning. It's only because text is such >> a lax datatype that you didn't notice the problem. > I saw exactly what you described - that it didn't error out and that the > text representation of the single output composite was being stored in the > first field of the target composite. i.e., that it "worked". Does that > fact that it only works if the first field of the composite type is text > change your opinion that the behavior is OK to break? No. That's not "working" for any useful value of "working". I would indeed be happy if we could just change this behavior, but I do not care to answer to the crowd of villagers that will appear if we do that. It's just way too easy to do, eg, declare r record; ... for r in select lots-o-columns from ... loop ... and then expect r to contain all the columns of the SELECT, separately. We can't change that behavior now. (And making FOR behave differently for this purpose than INTO wouldn't be very nice, either.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show backtrace when tap tests fail
Andres Freund writes: > On 2017-09-19 17:15:21 -0400, Tom Lane wrote: >> Meh --- Carp::Always isn't standard either, so I think this is just extra >> complication with little value-add. Let's just do the Devel::Confess >> incantation as Dagfinn has it. > Has ~25 times the installation base on debian tho... > https://qa.debian.org/popcon.php?package=libdevel-confess-perl (13) > vs > https://qa.debian.org/popcon.php?package=libcarp-always-perl (300) Both of those read like "lost in the noise" to me. I think with either of these, we're more or less asking PG developers to install a dependency they probably didn't have before. We might as well ask them to install the more useful one. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?
Andreas Karlsson writes:
> Hm, I like the idea but I see some issues.
> Enforcing the BCP47 seems like a good thing to me. I do not see any
> reason to allow input with syntax errors. The issue though is that we do
> not want to break people's databases when they upgrade to PostgreSQL 11.
> What if they have specified the locale in the old non-ICU format or they
> have a bogus value and we then error out on pg_upgrade or pg_restore?
Well, if PG10 shipped with that restriction in place then it wouldn't
be an issue ;-)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/subscription/t/002_types.pl hanging on particular environment
Thomas Munro writes: > This type of violent shutdown seems to be associated with occasional > corruption of .gcda files (the files output by GCC coverage builds). > The symptoms are that if you use --enable-coverage and make > check-world you'll very occasionally get a spurious TAP test failure > like this: > # Failed test 'pg_ctl start: no stderr' > # at > /home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_ctl/../../../src/test/perl/TestLib.pm > line 301. > # got: > 'profiling:/home/travis/build/postgresql-cfbot/postgresql/src/backend/nodes/copyfuncs.gcda:Merge > mismatch for function 94 > # ' > # expected: '' > I'm not sure of the exact mechanism though. GCC supplies a function > __gcov_flush() that normally runs at exit or execve, so if you're > killed without reaching those you don't get any .gcda data. Perhaps > we are in exit (or fork/exec) partway through writing out coverage > data in __gcov_flush(), and at that moment we are killed. Then a > subsequent run of instrumented code will find the half-written file > and print the "Merge mismatch" message. On a slow/loaded machine, perhaps it could be that the postmaster loses patience and SIGKILLs a backend that's still writing its .gcda data? If so, maybe we could make SIGKILL_CHILDREN_AFTER_SECS longer in coverage builds? Or bite the bullet and make it configurable ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show backtrace when tap tests fail
Andres Freund writes:
> On 2017-09-19 21:37:26 +0100, Dagfinn Ilmari Mannsåker wrote:
>> Devel::Confess is more thorough, so +1 on that.
> Or just try Devel::Confess first, and if the require fails, go to
> Carp::always.
Meh --- Carp::Always isn't standard either, so I think this is just extra
complication with little value-add. Let's just do the Devel::Confess
incantation as Dagfinn has it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: issue: record or row variable cannot be part of multiple-item INTO list
"David G. Johnston" writes:
> Actually, this does work, just not the way one would immediately expect.
On closer inspection, what's actually happening in your example is that
you're getting the SELECT's ct1 result crammed into the first column of
c1, and then a default NULL is stuck into its second column:
> ct1: (text, text)
> DO $$
> SELECT ('1', '2')::ct1 INTO c1;
> RAISE NOTICE '%', c1;
> END;
> $$;
> Notice: ("(1,2)",)
Notice the parens/comma positioning. It's only because text is such
a lax datatype that you didn't notice the problem.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SCRAM in the PG 10 release notes
Heikki Linnakangas writes: > I'm not sure what exactly to do here. Where should we stick that notice? > We could put it in the release notes, where the bullet point about SCRAM > is, but it would be well hidden. If we want to give advice to people who > might not otherwise pay attention, it should go to a more prominent > place. In the "Migration to version 10" section perhaps. Currently, it > only lists incompatibilities, which this isn't. Perhaps put the notice > after the list of incompatibilities (patch attached)? That seems pretty weird. I don't see a big reason not to just put it with the bullet point about SCRAM. People who care will notice it there just fine. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: issue: record or row variable cannot be part of multiple-item INTO list
"David G. Johnston" writes: >> Aside from being inconsistent, it doesn't cover all >> the cases --- what if you have just one query output column, that is >> composite, and you'd like it to go into a composite variable? That >> doesn't work today, and this patch doesn't fix it, but it does create >> enough confusion that we never would be able to fix it. > Actually, this does work, just not the way one would immediately expect. Uh ... how did you declare ct1, exactly? I tried this before claiming it doesn't work, and it doesn't, for me: create type complex as (r float8, i float8); create or replace function mkc(a float8, b float8) returns complex language sql as 'select a,b'; select mkc(1,2); create or replace function test() returns void language plpgsql as $$ declare c complex; begin select mkc(1,2) into c; raise notice 'c = %', c; end$$; select test(); I get ERROR: invalid input syntax for type double precision: "(1,2)" CONTEXT: PL/pgSQL function test() line 4 at SQL statement That's because it's trying to assign the result of mkc() into c.r, not into the whole composite variable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show backtrace when tap tests fail
Andrew Dunstan writes:
> On 09/19/2017 01:31 PM, Andres Freund wrote:
>> # Include module showing backtraces upon failures. As it's a
>> non-standard module, don't fail if not installed.
>> eval { use Carp::Always; }
> Or maybe Devel::Confess ?
Neither one seems to be present in a standard Perl installation :-(
> In an eval you need a 'require' rather than a 'use', AFAIK.
Yeah:
$ perl -e 'eval { use Carp::Always; }'
Can't locate Carp/Always.pm in @INC (@INC contains: /usr/local/lib64/perl5
/usr/local/share/perl5 /usr/lib64/perl5/vendor_perl
/usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5 .) at -e line 1.
BEGIN failed--compilation aborted at -e line 1.
$ perl -e 'eval { require Carp::Always; }'
$ echo $?
0
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add citext_pattern_ops for citext contrib module
Andrew Dunstan writes: > On 09/19/2017 11:11 AM, Tom Lane wrote: >> Actually, after looking closer, my advice is just to drop the new >> test cases involving accented letters. It surprises me not in the >> least that those would have nonportable behavior: probably, some >> locales will case-fold them and some won't. > Actually, it now looks to me like the problem is we forgot to tell > postgres that this data is in utf8. The problem appears to resolve (at > least on my CentOS test box, where I reproduced the buildfarm error) if > I add > set client_encoding = 'utf8'; > to the sql file. That took care of one source of problems, but I wouldn't have expected it to resolve all the problems ... and it didn't. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] issue: record or row variable cannot be part of multiple-item INTO list
Robert Haas writes: > I think the fact that single-target INTO lists and multiple-target > INTO lists are handled completely differently is extremely poor > language design. It would have been far better, as you suggested > downthread, to have added some syntax up front to let people select > the behavior that they want, but I think there's little hope of > changing this now without creating even more pain. How so? The proposal I gave is fully backwards-compatible. It's likely not the way we'd do it in a green field, but we don't have a green field. > I have a really hard time, however, imagining that anyone writes > SELECT a, b, c, d, e, f, g, h, i, j, k INTO x, y, z and wants some of > a-k to go into x, some more to go into y, and some more to go into z > (and heaven help you if you drop a column from x or y -- now the whole > semantics of the query change, yikes). What's reasonable is to write > SELECT a, b, c INTO x, y, z and have those correspond 1:1. That's certainly a case that we ought to support somehow. The problem is staying reasonably consistent with the two-decades-old precedent of the existing behavior for one target variable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] issue: record or row variable cannot be part of multiple-item INTO list
"David G. Johnston" writes: > On Tuesday, September 19, 2017, Tom Lane wrote: >> I'd be much happier if there were some notational difference >> between I-want-the-composite-variable-to-absorb-a-composite-column >> and I-want-the-composite-variable-to-absorb-N-scalar-columns. > If we change to considering exactly one output column for each target var > this seems unnecessary. Breaking backwards compatibility to that extent seems like a nonstarter. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] type cache for concat functions
Pavel Stehule writes: > [ faster-concat-2.patch ] Pushed with some cosmetic adjustments (mostly better comments). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: issue: record or row variable cannot be part of multiple-item INTO list
Pavel Stehule writes: > 2017-09-14 12:33 GMT+02:00 Anthony Bykov : >> As far as I understand, this patch adds functionality (correct me if I'm >> wrong) for users. Shouldn't there be any changes in doc/src/sgml/ with the >> description of new functionality? > It removes undocumented limit. I recheck plpgsql documentation and there > are not any rows about prohibited combinations of data types. I remain of the opinion that this patch is a fundamentally bad idea. It creates an inconsistency between what happens if the INTO target list is a single record/row variable (it absorbs all the columns of the query output) and what happens if a record/row variable is part of a multi-variable target list (now it just absorbs one column, which had better be composite). That's going to confuse people, especially since you haven't documented it. But even with documentation, it doesn't seem like good design. Aside from being inconsistent, it doesn't cover all the cases --- what if you have just one query output column, that is composite, and you'd like it to go into a composite variable? That doesn't work today, and this patch doesn't fix it, but it does create enough confusion that we never would be able to fix it. I'd be much happier if there were some notational difference between I-want-the-composite-variable-to-absorb-a-composite-column and I-want-the-composite-variable-to-absorb-N-scalar-columns. For backwards compatibility with what happens now, the latter would have to be the default. I'm wondering about "var.*" or "(var)" as the notation signaling that you want the former, though neither of those seem like they're very intuitive. If we had a notation like that, it'd be possible to ask for either behavior within a larger target list, except that we'd still need to say that I-want-a-RECORD-variable-to-absorb-N-scalar-columns has to be the only thing in its target list. Otherwise it's not very clear what N ought to be. (In some cases maybe you could reverse-engineer N from context, but I think that'd be overly complicated and bug prone.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running some tests with different segment sizes
Andres Freund writes:
> I'm working on merging the customizable segment size patch [1]. I'd
> like to run some of the regression tests using it, to guarantee
> non-standard settings have test coverage. The reason I'd like to adapt
> an existing test, rather than add a new run of the standard regression
> tests, is to avoid bloating the regression time unnecessarily.
Maybe there's a way to set up a buildfarm animal or two that run all
the tests with a different segsize?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes:
> On 2017-09-19 13:00:33 -0400, Robert Haas wrote:
>> You mean, in the postmaster?
> Yes. We try to avoid touch shmem there, but it's not like we're
> succeeding fully. See e.g. the pgstat_get_crashed_backend_activity()
> calls (which do rely on shmem being ok to some extent), pmsignal,
> BackgroundWorkerStateChange(), ...
Well, the point is to avoid touching data structures that could be
corrupted enough to confuse the postmaster. I don't have any problem with
adding some more functionality to pmsignal, say.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] USER Profiles for PostgreSQL
chiru r writes:
> We are looking for User profiles in ope source PostgreSQL.
> For example, If a user password failed n+ times while login ,the user
> access has to be blocked few seconds.
> Please let us know, is there any plan to implement user profiles in feature
> releases?.
Not particularly. You can do that sort of thing already via PAM,
for example.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG 10 release notes
"'Bruce Momjian'" writes: > On Tue, Sep 19, 2017 at 12:30:01PM -0400, Tom Lane wrote: >> Well, if the intent of the note was to encourage people to raise >> shared_buffers, it didn't do a very good job of that as written, >> because I sure didn't understand it that way. > Do you have any suggestions since it is not a code change that I can > point to? My guess is that the limitation was removed years ago, but we > only found out recently. TBH, I think that's another reason for not release-noting it. There's no concrete change to point to, and so it's hard to figure out what to say. I'm not even very sure that we should be encouraging people to change existing shared_buffers settings; the experiments that Takayuki-san did were only on Windows 10, so we don't really know that changing that would be a good idea on older Windows versions. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG 10 release notes
"'Bruce Momjian'" writes: > On Tue, Sep 19, 2017 at 12:22:39PM -0400, Tom Lane wrote: >> We don't normally release-note documentation changes. If this >> wasn't purely a documentation change, then I was probably in error >> to decide it didn't need to be in the notes. > It was purely a documentation change, but it was a documented change in a > long-standing and popular practice of not using too many shared buffers > on Windows, so I thought it wise to add it. Well, if the intent of the note was to encourage people to raise shared_buffers, it didn't do a very good job of that as written, because I sure didn't understand it that way. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes:
> Unfortunately the backends themselves also react with inaccurate error
> messages to things like immediate shutdowns...
Yeah, those signals are kind of overloaded these days. Not sure if
there's any good way to improve that.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG 10 release notes
"'Bruce Momjian'" writes:
> I am sure Tom can explain his reasoning.
We don't normally release-note documentation changes. If this
wasn't purely a documentation change, then I was probably in error
to decide it didn't need to be in the notes.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Setting pd_lower in GIN metapage
Amit Kapila writes: > On Tue, Sep 19, 2017 at 9:27 AM, Michael Paquier > wrote: >> I am not saying that no index AMs take advantage FPW compressibility >> for their meta pages. There are cases like this one, as well as one >> code path in BRIN where this is useful, and it is useful as well when >> logging FPWs of the init forks for unlogged relations. > Hmm, why is it useful for logging FPWs of the init forks for unlogged > relations? We don't use REGBUF_STANDARD in those cases. But if we started to do so, that would be a concrete benefit of this patch ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/subscription/t/005_encoding.pl is broken
Michael Paquier writes: > Now, I just had a look at the logs for a failure and a success, and > one difference can be seen in the subscriber's logs as follows: > -LOG: logical replication table synchronization worker for > subscription "mysub", table "test1" has started > -LOG: logical replication table synchronization worker for > subscription "mysub", table "test1" has finished > +WARNING: out of background worker slots > +HINT: You might need to increase max_worker_processes. > The "+" portion is for a failure, and I think that this causes the > subscription to not consume the changes from the publisher which > explains the failure in the test as the logical worker applying the > changes on the subscriber-side is not here. That would indicate that something isn't ever retrying the worker start; but if that's the case, how is it that we get through the other subscription tests with my random-failure patch in place? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add citext_pattern_ops for citext contrib module
Andrew Dunstan writes:
> This seems to have upset a number or animals in the buildfarm.
Actually, after looking closer, my advice is just to drop the new
test cases involving accented letters. It surprises me not in the
least that those would have nonportable behavior: probably, some
locales will case-fold them and some won't.
(This does open up some questions about whether the opclass will
ever be of use for Alexey's original purpose :-()
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Add citext_pattern_ops for citext contrib module
Andrew Dunstan writes:
> This seems to have upset a number or animals in the buildfarm.
Looks like all the ones that are testing in en_US locale.
> I could create a third output file, but I am seriously questioning the
> point of all this,
What locale did you use to create citext_1.out? We've never before
needed more than one output file for non-C locales.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small patch for pg_basebackup argument parsing
Ryan Murphy writes:
> Looked thru the diffs and it looks fine to me.
> Changing a lot of a integer/long arguments that were being read directly via
> atoi / atol
> to be read as strings first, to give better error handling.
>
> This looks good to go to me. Reviewing this as "Ready for Committer".
> Thanks for the patch, Pierre!
I took a quick look through this and had a few thoughts:
* If we're taking the trouble to sanity-check the input, I think we should
also check for ERANGE (i.e., overflow).
* I concur with Robert that you might as well just test for "*endptr != '\0'"
instead of adding a strlen() call.
* Replicating fiddly little code sequences like this all over the place
makes me itch. There's too much chance to get it wrong, and even more
risk of someone taking shortcuts. It has to be not much more complicated
than using atoi(), or we'll just be doing this exercise again in a few
years. So I'm thinking we need to introduce a convenience function,
perhaps located in src/common/, or else src/fe_utils/.
* Changing variables from int to long int is likely to have unpleasant
consequences on platforms where they're different; or at least, I don't
particularly feel like auditing the patch to ensure that that's not a
problem anywhere. So I think we should not do that but just make the
convenience function return int, with a suitable overflow test for that
result size. There's existing logic you can copy in
src/backend/nodes/read.c:
errno = 0;
val = strtol(token, &endptr, 10);
if (endptr != token + length || errno == ERANGE
#ifdef HAVE_LONG_INT_64
/* if long > 32 bits, check for overflow of int4 */
|| val != (long) ((int32) val)
#endif
)
... bad integer ...
If there are places where we actually do want a long result, we
could have two convenience functions, one returning int and one long.
The exact form of the convenience function is up for grabs, but one
way we could do it is
bool pg_int_convert(const char *str, int *value)
(where a true result indicates a valid integer value).
Then typical usage would be like
if (!pg_int_convert(optarg, &compresslevel) ||
compresslevel < 0 || compresslevel > 9)
... complain-and-exit ...
There might be something to be said for folding the range checks
into the convenience function:
bool pg_int_convert(const char *str, int min, int max, int *value)
if (!pg_int_convert(optarg, 0, 9, &compresslevel))
... complain-and-exit ...
That way you could protect code like
standby_message_timeout = atoi(optarg) * 1000;
fairly easily:
if (!pg_int_convert(optarg, 0, INT_MAX/1000,
&standby_message_timeout))
... complain-and-exit ...
standby_message_timeout *= 1000;
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small code improvement for btree
Doug Doole writes:
> Looks good to me.
> The new status of this patch is: Ready for Committer
Pushed. Grepping found a few more places that should be changed to
use these macros rather than referencing btpo_flags directly,
so I did that.
I tend to agree with Alvaro that it'd be better to get rid of
BT_READ/BT_WRITE in favor of using the same buffer flags used
elsewhere; but I also agree that as long as they're there we
should use them, so I included that change as well.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.
Robert Haas writes:
> On Tue, Jul 25, 2017 at 9:54 PM, Kyotaro HORIGUCHI
> wrote:
>> The patch is a kind of straightforward and looks fine for me.
> +1 for this change.
LGTM too, pushed.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes: > On 2017-09-18 12:16:42 -0400, Robert Haas wrote: >> I don't understand what you're talking about here. > I often see a backend crash, psql reacting to that crash by > reconnecting, successfully establish a new connection, just to be kicked > off by postmaster that does the crash restart cycle. I've not yet > figured out when exactly this happens and when not. It seems like this must indicate that psql sees the connection drop significantly before the postmaster gets SIGCHLD. Which is weird, but if you have core dumps enabled, maybe your kernel is doing something like (1) close files, (2) dump core, (3) exit process (resulting in SIGCHLD)? I don't think I've ever seen this myself. Peculiarity of some kernels, perhaps. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG_GETARG_GISTENTRY?
Mark Dilger writes: > I have written a patch to fix these macro definitions across src/ and > contrib/. > Find the patch, attached. All regression tests pass on my Mac laptop. Pushed after some rebasing and some minor additional editorialization. The original point about adding a wrapper for GISTENTRY fetches remains open ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/subscription/t/005_encoding.pl is broken
Andres Freund writes: > On 2017-09-18 11:50:06 -0400, Tom Lane wrote: >> The reason seems to be that its method of waiting for replication >> to happen is completely inapropos. It's watching for the master >> to say that the slave has received all the WAL, but that does not >> ensure that the logicalrep apply workers have caught up, does it? > To my knowledge here's not really any difference between the two in > logical replication. Received changes are immediately applied, there's > no equivalent to a walreceiver queing up "logical wal" onto disk. > So I'm not sure that theory holds. Well, there's *something* wrong with this test's wait method. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Boom filters for hash joins (was: A design for amcheck heapam verification)
Robert Haas writes:
> On Tue, Aug 29, 2017 at 10:22 PM, Thomas Munro
> wrote:
>> (2) We could push a Bloom filter down to scans
>> (many other databases do this, and at least one person has tried this
>> with PostgreSQL and found it to pay off[1]).
> I think the hard part is going to be figuring out a query planner
> framework for this, because pushing down the Bloom filter down to the
> scan changes the cost and the row-count of the scan.
Uh, why does the planner need to be involved at all? This seems like
strictly an execution-time optimization. Even if you wanted to try
to account for it in costing, I think the reliability of the estimate
would be nil, never mind any questions about whether the planner's
structure makes it easy to apply such an adjustment.
Personally though I would not bother with (2); I think (1) would
capture most of the win for a very small fraction of the complication.
Just for starters, I do not think (2) works for batched hashes.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reporting query on crash even if completed
Robert Haas writes: > On Mon, Sep 18, 2017 at 9:47 AM, Tom Lane wrote: >> Now, for pg_stat_activity part of the argument why this wouldn't be >> confusing was that you could also see the "state" field. Maybe we >> should try to shoehorn equivalent info into the crash log entry? > Yeah, I think so. Really, I think this is an inadvertency, and thus a > bug. But instead of just not showing the query when the backend is > idle, I'd change the display for that case to: > DETAIL: Failed process was idle; last query was: %s WFM. > Or something like that. I guess we'd need another case for a backend > that crashed without ever running a query. We already print nothing in that case, which seems fine. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add citext_pattern_ops to citext contrib module
Alexey Chernyshov writes:
> Do we need expected/citext.out? It seems that only
> expected/citext_1.out has correct output.
Well, for me, citext.out matches the results in C locale,
and citext_1.out matches the results in en_US. If you don't
satisfy both of those cases, the buildfarm will not like you.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/subscription/t/002_types.pl hanging on particular environment
Amit Kapila writes: > On Mon, Sep 18, 2017 at 7:46 PM, Tom Lane wrote: >> The subscriber log includes >> 2017-09-18 08:43:08.240 UTC [15672] WARNING: out of background worker slots >> Maybe that's harmless, but I'm suspicious that it's a smoking gun. > I have noticed while fixing the issue you are talking that this path > is also susceptible to such problems. In > WaitForReplicationWorkerAttach(), it relies on > GetBackgroundWorkerPid() to know the status of the worker which won't > give the correct status in case of fork failure. The patch I have > posted has a fix for the issue, Your GetBackgroundWorkerPid fix looks good as far as it goes, but I feel that WaitForReplicationWorkerAttach's problems go way deeper than that --- in fact, that function should not exist at all IMO. The way that launcher.c is set up, it's relying on either the calling process or the called process to free the logicalrep slot when done. The called process might never exist at all, so the second half of that is obviously unreliable; but WaitForReplicationWorkerAttach can hardly be relied on either, seeing it's got a big fat exit-on- SIGINT in it. I thought about putting a PG_TRY, or more likely PG_ENSURE_ERROR_CLEANUP, into it, but that doesn't fix the basic problem: you can't assume that the worker isn't ever going to start, just because you got a signal that means you shouldn't wait anymore. Now, this rickety business might be fine if it were only about the states of the caller and called processes, but we've got long-lived shared data involved (ie the worker slots); failing to clean it up is not an acceptable outcome. So, frankly, I think we would be best off losing the "logical rep worker slot" business altogether, and making do with just bgworker slots. The alternative is getting the postmaster involved in cleaning up lrep slots as well as bgworker slots, and I'm going to resist any such idea strongly. That way madness lies, or at least an unreliable postmaster --- if we need lrep slots, what other forty-seven data structures are we going to need the postmaster to clean up in the future? I haven't studied the code enough to know why it grew lrep worker slots in the first place. Maybe someone could explain? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallel.c oblivion of worker-startup failures
Amit Kapila writes:
> Attached patch fixes these problems.
Hmm, this patch adds a kill(notify_pid) after one call to
ForgetBackgroundWorker, but the postmaster has several more such calls.
Shouldn't they all notify the notify_pid? Should we move that
functionality into ForgetBackgroundWorker itself, so we can't forget
it again?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] src/test/subscription/t/005_encoding.pl is broken
To reproduce the subscription-startup hang that Thomas Munro observed,
I changed src/backend/replication/logical/launcher.c like this:
@@ -427,7 +427,8 @@ retry:
bgw.bgw_notify_pid = MyProcPid;
bgw.bgw_main_arg = Int32GetDatum(slot);
- if (!RegisterDynamicBackgroundWorker(&bgw, &bgw_handle))
+ if (random() < 10 ||
+ !RegisterDynamicBackgroundWorker(&bgw, &bgw_handle))
{
/* Failed to start worker, so clean up the worker slot. */
LWLockAcquire(LogicalRepWorkerLock, LW_EXCLUSIVE);
This causes about 50% of worker launch requests to fail.
With the fix I just committed, 002_types.pl gets through fine,
but 005_encoding.pl does not; it sometimes fails like this:
t/005_encoding.pl . 1/1
# Failed test 'data replicated to subscriber'
# at t/005_encoding.pl line 49.
# got: ''
# expected: '1'
# Looks like you failed 1 test of 1.
t/005_encoding.pl . Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests
The reason seems to be that its method of waiting for replication
to happen is completely inapropos. It's watching for the master
to say that the slave has received all the WAL, but that does not
ensure that the logicalrep apply workers have caught up, does it?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/subscription/t/002_types.pl hanging on particular environment
I wrote: > The subscriber log includes > 2017-09-18 08:43:08.240 UTC [15672] WARNING: out of background worker slots > 2017-09-18 08:43:08.240 UTC [15672] HINT: You might need to increase > max_worker_processes. > Maybe that's harmless, but I'm suspicious that it's a smoking gun. Actually: looking at the place where this is issued, namely logicalrep_worker_launch, and comparing it to the cleanup logic in WaitForReplicationWorkerAttach, it looks to me like the problem is we're failing to do logicalrep_worker_cleanup() in this case. We have initialized a logical rep worker slot, and we're just walking away from it. I'll go see if I can't reproduce this by injecting random failures right there. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] src/test/subscription/t/002_types.pl hanging on particular environment
Thomas Munro writes: > In this build you can see the output of the following at the end, > which might provide clues to the initiated. You might need to click a > small triangle to unfold the commands' output. > cat ./src/test/subscription/tmp_check/log/002_types_publisher.log > cat ./src/test/subscription/tmp_check/log/002_types_subscriber.log > cat ./src/test/subscription/tmp_check/log/regress_log_002_types The subscriber log includes 2017-09-18 08:43:08.240 UTC [15672] WARNING: out of background worker slots 2017-09-18 08:43:08.240 UTC [15672] HINT: You might need to increase max_worker_processes. Maybe that's harmless, but I'm suspicious that it's a smoking gun. I think perhaps this reflects a failed attempt to launch a worker, which the caller does not realize has failed to launch because of the lack of worker-fork-failure error recovery I bitched about months ago [1], leading to subscription startup waiting forever for a worker that's never going to report finishing. I see Amit K. just posted a patch in that area [2], haven't looked at it yet. regards, tom lane [1] https://www.postgresql.org/message-id/4905.1492813...@sss.pgh.pa.us [2] https://www.postgresql.org/message-id/CAA4eK1KDfKkvrjxsKJi3WPyceVi3dH1VCkbTJji2fuwKuB=3...@mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reporting query on crash even if completed
Andres Freund writes: > When a backend dies, in a manner triggering a crash restart, we > currently log something like: > DETAIL: Failed process was running: %s > > That used to be only when there's an active query, but since > commit 4f42b546fd87a80be30c53a0f2c897acb826ad52 > that's not the case anymore. I can't recall anybody complaining, but to > me it seems fairly confusing to report that some query was running when > it's actually not. I think this is fine. If the backend crashes in nominally-post-query mopup, it might be useful to know what it had been running. If it really was idle, it seems no more confusing than in pg_stat_activity. Now, for pg_stat_activity part of the argument why this wouldn't be confusing was that you could also see the "state" field. Maybe we should try to shoehorn equivalent info into the crash log entry? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Generic type subscripting
Arthur Zakirov writes:
> CREATE SUBSCRIPTING FOR type_name
> INITFUNC = subscripting_init_func
> FETCHFUNC = subscripting_fetch_func
> ASSIGNFUNC = subscripting_assign_func
> DROP SUBSCRIPTING FOR type_name
Reasonable, but let's make the syntax more like other similar
utility commands such as CREATE AGGREGATE --- basically just
adding some parens, IIRC.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] Remove useless joins (VARCHAR vs TEXT)
David Rowley writes:
> On 17 September 2017 at 08:07, Kim Rose Carlsen wrote:
>> It seems there are some difference in VARCHAR vs TEXT when postgres tries to
>> decide if a LEFT JOIN is useful or not.
> Yeah, it looks like the code to check for distinctness in the subquery
> fails to consider that the join condition may contain RelabelTypes
> instead of plain Vars.
>
> The attached fixes.
Looks like a good fix to me (except for the copied-and-pasted,
not-quite-on-point comment ;-)). Pushed.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Pre-existing bug in trigger.c
I wrote: > While fooling with the transition-tables bug, I noticed a problem > in trigger.c that has been there a very long time. > ... > I think possibly the best solution is to change the query_stack > data structure enough so that pre-existing entries don't get > moved during an enlargement. Or maybe we can fix it so the > "active" events list has a static location and the items in the > query_stack are only valid for levels below the top. After studying this awhile, I've concluded that neither of those ideas leads to a fix simple enough that I'd be comfortable with back-patching it. What seems like the best answer is to not pass delete_ok = true to afterTriggerInvokeEvents in AfterTriggerEndQuery. Then, afterTriggerInvokeEvents will examine the passed event list pointer only during its loop initialization, at which time it's surely still valid. This loses a bit of efficiency in the situation where we have to loop back because more events got added to the event list during trigger execution: we're then looking at having to scan over all the old events a second time in afterTriggerMarkEvents and afterTriggerInvokeEvents. But there's another solution to that, which is just about as efficient, if not more so: if we'll need to repeat the loop, we can first delete all event chunks before the one that was the "tail" chunk at the head of the loop. We know that afterTriggerMarkEvents marked all events then in existence as either firable or done, so any events remaining to process must have been added since then, and therefore are in the old "tail" chunk or a later one. This way turns out to get rid of exactly the same events that the delete_ok = true way did, except in the narrow corner case that the old "tail" chunk was completely full at the top of the loop, so that all the new events are in new chunks. (If it was only partly full, then it must now contain both old and new events, so that afterTriggerInvokeEvents could not have cleared it anyway.) That's close enough for me. We could possibly cover even that case by noting whether or not the tail chunk's freeptr moved, but I think it's too much complication for too little chance of gain. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes: > On 2017-09-16 14:30:21 -0400, Tom Lane wrote: >> I wonder whether we shouldn't just revert this patch altogether. > The problem is that the patch that makes the segment size configurable > also adds a bunch more ordering constraints due to the fact that the > contents of the control file influence how much shared buffers are > needed (via wal_buffers = -1, which requires the segment size, which is > read from the control file). Reading things in the wrong order leads to > bad results too. Maybe we could redefine wal_buffers to avoid that. Or read the control file at the time where we need it. This does not seem like a problem that justifies a system-wide change that's much more delicate than you thought. >> I'm now quite worried about whether we aren't introducing >> hazards of using stale values from the file; if a system crash isn't >> enough to get it to flush its cache, then what is? > I don't think the problem here is stale values, it's "just" a stale > pointer pointing into shared memory that gets reiniitalized? The code that's crashing is attempting to install some pre-existing copy of pg_control into shared memory. Even if there were good reason to think the copy were up to date, I'm not sure we should trust it in a crash recovery context. I'd rather see this hunk of code just playing dumb and reading the file (which, I'm pretty sure, is what it did up till this week). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes:
> Looking into it.
I wonder whether we shouldn't just revert this patch altogether.
Certainly, extra reads of pg_control are not a noticeable performance
problem. I'm now quite worried about whether we aren't introducing
hazards of using stale values from the file; if a system crash isn't
enough to get it to flush its cache, then what is?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Sync BEFORE STATEMENT trigger behavior with AFTER STATEMENT
The just-committed patch 0f79440fb arranges to suppress extra firings
of AFTER STATEMENT triggers that historically have occurred when
several FK enforcement trigger firings affect the same target table.
This leaves us in a situation where you may get more BEFORE STATEMENT
trigger firings than AFTER STATEMENT firings, which seems weird.
I did the attached simple patch to change this, using state very
similar to that now used for AFTER STATEMENT triggers. It works, in
terms of syncing the number of trigger calls. The timing of the
calls might seem a bit weird, as shown in the modified regression
test case. In the corner cases where you can get multiple statement
trigger firings out of one original SQL statement, the ordering
tends to look like
BEFORE STATEMENT
BEFORE STATEMENT
AFTER STATEMENT
AFTER STATEMENT
not what you might expect,
BEFORE STATEMENT
AFTER STATEMENT
BEFORE STATEMENT
AFTER STATEMENT
I'm not sure there's anything we can or should do about that, though.
Certainly we don't want to delay the execution of BEFORE calls.
I think we should push this into v10 so that we maintain consistency
of BEFORE vs. AFTER STATEMENT behavior. Objections?
regards, tom lane
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 7e391a1..78e6ce7 100644
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
*** static void AfterTriggerSaveEvent(EState
*** 100,105
--- 100,106
List *recheckIndexes, Bitmapset *modifiedCols,
TransitionCaptureState *transition_capture);
static void AfterTriggerEnlargeQueryState(void);
+ static bool before_stmt_triggers_fired(Oid relid, CmdType cmdType);
/*
*** ExecBSInsertTriggers(EState *estate, Res
*** 2229,2234
--- 2230,2240
if (!trigdesc->trig_insert_before_statement)
return;
+ /* no-op if we already fired BS triggers in this context */
+ if (before_stmt_triggers_fired(RelationGetRelid(relinfo->ri_RelationDesc),
+ CMD_INSERT))
+ return;
+
LocTriggerData.type = T_TriggerData;
LocTriggerData.tg_event = TRIGGER_EVENT_INSERT |
TRIGGER_EVENT_BEFORE;
*** ExecBSDeleteTriggers(EState *estate, Res
*** 2439,2444
--- 2445,2455
if (!trigdesc->trig_delete_before_statement)
return;
+ /* no-op if we already fired BS triggers in this context */
+ if (before_stmt_triggers_fired(RelationGetRelid(relinfo->ri_RelationDesc),
+ CMD_DELETE))
+ return;
+
LocTriggerData.type = T_TriggerData;
LocTriggerData.tg_event = TRIGGER_EVENT_DELETE |
TRIGGER_EVENT_BEFORE;
*** ExecBSUpdateTriggers(EState *estate, Res
*** 2651,2656
--- 2662,2672
if (!trigdesc->trig_update_before_statement)
return;
+ /* no-op if we already fired BS triggers in this context */
+ if (before_stmt_triggers_fired(RelationGetRelid(relinfo->ri_RelationDesc),
+ CMD_UPDATE))
+ return;
+
updatedCols = GetUpdatedColumns(relinfo, estate);
LocTriggerData.type = T_TriggerData;
*** struct AfterTriggersTableData
*** 3576,3583
Oid relid; /* target table's OID */
CmdType cmdType; /* event type, CMD_INSERT/UPDATE/DELETE */
bool closed; /* true when no longer OK to add tuples */
! bool stmt_trig_done; /* did we already queue stmt-level triggers? */
! AfterTriggerEventList stmt_trig_events; /* if so, saved list pointer */
Tuplestorestate *old_tuplestore; /* "old" transition table, if any */
Tuplestorestate *new_tuplestore; /* "new" transition table, if any */
};
--- 3592,3600
Oid relid; /* target table's OID */
CmdType cmdType; /* event type, CMD_INSERT/UPDATE/DELETE */
bool closed; /* true when no longer OK to add tuples */
! bool before_trig_done; /* did we already queue BS triggers? */
! bool after_trig_done; /* did we already queue AS triggers? */
! AfterTriggerEventList after_trig_events; /* if so, saved list pointer */
Tuplestorestate *old_tuplestore; /* "old" transition table, if any */
Tuplestorestate *new_tuplestore; /* "new" transition table, if any */
};
*** AfterTriggerSaveEvent(EState *estate, Re
*** 5651,5656
--- 5668,5704
}
/*
+ * Detect whether we already queued BEFORE STATEMENT triggers for the given
+ * relation + operation, and set the flag so the next call will report "true".
+ */
+ static bool
+ before_stmt_triggers_fired(Oid relid, CmdType cmdType)
+ {
+ bool result;
+ AfterTriggersTableData *table;
+
+ /* Check state, like AfterTriggerSaveEvent. */
+ if (afterTriggers.query_depth < 0)
+ elog(ERROR, "before_stmt_triggers_fired() called outside of query");
+
+ /* Be sure we have enough space to record events at this query depth. */
+ if (afterTrigge
Re: [HACKERS] [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes: > Perform only one ReadControlFile() during startup. This patch or something closely related to it has broken the postmaster's ability to recover from a backend crash. For example, after exercising the backend crash Andreas just reported: regression=# select from information_schema.user_mapping_options; 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. !> \q attempting to reconnect fails, because the postmaster isn't there. It left a core file behind though, in which I find Program terminated with signal 11, Segmentation fault. #0 0x0088b792 in GetMemoryChunkContext (pointer=0x7fdb36fc3f00) at ../../../../src/include/utils/memutils.h:124 124 AssertArg(MemoryContextIsValid(context)); (gdb) bt #0 0x0088b792 in GetMemoryChunkContext (pointer=0x7fdb36fc3f00) at ../../../../src/include/utils/memutils.h:124 #1 pfree (pointer=0x7fdb36fc3f00) at mcxt.c:951 #2 0x00512843 in XLOGShmemInit () at xlog.c:4897 #3 0x00737fd9 in CreateSharedMemoryAndSemaphores ( makePrivate=0 '\000', port=5440) at ipci.c:220 #4 0x006e4a78 in reset_shared () at postmaster.c:2516 #5 PostmasterStateMachine () at postmaster.c:3832 #6 0x006e541d in reaper (postgres_signal_arg=) at postmaster.c:3081 #7 #8 0x003b78ae1603 in __select_nocancel () at ../sysdeps/unix/syscall-template.S:82 #9 0x008a432a in pg_usleep (microsec=) at pgsleep.c:56 #10 0x006e75d7 in ServerLoop (argc=, argv=) at postmaster.c:1705 #11 PostmasterMain (argc=, argv=) at postmaster.c:1364 It's dying at "pfree(localControlFile)". localControlFile seems to be pointing at a region of memory that's entirely zeroes; certainly the data that it just moved into shared memory is all zeroes. It looks like someone didn't think hard enough about when to reset ControlFile to null. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] More efficient truncation of pg_stat_activity query strings
Andres Freund writes:
> Version correcting these is attached. Thanks!
I'd vote for undoing the s/st_activity/st_activity_raw/g business.
That may have been useful while writing the patch, to ensure you
found all the references; but it's just creating a lot of unnecessary
delta in the final code, with the attendant hazards for back-patches.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] additional contrib test suites
I wrote: > Peter Eisentraut writes: >> So, we have one failure for chkpass on OpenBSD, because OpenBSD crypt() >> doesn't support the traditional two-character salt format. >> Option: >> - Use the resultmap features to make this an expected failure on OpenBSD. >> - Fix the module to work on OpenBSD. This would involve making a >> platform-specific modification to use whatever advanced salt format they >> want. >> - Replace the entire module with something that does not depend on crypt(). > Or (4) drop the module's regression test again. Noting that mandrill is showing yet a different failure, one that I think is inherent to chkpass: CREATE TABLE test (i int, p chkpass); INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); + WARNING: type chkpass has unstable input conversion for "hello" + LINE 1: INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); + ^ + WARNING: type chkpass has unstable input conversion for "goodbye" + LINE 1: INSERT INTO test VALUES (1, 'hello'), (2, 'goodbye'); + ^ I'm starting to think that (4) might be the best avenue. Or we could consider (5) drop contrib/chkpass altogether, on the grounds that it's too badly designed, and too obsolete crypto-wise, to be useful or supportable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COMMIT TRIGGERs, take n, implemented with CONSTRAINT TRIGGERS
Nico Williams writes: > On Fri, Sep 15, 2017 at 02:19:29PM -0500, Nico Williams wrote: >> On Fri, Sep 15, 2017 at 11:26:08AM -0700, Andres Freund wrote: >>> I think you should also explain why that's a desirable set of >>> semantics. > Now, why is this desirable: atomicity. >The client/user can no longer add statements to the transaction, >therefore now (commit-time) is a good time to run a procedure that >can *atomically* examine the totatility of the changes made by the >user. I do not really understand this claim. The argument for a commit trigger seems to be exactly "I want to be the last thing that happens in the transaction". But as soon as you have two of them, one of them is not going to be the last thing. Maybe you could address that by requiring the triggers to be read-only, but then it doesn't seem like a very useful feature. If there can be only one, maybe it's a usable feature or maybe not, but I'm inclined to think that CREATE TRIGGER is a pretty poor API for such a definition. Triggers are generally expected to be objects you can create any number of. Another question is exactly how you're going to "examine the totality of the transaction's changes"; a trigger, per se, isn't going to know *any* of what the transaction did let alone all of it. We have some features like transition tables, but they're not accessible after end of statement; and they're pretty useless in the face of DDL too. It's going to be hard to produce a very credible use-case without a lot of work to expose that information somehow. (Some closely related work is being done for logical decoding, btw. I wonder which use-cases for this might be satisfied by having a logical decoding plug-in watching the WAL output.) > Commit triggers also allow one to record transaction boundaries, and > NOTIFY listeners less frequently than if one did a NOTIFY in normal > for-each-row/statement triggers. Um, NOTIFY already collapses duplicate notifications per-transaction. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] POC: Sharing record typmods between backends
Andres Freund writes: > On 2017-09-14 23:29:05 -0400, Tom Lane wrote: >> FWIW, I'm not on board with that. I think the version of typedefs.list >> in the tree should reflect the last official pgindent run. > Why? I see pretty much no upside to that. You can't reindent anyway, due > to unindented changes. You can get the used typedefs.list trivially from > git. Perhaps, but the real problem is still this: >> There's also a problem that it only works well if *every* committer >> faithfully updates typedefs.list, which isn't going to happen. We can't even get everybody to pgindent patches before commit, let alone update typedefs.list. So sooner or later your process is going to need to involve getting a current list from the buildfarm. >> For local pgindent'ing, I pull down >> https://buildfarm.postgresql.org/cgi-bin/typedefs.pl > That's a mighty manual process - I want to be able to reindent files, > especially new ones where it's still reasonably possible, without having > to download files, then move changes out of the way, so I can rebase, Well, that just shows you don't know how to use it. You can tell pgindent to use an out-of-tree copy of typedefs.list. I have the curl fetch and using the out-of-tree list all nicely scripted ;-) There might be something to be said for removing the typedefs list from git altogether, and adjusting the standard wrapper script to pull it from the buildfarm into a .gitignore'd location if there's not a copy there already. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] additional contrib test suites
Peter Eisentraut writes: > So, we have one failure for chkpass on OpenBSD, because OpenBSD crypt() > doesn't support the traditional two-character salt format. > Option: > - Use the resultmap features to make this an expected failure on OpenBSD. > - Fix the module to work on OpenBSD. This would involve making a > platform-specific modification to use whatever advanced salt format they > want. > - Replace the entire module with something that does not depend on crypt(). Or (4) drop the module's regression test again. I'd go for (1) at least as a short-term answer. It's not clear to me that it's worth the effort to do (2) or (3). Also, (3) probably breaks backwards compatibility, if there is anyone out there actually using this datatype. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add support for coordinating record typmods among parallel worke
Andres Freund writes:
> Sorry for missing that during review - unfortunately I don't have a computer
> with me now - so I won't get around to this till tomorrow...
It turns out that this breaks my local build, too, so I went ahead and
pushed a fix.
At least two buildfarm members think there's something deeper broken
here:
2017-09-15 00:08:34.061 EDT [97092:75] LOG: worker process: parallel worker
for PID 101175 (PID 101256) was terminated by signal 11: Segmentation fault
2017-09-15 00:08:34.061 EDT [97092:76] DETAIL: Failed process was running:
select * from pg_get_object_address('operator of access method',
'{btree,integer_ops,1}', '{int4,bool}');
I wonder if this indicates you forgot to consider transmission of state
to parallel worker processes.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] POC: Sharing record typmods between backends
Thomas Munro writes: > On Fri, Sep 15, 2017 at 3:03 PM, Andres Freund wrote: >> - added typedefs to typedefs.list > Should I do this manually with future patches? FWIW, I'm not on board with that. I think the version of typedefs.list in the tree should reflect the last official pgindent run. There's also a problem that it only works well if *every* committer faithfully updates typedefs.list, which isn't going to happen. For local pgindent'ing, I pull down https://buildfarm.postgresql.org/cgi-bin/typedefs.pl and then add any typedefs created by the patch I'm working on to that. But I don't put the result into the commit. Maybe we need a bit better documentation and/or tool support for using an unofficial typedef list. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Trouble with amcheck
Stephen Frost writes: > * Andres Freund (and...@anarazel.de) wrote: >> I'm very unconvinced by this, given that one use of installcheck is to >> run against an existing server. For which one might not even have access >> to the relevant directories to install extensions into. > Sure, but if the extensions aren't in place and you're trying to run > make installcheck-world, it's not like it's somehow going to succeed. I'm more or less with Andres: this is just pilot error. If you didn't do install-world you shouldn't expect installcheck-world to succeed. > Now, that said, perhaps a bit more smarts would be in order here to, > instead, check that the extensions are available before trying to run > the checks for them. I'm thinking about something like this: check if > the extension is available and, if not, skip the check of that module, > with a warning or notification that it was skipped because it wasn't > available. Meh. I'm worried that that would have undesirable failure modes, ie letting something pass when it should have failed. Maybe we need some documentation improvements, though, to clarify the relationships between these make targets. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] taking stdbool.h into use
Peter Eisentraut writes:
> 0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
> 0008-Use-stdbool.h-if-available.patch
> These need some more work based on Tom's feedback.
> Attached is a new patch set. Based on the discussion so far, 0001
> through 0007 might be ready; the other two need some more work.
Do you need me to do another round of tests on prairiedog/gaur/pademelon?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Pre-existing bug in trigger.c
While fooling with the transition-tables bug, I noticed a problem in trigger.c that has been there a very long time. AfterTriggerEndQuery correctly notes * ... Be careful here: firing a * trigger could result in query_stack being repalloc'd, so we can't save * its address across afterTriggerInvokeEvents calls. However, it then proceeds to pass the address of a query_stack item to afterTriggerInvokeEvents, so that if such a repalloc occurs, afterTriggerInvokeEvents is already working with an obsolete dangling pointer while it scans the rest of the events. This isn't very trivial to fix. I thought of making a local copy of the events pointer struct, but that's problematic because data can pass in a couple of directions here. Queuing of additional trigger events would modify the events list from "outside", while afterTriggerInvokeEvents occasionally wants to do if (chunk == events->tail) events->tailfree = chunk->freeptr; which has to be in sync with the real state of the events list including any subsequent additions. I think possibly the best solution is to change the query_stack data structure enough so that pre-existing entries don't get moved during an enlargement. Or maybe we can fix it so the "active" events list has a static location and the items in the query_stack are only valid for levels below the top. Given the lack of reports traceable to this, this doesn't seem super urgent to fix, so I'm not going to try to address it while hacking the transition table business. But we'll need to return to it later. Whatever we do about it has to be back-patched further than v10, anyway. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Is it time to kill support for very old servers?
Andres Freund writes: > Re-upping this topic. > On 2016-10-07 10:06:07 -0400, Tom Lane wrote: >> In the same line, maybe we should kill libpq's support for V2 protocol >> (which would make the cutoff 7.4). And maybe the server's support too, >> though that wouldn't save very much code. The argument for cutting this >> isn't so much that we would remove lots of code as that we're removing >> code that never gets tested, at least not by us. > I'd like to do this in the not too far away future for at least the > backend. There's enough not particularly pretty code to deal with v2 > that that'd be worthwhile. Hm, I don't recall that there's very much on the server side that could be saved --- what's incurring your ire, exactly? >> One small problem with cutting libpq's V2 support is that the server's >> report_fork_failure_to_client() function still sends a V2-style message. > We should really fix that so it reports the error as a v3 message, > independent of ripping out libpq-fe support for v2. It might be reasonable to do that, but libpq would have to be prepared for the other case for many years to come :-( The real problem in this area, to my mind, is that we're not testing that code --- either end of it --- in any systematic way. If it's broken it could take us quite a while to notice. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
