Re: [HACKERS] Illegal SJIS mapping
> However, running the script with that doesn't produce exactly what we
> have in utf8_to_sjis.map, either. It's otherwise same, but we have
> some extra mappings:
>
> - {0xc2a5, 0x5c},
0xc2a5 is U+00a5. The glyph is "YEN SIGN" which is corresponding to
0x5c in SJIS. So this is a valid mapping.
In the mean time, Microsoft wants to map U+005c to 0x5c in CP932. The
glyph of U+005c is "REVERSE SOLDIUS" (back slash). So MS
decided that the glyph of U+00x5c is "YEN SIGN" in CP932!
In summary we need to keep both of mappings:
U+00a5 (utf 0xc2a5) -> 0x5c and U+005c -> 0x5c.
Obviously this breaks the round trip conversion between UTF8 and SJIS
encoding in this case though.
> - {0xc2ac, 0x81ca},
U+00ac (NOT SIGN). Exists in SJIS.
> - {0xe28096, 0x8161},
U+2016 (DOUBLE VERTICAL LINE). Exists in SJIS.
> - {0xe280be, 0x7e},
U+213e (OVERLINE). Mapped to acii 0x7e, which is "half width tilde".
> - {0xe28892, 0x817c},
U+2212 (MINUS SIGN). Mapped to "double width minus sign" in SJIS.
> - {0xe3809c, 0x8160},
u+301c (WAVE DASH). Mapped to "double width wave dash" in SJIS.
> Those mappings were added in commit
> a8bd7e1c6e026678019b2f25cffc0a94ce62b24b, back in 2002. The bogus
> mapping for the invalid 0xc19c UTF-8 byte sequence was also added by
> that commit, as well a few valid mappings that UCS_to_SJIS.pl also
> produces.
>
> I can't judge if those mappings make sense. If we can't find an
> authoritative source for them, I suggest that we leave them as they
> are, but also hard-code them to UCS_to_SJIS.pl, so that running that
> script produces those mappings in utf8_to_sjis.map, even though they
> are not present in the CP932.TXT source file.
Sounds acceptable.
In summary current PostgreSQL UTF8 <--> SJIS mapping is a somewhat
mixture of SJIS (Shift_JIS) and MS932. There's no cleaner solution to
exodus this situation. I think we need live with it.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
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] Illegal SJIS mapping
Hello,
At Fri, 7 Oct 2016 23:58:45 +0300, Heikki Linnakangas wrote
in <9c544547-7214-aebe-9b04-57624aedd...@iki.fi>
> > So, I wonder how the mappings related to SJIS (and/or EUC-JP) are
> > maintained. If no authoritative information is available, the
> > generating script no longer usable. If any other autority is
> > choosed, it is to be modified according to whatever the new
> > source format is.
>
> The script is clearly intended to read CP932.TXT, rather than
> SHIFTJIS.TXT, despite the comments in it. CP932.TXT can be found at
>
> ftp://ftp.unicode.org/Public/MAPPINGS/VENDORS/MICSFT/WINDOWS/CP932.TXT
>
> However, running the script with that doesn't produce exactly what we
> have in utf8_to_sjis.map, either. It's otherwise same, but we have
> some extra mappings:
>
> - {0xc2a5, 0x5c},
> - {0xc2ac, 0x81ca},
> - {0xe28096, 0x8161},
> - {0xe280be, 0x7e},
> - {0xe28892, 0x817c},
> - {0xe3809c, 0x8160},
>
> Those mappings were added in commit
> a8bd7e1c6e026678019b2f25cffc0a94ce62b24b, back in 2002. The bogus
> mapping for the invalid 0xc19c UTF-8 byte sequence was also added by
> that commit, as well a few valid mappings that UCS_to_SJIS.pl also
> produces.
>
> I can't judge if those mappings make sense. If we can't find an
> authoritative source for them, I suggest that we leave them as they
The mappings have a hystorical reason came from differences
between Unicode definition and Oracle and Microsoft
implementations and developing of Unicode specification. So the
several SJIS (and EUC-JP) characters have two or more mappings to
Unicode. There's also several variations of the opposite
mapping. But none of them is the autority and what to adopt
depends on system requirement. The only requirement that
PostgreSQL should keep seems to be round-trip consistency starts
from SJIS input.
> are, but also hard-code them to UCS_to_SJIS.pl, so that running that
> script produces those mappings in utf8_to_sjis.map, even though they
> are not present in the CP932.TXT source file.
Agreed. I do that at least for Japanese charsets.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
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] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
2016-10-18 5:48 GMT+02:00 Craig Ringer : > On 18 October 2016 at 04:19, Andres Freund wrote: > > On 2016-10-17 16:16:37 -0400, Robert Haas wrote: > >> I wouldn't think that cross-file references would be especially > >> common. Functions that take PG_FUNCTION_ARGS and return Datum aren't > >> a lot of fun to call from C. But maybe I'm wrong. > > > > There's a fair number of DirectFunctionCall$Ns over the backend. > > It's only an issue if one contrib calls another contrib (or the core > backend code calls into a contrib, but that's unlikely) via > DirectFunctionCall . > > If changed to use regular OidFunctionCall they'll go via the catalogs > and be fine, albeit at a small performance penalty. In many cases that > can be eliminated by caching the fmgr info and using FunctionCall > directly, but it requires taking a lock on the function or registering > for invalidations so it's often not worth it. > > Personally I think it'd be cleaner to avoid one contrib referencing > another's headers directly and we have the fmgr infrastructure to make > it unnecessary. But I don't really think it's a problem that > especially needs solving either. > Not all called functions has V1 interface. I understand so plpgsql_check is not usual extension and it is a exception, but there is lot of cross calls. I can use a technique used by Tom in last changes in python's extension, but I am not able do check in linux gcc. Regards Pavel > > -- > Craig Ringer http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services > > > -- > 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] FSM corruption leading to errors
On Mon, Oct 17, 2016 at 4:34 PM, Heikki Linnakangas wrote: > >> > visibilitymap_truncate is actually also wrong, in a different way. The > truncation WAL record is written only after the VM (and FSM) are truncated. > But visibilitymap_truncate() has already modified and dirtied the page. If > the VM page change is flushed to disk before the WAL record, and you crash, > you might have a torn VM page and a checksum failure. > > Right, I missed the problem with checksums. > Simply replacing the MarkBufferDirtyHint() call with MarkBufferDirty() in > FreeSpaceMapTruncateRel would have the same issue. If you call > MarkBufferDirty(), you must WAL-log the change, and also set the page's LSN > to make sure the WAL record is flushed first. > > I'm not sure I fully understand this. If the page is flushed before the WAL record, we might have a truncated FSM where as the relation truncation is not replayed. But that's not the same problem, right? I mean, you might have an FSM which is not accurate, but it won't at the least return the blocks outside the range. Having said that, I agree your proposed changes are more robust. BTW any thoughts on race-condition on the primary? Comments at MarkBufferDirtyHint() seems to suggest that a race condition is possible which might leave the buffer without the DIRTY flag, but I'm not sure if that can only happen when the page is locked in shared mode. While most of the reports so far involved standbys, and the bug can also hit a standalone master during crash recovery, I wonder if a race can occur even on a live system. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Idempotency for all DDL statements
On 18 October 2016 at 10:26, Peter Eisentraut wrote: > On 10/17/16 2:05 PM, Kenaniah Cerny wrote: >> Could I request DDL idempotency as an addition to the development roadmap? > > There is not really a roadmap, but I think there is general interest in > this. If you want to make it happen faster, however, you will need to > start coding it yourself. Yeah. The way to make that happen is to start submitting patches. There have been a number of "IF NOT EXISTS" options added lately so take a look at those patches for guidance. Be aware that not all cases are simple, and some community members are less than fond of the whole concept so you'll need to do some convincing. I'm ambivalent myself. I'm certain that every DROP should support it. I'm less sure about CREATE and ALTER, since assuming it is in fact idempotent is questionable at best. All that IF NOT EXISTS does is skip acting if the target already exists. If the target exists but is entirely different to what would result from running the CREATE statement it silently does nothing. CREATE TABLE fred (id integer); CREATE TABLE IF NOT EXISTS fred(id text); The latter statement is not truly idempotent. To achieve that, we'd have to implicitly transform it into an ALTER and magically figure out what the correct way to preserve/transform user data is, which is not possible since there are so many possibilities and there's no way to choose between them. Second-best would be to detect that the target exists, but does not match the results of executing the current statement and ERROR appropriately. Personally I think use of CREATE / ALTER ... IF NOT EXISTS is almost always a mistake, and you should instead use a schema versioning system that handles schema changes in a managed way. But I'm still in favour of IF NOT EXISTS for convenience, easy development, and user friendliness, though they need warnings in the docs really. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
On 18 October 2016 at 04:11, Tom Lane wrote: > As for the core problem, I wonder why we aren't recommending that > third-party modules be built using the same infrastructure contrib > uses, rather than people ginning up their own infrastructure and > then finding out the hard way that that means they need PGDLLEXPORT > marks. Effectively, "PGXS for Windows". I had a quick look at getting that going a while ago, but it was going to leave me eyeballs-deep in Perl and I quickly found something more interesting to do. I think it's worthwhile, but only if we can agree in advance that the necessary infrastructure will be backported to all supported branches if at all viable. Otherwise it's a massive waste of time, since you can't actually avoid needing your own homebrew build for 5+ years. I've kind of been hoping the CMake work would make the whole mess of Perl build stuff go away. CMake would solve this quite neatly since we can bundle CMake parameters file for inclusion in other projects and could also tell pg_config how to point to it. Extensions then become trivial CMake projects. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
On 18 October 2016 at 04:19, Andres Freund wrote: > On 2016-10-17 16:16:37 -0400, Robert Haas wrote: >> I wouldn't think that cross-file references would be especially >> common. Functions that take PG_FUNCTION_ARGS and return Datum aren't >> a lot of fun to call from C. But maybe I'm wrong. > > There's a fair number of DirectFunctionCall$Ns over the backend. It's only an issue if one contrib calls another contrib (or the core backend code calls into a contrib, but that's unlikely) via DirectFunctionCall . If changed to use regular OidFunctionCall they'll go via the catalogs and be fine, albeit at a small performance penalty. In many cases that can be eliminated by caching the fmgr info and using FunctionCall directly, but it requires taking a lock on the function or registering for invalidations so it's often not worth it. Personally I think it'd be cleaner to avoid one contrib referencing another's headers directly and we have the fmgr infrastructure to make it unnecessary. But I don't really think it's a problem that especially needs solving either. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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: Replace PostmasterRandom() with a stronger way of generating ran
Michael Paquier writes: > And actually, enabling prngd would need to be controlled by a > configure switch as well disabled by default, no? AFAICT, openssl has no configuration options related to prngd; they seem to be able to use it automatically when /dev/[u]random isn't there. This surprises me a bit because the location of prngd's random-data socket is evidently variable. I've not dug into exactly how openssl figures that out, but I'm sure a little quality time with the openssl sources would explain 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] FSM corruption leading to errors
On Mon, Oct 17, 2016 at 8:04 PM, Heikki Linnakangas wrote: > visibilitymap_truncate is actually also wrong, in a different way. The > truncation WAL record is written only after the VM (and FSM) are truncated. > But visibilitymap_truncate() has already modified and dirtied the page. If > the VM page change is flushed to disk before the WAL record, and you crash, > you might have a torn VM page and a checksum failure. Good point! I didn't think about that. > Simply replacing the MarkBufferDirtyHint() call with MarkBufferDirty() in > FreeSpaceMapTruncateRel would have the same issue. If you call > MarkBufferDirty(), you must WAL-log the change, and also set the page's LSN > to make sure the WAL record is flushed first. Right.. All the code paths calling FreeSpaceMapTruncateRel or visibilitymap_truncate do flush the record, but it definitely make things more robust to set the LSN on the page. So +1 this way. > I think we need something like the attached. OK, I had a look at that. MarkBufferDirty(mapBuffer); + if (lsn != InvalidXLogRecPtr) + PageSetLSN(page, lsn); UnlockReleaseBuffer(mapBuffer); Nit: using XLogRecPtrIsInvalid() makes more sense for such checks? pg_visibility calls as well visibilitymap_truncate, but this patch did not update it. And it would be better to do the actual truncate after writing the WAL record I think. You are also breaking XLogFlush() in RelationTruncate() because vm and fsm are assigned thanks to a call of smgrexists(), something done before XLogFlush is called on HEAD, and after with your patch. So your patch finishes by never calling XLogFlush() on relation truncation even if there is a VM or a FSM. Attached is an updated version with those issues fixed. The final version does not need the test as that's really a corner-case, but I still included it to help testing for now. -- Michael truncation-vm-fsm.patch Description: application/download -- 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 Index Scans
On Thu, Oct 13, 2016 at 8:48 AM, Amit Kapila wrote: > As of now, the driving table for parallel query is accessed by > parallel sequential scan which limits its usage to a certain degree. > Parallelising index scans would further increase the usage of parallel > query in many more cases. This patch enables the parallelism for the > btree scans. Supporting parallel index scan for other index types > like hash, gist, spgist can be done as separate patches. > I would like to have an input on the method of selecting parallel workers for scanning index. Currently the patch selects number of workers based on size of index relation and the upper limit of parallel workers is max_parallel_workers_per_gather. This is quite similar to what we do for parallel sequential scan except for the fact that in parallel seq. scan, we use the parallel_workers option if provided by user during Create Table. User can provide parallel_workers option as below: Create Table With (parallel_workers = 4); Is it desirable to have similar option for parallel index scans, if yes then what should be the interface for same? One possible way could be to allow user to provide it during Create Index as below: Create Index With (parallel_workers = 4); If above syntax looks sensible, then we might need to think what should be used for parallel index build. It seems to me that parallel tuple sort patch [1] proposed by Peter G. is using above syntax for getting the parallel workers input from user for parallel index builds. Another point which needs some thoughts is whether it is good idea to use index relation size to calculate parallel workers for index scan. I think ideally for index scans it should be based on number of pages to be fetched/scanned from index. [1] - https://www.postgresql.org/message-id/CAM3SWZTmkOFEiCDpUNaO4n9-1xcmWP-1NXmT7h0Pu3gM2YuHvg%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.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] Idempotency for all DDL statements
On 10/17/16 2:05 PM, Kenaniah Cerny wrote: > Could I request DDL idempotency as an addition to the development roadmap? There is not really a roadmap, but I think there is general interest in this. If you want to make it happen faster, however, you will need to start coding it yourself. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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: Replace PostmasterRandom() with a stronger way of generating ran
On Tue, Oct 18, 2016 at 5:35 AM, Tom Lane wrote: > If we want it to fail, and don't want to retire pademelon, there are > multiple ways we could get to that goal: > > * Enable --with-openssl in pademelon's build (don't really want to do > this, since I believe almost all the rest of the buildfarm tests with > openssl) Yes, I don't think that's a good thing to make openssl installation mandatory for this animal. > * Add variant expected-files (probably bad, it'd hide real failures) > > * Add a configure option to suppress building/testing pgcrypto (maybe > just make it contingent on --with-openssl, which would allow deletion > of a bunch of code that duplicates openssl functionality...) > > * Support reading entropy from prngd (but this means we have no buildfarm > coverage for entropy-daemon-less platforms) > > None of these are perfect, but I'd say the last one is not so obviously > the best that we shouldn't consider alternatives. In light of this discussion, it seems to me that we still want at the end the --allow-weak-keys anyway as an extreme fallback, and this even if there is additional support for prngd. An essential part is to document the weakness of this option properly, like not using pgcrypto with that if there is no other entropy source on an OS. By reading this thread, the point is that we should not complicate the support for obscure nix platforms, and it would be user-unfriendly to require users to install prngd to get more entropy from the system. And actually, enabling prngd would need to be controlled by a configure switch as well disabled by default, no? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] emergency outage requiring database restart
On 18/10/16 14:12, Michael Paquier wrote: On Tue, Oct 18, 2016 at 4:21 AM, Alvaro Herrera wrote: Merlin Moncure wrote: We had several good backups since the previous outage so it's not clear the events are related but after months of smooth operation I find that coincidence highly suspicious. As always, we need to suspect hardware problems but I'm highly abstracted from them -- using esx + san. Ah, so you're subject not only to hardware flaws but also to virtualization layer bugs :-) Wait a couple of more years, and we'll get more complains about Postgres running in containers running in VMs. Even more fun waiting ahead. that started life on different hardware with a different O/S -- 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] asynchronous execution
Hello, this works but ExecAppend gets a bit degradation. At Mon, 03 Oct 2016 19:46:32 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI wrote in <20161003.194632.204401048.horiguchi.kyot...@lab.ntt.co.jp> > > Some notes: > > > > - EvalPlanQual rechecks are broken. This is fixed by adding (restoring) async-cancelation. > > - EXPLAIN ANALYZE instrumentation is broken. EXPLAIN ANALYE seems working but async-specific information is not available yet. > > - ExecReScanAppend is broken, because the async stuff needs some way > > of canceling an async request and I didn't invent anything like that > > yet. Fixed as EvalPlanQual. > > - The postgres_fdw changes pretend to be async but aren't actually. > > It's just a demo of (part of) the interface at this point. Applied my previous patch with some modification. > > - The postgres_fdw changes also report all pg-fdw paths as > > async-capable, but actually the direct-modify ones aren't, so the > > regression tests fail. All actions other than scan does vacate_connection() to use a connection. > > - Errors in the executor can leak the WaitEventSet. Probably we need > > to modify ResourceOwners to be able to own WaitEventSets. WaitEventSet itself is not leaked but epoll-fd should be closed at failure. This seems doable with TRY-CATCHing in ExecAsyncEventLoop. (not yet) > > - There are probably other bugs, too. > > > > Whee! > > > > Note that I've tried to solve the re-entrancy problems by (1) putting > > all of the event loop's state inside the EState rather than in local > > variables and (2) having the function that is called to report arrival > > of a result be thoroughly different than the function that is used to > > return a tuple to a synchronous caller. > > > > Comments welcome, if you're feeling brave enough to look at anything > > this half-baked. This doesn't cause reentry since this no longer bubbles up tupples through async-unaware nodes. This framework passes tuples through private channels for requestor and requestees. Anyway, I amended this and made postgres_fdw async and then finally all regtests passed with minor modifications. The attached patches are the following. 0001-robert-s-2nd-framework.patch The patch Robert shown upthread 0002-Fix-some-bugs.patch A small patch to fix complation errors of 0001. 0003-Modify-async-execution-infrastructure.patch Several modifications on the infrastructure. The details are shown after the measurement below. 0004-Make-postgres_fdw-async-capable.patch True-async postgres-fdw. gentblr.sql, testrun.sh, calc.pl Performance test script suite. gentblr.sql - creates test tables. testrun.sh - does single test run and calc.pl - drives testrunc.sh and summirize its results. I measured performance and had the following result. t0 - SELECT sum(a) FROM ; pl - SELECT sum(a) FROM <4 local children>; pf0 - SELECT sum(a) FROM <4 foreign children on single connection>; pf1 - SELECT sum(a) FROM <4 foreign children on dedicate connections>; The result is written as "time (std dev )" sync t0: 3820.33 ( 1.88) pl: 1608.59 ( 12.06) pf0: 7928.29 ( 46.58) pf1: 8023.16 ( 26.43) async t0: 3806.31 ( 4.49)0.4% faster (should be error) pl: 1629.17 ( 0.29)1.3% slower pf0: 6447.07 ( 25.19) 18.7% faster pf1: 1876.80 ( 47.13) 76.6% faster t0 is not affected since the ExecProcNode stuff has gone. pl is getting a bit slower. (almost the same to simple seqscan of the previous patch) This should be a misprediction penalty. pf0, pf1 are faster as expected. The below is a summary of modifications made by 0002 and 0003 patch. execAsync.c, execnodes.h: - Added include "pgstat.h" to use WAIT_EVENT_ASYNC_WAIT. - Changed the interface of ExecAsyncRequest to return if a tuple is immediately available or not. - Made ExecAsyncConfigureWait to return if it registered at least one waitevent or not. This is used to know the caller (ExecAsyncEventWait) has a event to wait (for safety). If two or more postgres_fdw nodes are sharing one connection, only one of them can be waited at once. It is a responsibility to the FDW drivers to ensure at least one wait event to be added but on failure WaitEventSetWait silently waits forever. - There were separate areq->callback_pending and areq->request_complete but they are altering together so they are replaced with one state variable areq->state. New enum AsyncRequestState for areq->state in execnodes.h. nodeAppend.c: - Return a tuple immediately if ExecAsyncRequest says that a tuple is available. - Reduced nest level of for(;;). nodeForeignscan.[ch], fdwapi.h, execProcnode.c:: - Calling postgresIterateForeignScan can yield tuples in wrong shape. Call ExecForeignScan instead. - Changed the interface of AsyncConfigureWait as execAsync.c. - Added ShutdownForeignScan interface. createplan.c, ruleutils.c, plannodes.h: - With the Rebert's change, explain sh
Re: [HACKERS] emergency outage requiring database restart
On Tue, Oct 18, 2016 at 4:21 AM, Alvaro Herrera wrote: > Merlin Moncure wrote: > >> We had several good backups since the previous outage so it's not >> clear the events are related but after months of smooth operation I >> find that coincidence highly suspicious. As always, we need to suspect >> hardware problems but I'm highly abstracted from them -- using esx + >> san. > > Ah, so you're subject not only to hardware flaws but also to > virtualization layer bugs :-) Wait a couple of more years, and we'll get more complains about Postgres running in containers running in VMs. Even more fun waiting ahead. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw super user checks
On Mon, Oct 17, 2016 at 10:51 PM, Robert Haas wrote: > On Mon, Oct 17, 2016 at 2:18 AM, Michael Paquier > wrote: >> On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes wrote: >>> postgres_fdw has some checks to enforce that non-superusers must connect to >>> the foreign server with a password-based method. The reason for this is to >>> prevent the authentication to the foreign server from happening on the basis >>> of the OS user who is running the non-foreign server. >>> >>> But I think these super user checks should be run against the userid of the >>> USER MAPPING being used for the connection, not the userid of currently >>> logged on user. >> >> So, if the user mapping user is a superuser locally, this would allow >> any lambda user of the local server to attempt a connection to the >> remote server. It looks dangerous rather dangerous to me to authorize >> that, even if the current behavior is a bit inconsistent I agree. > > I don't know what "any lambda user" means. Did you mean to write "any > random user"? Yes, in this context that would be "any non-superuser" or "any user without superuser rights". Actually that's a French-ism. I just translated it naturally to English to define a user that has no access to advanced features :) -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [ADMIN] 9.5 new setting "cluster name" and logging
* Thomas Munro (thomas.mu...@enterprisedb.com) wrote: > On Tue, Feb 9, 2016 at 3:24 AM, Andres Freund wrote: > > (x-posting to -hackers, more relevant audience) > > > > On 2016-01-29 22:19:45 -0800, Evan Rempel wrote: > >> Now that there is a setting to give a cluster a "name", it would be nice to > >> have an escape sequence in the log_line_prefix setting that could reference > >> the cluster_name. > > > > I've argued[1][2] for this when cluster_name was introduced, but back > > then I seemed to have been the only one arguing for it. Josh later > > jumped that train. > > > > Given that we now had a number of people wishing for this, can we maybe > > reconsider? > > +1 > > I missed the part of this conversation that took place on -admin. I > agree with Evan Rempel's post over there[1] that it's useful for > syslog users, and that it's not ideal to have to hijack syslog_ident > or explicitly copy the name into log_line_prefix. > > [1] https://www.postgresql.org/message-id/56C64017.7050303%40uvic.ca I'm with Thomas on this and I disagree that the "csvlog bloat" argument has merit. If we're worried about bloat in csv then we should provide a way for users to control what goes into the csvlog, not argue that something which is clearly useful be excluded. There's already a patch out there for adding a way to control what goes into csvlog and it probably wouldn't be too hard to update it. I know, because I wrote it. I continue to feel that it would be useful to add and that tools which need to deal with csvlogs should be updated to handle being told what's in it, just like pgBadger does for our much more complicated and painful regular log files. That would remove these bars to moving forward with more flexibility in log_line_prefix and csvlog output. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)
On Mon, Oct 17, 2016 at 5:36 AM, Amit Kapila wrote: > Okay, but what is the proof or why do you think second is going to > better than first? I don't have proof. It's my opinion that it probably would be, based on partial information, and my intuition. It's hard to prove something like that, because it's not really clear what that alternative would look like. Also, finding all of that out would take a long time -- it's hard to prototype. Do tuple table slots need to care about IndexTuples now? What does that even look like? What existing executor code needs to be taught about this new requirement? > One thing which strikes as a major difference > between your approach and Gather Merge is that in your approach leader > has to wait till all the workers have done with their work on sorting > whereas with Gather Merge as soon as first one is done, leader starts > with merging. I could be wrong here, but if I understood it > correctly, then there is a argument that Gather Merge kind of approach > can win in cases where some of the workers can produce sorted outputs > ahead of others and I am not sure if we can dismiss such cases. How can it? You need to have at least one tuple from every worker (before the worker has exhausted its supply of output tuples) in order to merge to return the next tuple to the top level consumer (the thing above the Gather Merge). If you're talking about "eager vs. lazy merging", please see my previous remarks on that, on this thread. (In any case, whether we merge more eagerly seems like an orthogonal question to the one you ask.) The first thing to note about my approach is that I openly acknowledge that this parallel CREATE INDEX patch is not much use for parallel query. I have only generalized tuplesort.c to support parallelizing a sort operation. I think that parallel query needs partitioning to push down parts of a sort to workers, with little or no need for them to be funneled together at the end, since most tuples are eliminated before being passed to the Gather/Gather Merge node. The partitioning part is really hard. I guess that Gather Merge nodes have value because they allow us to preserve the sorted-ness of a parallel path, which might be most useful because it enables things elsewhere. But, that doesn't really recommend making Gather Merge nodes good at batch processing a large number of tuples, I suspect. (One problem with the tuple queue mechanism is that it can be a big bottleneck -- that's why we want to eliminate most tuples before they're passed up to the leader, in the case of parallel sequential scan in 9.6.) I read the following paragraph from the Volcano paper just now: """ During implementation and benchmarking of parallel sorting, we added two more features to exchange. First, we wanted to implement a merge network in which some processors produce sorted streams merge concurrently by other processors. Volcano’s sort iterator can be used to generate a sorted stream. A merge iterator was easily derived from the sort module. It uses a single level merge, instead of the cascaded merge of runs used in sort. The input of a merge iterator is an exchange. Differently from other operators, the merge iterator requires to distinguish the input records by their producer. As an example, for a join operation it does not matter where the input records were created, and all inputs can be accumulated in a single input stream. For a merge operation, it is crucial to distinguish the input records by their producer in order to merge multiple sorted streams correctly. """ I don't really understand this paragraph, but thought I'd ask: why the need to "distinguish the input records by their producer in order to merge multiple sorted streams correctly"? Isn't that talking about partitioning, where each workers *ownership* of a range matters? My patch doesn't care which values belong to which workers. And, it focuses quite a lot on dealing well with the memory bandwidth bound, I/O bound part of the sort where we write out the index itself, just by piggy-backing on tuplesort.c. I don't think that that's useful for a general-purpose executor node -- tuple-at-a-time processing when fetching from workers would kill performance. > By looking at 'workersFinished' usage, it looks like you have devised > a new way for leader to know when workers have finished which might be > required for this patch. However, have you tried to use or > investigate if existing infrastructure which serves same purpose could > be used for it? Yes, I have. I think that Robert's "condition variables" patch would offer a general solution to what I've devised. What I have there is, as you say, fairly ad-hoc, even though my requirements are actually fairly general. I was actually annoyed that there wasn't an easier way to do that myself. Robert has said that he won't commit his "condition variables" work until it's clear that there will be some use for the facility. Well, I'd use it for this patch, if
Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
Robert Haas wrote: Yeah, I don't know. For my money, decorating the function definitions in place seems easier than having to maintain a separate export list, especially if it can be hidden under the carpet using the existing stupid macro tricks. But I am not a Windows expert. I suppose we should to establish politics for this case. Because any who see this and who have experience in Windows surprised by this. For windows any DLL it is like plugins which must use strict API for communications and resolving symbols. The situation is that in Postgres we have not API for extensions in the Windows terms. In future CMake will hide all this troubles in itself but if tell in truth I don't like this situation when any extension has access to any non-static symbols. However time to time we meet static function that we want to reusing in our extension and today I know only one decision - copy-paste. Without strict politics in this case we will be time to time meet new persons who ask this or similar question. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [ADMIN] 9.5 new setting "cluster name" and logging
On Tue, Feb 9, 2016 at 3:24 AM, Andres Freund wrote: > Hi, > > (x-posting to -hackers, more relevant audience) > > On 2016-01-29 22:19:45 -0800, Evan Rempel wrote: >> Now that there is a setting to give a cluster a "name", it would be nice to >> have an escape sequence in the log_line_prefix setting that could reference >> the cluster_name. > > I've argued[1][2] for this when cluster_name was introduced, but back > then I seemed to have been the only one arguing for it. Josh later > jumped that train. > > Given that we now had a number of people wishing for this, can we maybe > reconsider? +1 I missed the part of this conversation that took place on -admin. I agree with Evan Rempel's post over there[1] that it's useful for syslog users, and that it's not ideal to have to hijack syslog_ident or explicitly copy the name into log_line_prefix. [1] https://www.postgresql.org/message-id/56C64017.7050303%40uvic.ca -- Thomas Munro http://www.enterprisedb.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] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
17 октября 2016 г. 23:42:30 MSK, Tom Lane wrote: [ wanders away wondering what cmake does with this... ] CMake can export all symbols using only one setting - WINDOWS_EXPORT_ALL_SYMBOLS for shared libraries and special for Postgres I made "export all" for executable files. You can try this in cmake-3.7.0-rc1. Current postgres_cmake is using the modified gendef.pl (use only stdin/stdout for objtool without any files). regards, Yury -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Make getrusage() output a little more readable
On Fri, Oct 14, 2016 at 7:06 AM, Peter Eisentraut wrote: > The getrusage() output that is shown by VACUUM VERBOSE and other places > has always been a little too terse and confusing for me. I propose to > improve that a bit: > > -CPU 0.01s/0.08u sec elapsed 0.18 sec. > +CPU: user: 0.08s, system: 0.01s, elapsed: 0.18s. I look at trace_sort output a lot, and do agree that this is confusing. +1. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Make getrusage() output a little more readable
On Fri, Oct 14, 2016 at 10:06 AM, Peter Eisentraut wrote: > The getrusage() output that is shown by VACUUM VERBOSE and other places > has always been a little too terse and confusing for me. I propose to > improve that a bit: > > -CPU 0.01s/0.08u sec elapsed 0.18 sec. > +CPU: user: 0.08s, system: 0.01s, elapsed: 0.18s. > > Patch attached. > > I have also updated the ShowUsage() output used by log_parser_stats etc. > to match this change, but I don't claim that that output is particularly > readable either way. > > While updating the VACUUM man page I noticed that the example output > doesn't match exactly anymore, but I didn't do anything about that in > this patch, just changed the lines that are affected by this patch. +1 for this change. I have reviewed the patch and it seems fine. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bit|varbit #, xor operator
On Sun, Oct 16, 2016 at 4:13 PM, Tom Lane wrote: > Jim Nasby writes: >> Personally I think it was a mistake to use # for intersection. Range >> doesn't do that (using * instead), and AFAICT PostGIS doesn't either >> (preferring &). So I propose renaming those operators, as well as the >> XOR ones. I think ^^ is pretty logical for XOR. I'm not sure about >> intersect... * doesn't seem like a good idea, && is overlaps, maybe &*. > > I'm pretty much -1 on renaming any of these existing operators. I'm *definitely* -1 on renaming any of these existing operators. > As for renaming intersection, renaming operators that have had those names > since Berkeley, and are perfectly consistent within their datatype family, > seems likely to create much more pain than it removes. Agreed. > As for counting bits in a bitstring, why do we have to make that an > operator at all? Using a function would decrease the stress involved > in choosing a name, and it's hard to believe that the requirement is > so common that we need to shave a few keystrokes. But if you must have > an operator there's not that much wrong with using prefix # for it. Yeah, I think the value of operators other than the basic arithmetic and logical operations is very low. I mean, if it's not going to be familiar to people based on their general knowledge of mathematics and/or other programming languages, it's actually easier to find a function than it is to find an operator. If I want a function that does something to do with counting, I can type: \df *count* If I want an operator that does something similar, I'm out of luck: \do *count* looks for operators that contain count in the operator name, not the description. Yeah, that could be changed, but bits_count() is a lot easier to remember than # or whatever. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
On Mon, Oct 17, 2016 at 4:42 PM, Tom Lane wrote: > Robert Haas writes: >> On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane wrote: >>> As for the core problem, I wonder why we aren't recommending that >>> third-party modules be built using the same infrastructure contrib >>> uses, rather than people ginning up their own infrastructure and >>> then finding out the hard way that that means they need PGDLLEXPORT >>> marks. > >> So, they'd need to generate export files somehow? > > My point is that that's a solved problem. Perhaps the issue is that > we haven't made our src/tools/msvc infrastructure available for outside > use in the way that we've exported our Unix build infrastructure through > PGXS. But if so, I should think that working on that is the thing to do. Yeah, I don't know. For my money, decorating the function definitions in place seems easier than having to maintain a separate export list, especially if it can be hidden under the carpet using the existing stupid macro tricks. But I am not a Windows expert. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Non-empty default log_line_prefix
On Fri, Oct 14, 2016 at 7:04 AM, Christoph Berg wrote: > Re: Stephen Frost 2016-10-12 <20161012190732.gj13...@tamriel.snowman.net> >> For my 2c, I'd rather have %m, but I definitely agree with Robert that >> we need to do *something* here and if the only thing holding us back is >> %t vs. %m, then let's just pick one and move on. I'll just hold my nose >> when I see the default and change it to %m. > > Here's the very same patch with %m instead of %t. Pick one :) > > (Fwiw, I'm still leaning towards %t, but my eyes are becoming more and > more accustomed to %m as well. I'd be fine with it as well. (I'd > rather want to try to get rid of the timezone identifier there...)) So, surveying this thread, what I see is: People who prefer %m: Jeff Janes, Devrim Gündüz, Stephen Frost People who prefer %t: Christoph Berg, Tom Lane (I think) We can kibitz this more later, but for now I've committed it with %m. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bit|varbit #, xor operator
On Mon, Oct 17, 2016 at 1:39 PM, Jim Nasby wrote: > On 10/17/16 11:29 AM, Tom Lane wrote: > >> Jim Nasby writes: >> >>> On 10/16/16 3:13 PM, Tom Lane wrote: >>> Related to this I'd also like to add a boolean XOR operator as that's a > relatively common request/question. > >> We have boolean XOR; it's spelled "<>". >>> >> I always forget about that... >>> >> >> Maybe it should be mentioned explicitly in the docs. >> > > Hrm, went to go add it and it appears we don't have a section for boolean > type operators. I guess I should add it to 9.1? > > There are no symbolic operators, just the standard SQL keywords: AND, OR, NOT. https://www.postgresql.org/docs/current/static/functions-logical.html Adding a note there pertaining to XOR should be sufficient. though, it doesn't work for boolean arrays. >>> >> >> Define doesn't work? >> > > I would expect array[true, false] XOR array[true, true] to return > array[false, true], but <> just returns a single true (since the arrays are > !=). > > Though, I guess what would make the most sense there is a map function > that would apply an operator or function to every element of a set of > arrays. But I don't see any way to do that without major changes to how > anyarray works. Yeah, your expectations seem off here given that: SELECT array[true, false]::boolean[] AND array[true, true]::boolean[] is invalid... David J.
Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
Robert Haas writes: > On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane wrote: >> As for the core problem, I wonder why we aren't recommending that >> third-party modules be built using the same infrastructure contrib >> uses, rather than people ginning up their own infrastructure and >> then finding out the hard way that that means they need PGDLLEXPORT >> marks. > So, they'd need to generate export files somehow? My point is that that's a solved problem. Perhaps the issue is that we haven't made our src/tools/msvc infrastructure available for outside use in the way that we've exported our Unix build infrastructure through PGXS. But if so, I should think that working on that is the thing to do. [ wanders away wondering what cmake does with this... ] 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] bit|varbit #, xor operator
On 10/17/16 11:29 AM, Tom Lane wrote: Jim Nasby writes: On 10/16/16 3:13 PM, Tom Lane wrote: Related to this I'd also like to add a boolean XOR operator as that's a relatively common request/question. We have boolean XOR; it's spelled "<>". I always forget about that... Maybe it should be mentioned explicitly in the docs. Hrm, went to go add it and it appears we don't have a section for boolean type operators. I guess I should add it to 9.1? though, it doesn't work for boolean arrays. Define doesn't work? I would expect array[true, false] XOR array[true, true] to return array[false, true], but <> just returns a single true (since the arrays are !=). Though, I guess what would make the most sense there is a map function that would apply an operator or function to every element of a set of arrays. But I don't see any way to do that without major changes to how anyarray works. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- 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] process type escape for log_line_prefix
On 2016-10-15 17:43:40 -0700, Jeff Janes wrote: > On Fri, Oct 14, 2016 at 11:51 AM, Andres Freund wrote: > > > On 2016-10-14 13:11:51 +0200, Christoph Berg wrote: > > > Re: Michael Paquier 2016-02-10 > 5y9bpxz+dejbfcctebf06ef2u...@mail.gmail.com> > > > > On Mon, Feb 8, 2016 at 11:32 PM, Andres Freund > > wrote: > > > > > Frequently when reading postgres logs to do some post mortem analysis > > > > > I'm left wondering what process emitted an error/log message. After > > the > > > > > fact it's often hard to know wether an error message was emitted by a > > > > > user backend or by something internal, say the checkpointer or > > > > > autovacuum. Logging all process starts is often impractical given > > the > > > > > log volume that causes. > > > > > > > > > > So I'm proposing adding an escape displaying the process title (say > > 'k' > > > > > for kind?). So %k would emit something like "autovacuum worker > > process", > > > > > "wal sender process" etc. > > > > > > > > It would be nice to get something consistent between the ps output and > > > > this new prefix, say with for example a miscadmin.h parameter like > > > > MyProcName. > > > > > > > > > I'm thinking it might make sense to give normal connections "" as the > > > > > name, they're usually already discernible. > > > > > > > > Yeah, that sounds fine for me. What about background workers? I would > > > > think that they should use BackgroundWorker->bgw_name. > > > > > > (Rediscovering an old horse) > > > > > > Couldn't these processes just set %a = application_name? > > > > It'd not get me what I'd want, no. E.g for walsenders that'd not be > > parsable in a meaningful way. I really would like an escape that'd > > always output one of: > > Postmaster, Startup, BgWriter, Checkpointer, WalWriter, WalReceiver, > > AutovacLauncher, AutovacWorker, PgArch, PgStat, SysLogger, Backend, > > BackgroundWorker. > > > > I'm not sure what you are proposing. Which of those 13 strings you listed > would a walsender advertise itself as? Oops, left that one out (as it's not one of the pids explicitly listed in postmaster.c, which I went over). 'WalSender'. > Why would stuffing one of those words into %k be different than > stuffing that same word into %a, where %a would otherwise be the empty > string? Because you very well might want to keep tracking application_name for walsenders - as that's important e.g. for sync replica tracking purposes - and be able to categorize log messages by the type of process. - Andres -- 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: Replace PostmasterRandom() with a stronger way of generating ran
Robert Haas writes: > On Mon, Oct 17, 2016 at 1:48 PM, Heikki Linnakangas wrote: >> I'm going to try implementing prngd support. It seems easy enough, and prngd >> can be run on modern systems too, which is important for testing it. > TBH, if pandemolon is the only system in the BF that doesn't have any > other source of entropy, I think that's going 100% in the wrong > direction. Even with prngd support, this carries a significant risk > of effectively desupporting a large number of obscure UNIX platforms, > which I think is a bad decision. It's fine (IMHO) to have > optimizations in the code here and there that cater to Windows and > Linux because those are the most widely-used platfoms, but we've been > pretty diligent about trying to be portable to a wide variety of > UNIX-like platforms. I think that's a good decision, and reversing it > over the strength of cancel keys seems like letting the tail wag the > dog. This is largely my position as well. A relevant point here is that prngd did not come with that platform --- I had to install it after the fact. (If memory serves, which it may not because this was years ago, I did so because some new version of openssl started whining about lack of an entropy source.) Strong cancel keys are simply not important enough to desupport a bunch of old platforms over. I'm not convinced either way about pgcrypto. It's certainly a reasonable position that you want that to generate sound keys or else fail entirely. But if nothing else on the platform is generating sound keys, do we want to insist on being the first? If we want it to fail, and don't want to retire pademelon, there are multiple ways we could get to that goal: * Enable --with-openssl in pademelon's build (don't really want to do this, since I believe almost all the rest of the buildfarm tests with openssl) * Add variant expected-files (probably bad, it'd hide real failures) * Add a configure option to suppress building/testing pgcrypto (maybe just make it contingent on --with-openssl, which would allow deletion of a bunch of code that duplicates openssl functionality...) * Support reading entropy from prngd (but this means we have no buildfarm coverage for entropy-daemon-less platforms) None of these are perfect, but I'd say the last one is not so obviously the best that we shouldn't consider alternatives. 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] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
On 2016-10-17 16:16:37 -0400, Robert Haas wrote: > I wouldn't think that cross-file references would be especially > common. Functions that take PG_FUNCTION_ARGS and return Datum aren't > a lot of fun to call from C. But maybe I'm wrong. There's a fair number of DirectFunctionCall$Ns over the backend. Greetings, Andres Freund -- 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] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
On Mon, Oct 17, 2016 at 4:11 PM, Tom Lane wrote: > Robert Haas writes: >> On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz >> wrote: >>> Actually I would say that the correct solution is to remove the function >>> declarations from all the header files in contrib, since from commit >>> e7128e8d >>> on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right? > >> Right. Why isn't that already the case? Commit e7128e8d seems to >> have tried. Did it just miss a bunch of cases? > > That only works to the extent that there are no cross-file references to > those symbols within the extension. If that's true for 99% or more of > them then this is probably a good way to go. If it's only true for, say, > 75%, I'm not sure we want to decimate the headers that way. We'd end > up with something doubly ugly: both haphazard-looking lists of only > some of the symbols, and PGDLLEXPORT marks on those that remain. I wouldn't think that cross-file references would be especially common. Functions that take PG_FUNCTION_ARGS and return Datum aren't a lot of fun to call from C. But maybe I'm wrong. > As for the core problem, I wonder why we aren't recommending that > third-party modules be built using the same infrastructure contrib > uses, rather than people ginning up their own infrastructure and > then finding out the hard way that that means they need PGDLLEXPORT > marks. So, they'd need to generate export files somehow? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel bitmap heap scan
Hi, On 2016-10-07 11:46:40 +0530, Dilip Kumar wrote: > Brief design idea: > --- > #1. Shared TIDBitmap creation and initialization > First worker to see the state as parallel bitmap info as PBM_INITIAL > become leader and set the state to PBM_INPROGRESS All other workers > see the state as PBM_INPROGRESS will wait for leader to complete the > TIDBitmap. > > #2 At this level TIDBitmap is ready and all workers are awake. > > #3. Bitmap processing (Iterate and process the pages). > In this phase each worker will iterate over page and chunk array and > select heap pages one by one. If prefetch is enable then there will be > two iterator. Since multiple worker are iterating over same page and > chunk array we need to have a shared iterator, so we grab a spin lock > and iterate within a lock, so that each worker get and different page > to process. I don't quite understand why the bitmap has to be parallel at all. As far as I understand your approach as described here, the only thing that needs to be shared are the iteration arrays. Since they never need to be resized and such, it seems to make a lot more sense to just add an API to share those, instead of the whole underlying hash. Greetings, Andres Freund -- 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] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
Robert Haas writes: > On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz > wrote: >> Actually I would say that the correct solution is to remove the function >> declarations from all the header files in contrib, since from commit e7128e8d >> on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right? > Right. Why isn't that already the case? Commit e7128e8d seems to > have tried. Did it just miss a bunch of cases? That only works to the extent that there are no cross-file references to those symbols within the extension. If that's true for 99% or more of them then this is probably a good way to go. If it's only true for, say, 75%, I'm not sure we want to decimate the headers that way. We'd end up with something doubly ugly: both haphazard-looking lists of only some of the symbols, and PGDLLEXPORT marks on those that remain. As for the core problem, I wonder why we aren't recommending that third-party modules be built using the same infrastructure contrib uses, rather than people ginning up their own infrastructure and then finding out the hard way that that means they need PGDLLEXPORT marks. 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 bitmap heap scan
On Mon, Oct 17, 2016 at 1:23 AM, Dilip Kumar wrote: > There is major chance in tidbitmap.c file after efficient hash table > commit [1] and my patch need to be rebased. > > Only parallel-bitmap-heap-scan need to be rebased, all other patch can > be applied on head as is. > Rebased version (v2) of parallel-bitmap-heap-scan is attached. But what's the impact on performance? Presumably parallel bitmap heap scan was already slower than the non-parallel version, and that commit presumably widens the gap. Seems like something to worry about... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Replace PostmasterRandom() with a stronger way of generating ran
On Mon, Oct 17, 2016 at 1:48 PM, Heikki Linnakangas wrote: > I'm going to try implementing prngd support. It seems easy enough, and prngd > can be run on modern systems too, which is important for testing it. TBH, if pandemolon is the only system in the BF that doesn't have any other source of entropy, I think that's going 100% in the wrong direction. Even with prngd support, this carries a significant risk of effectively desupporting a large number of obscure UNIX platforms, which I think is a bad decision. It's fine (IMHO) to have optimizations in the code here and there that cater to Windows and Linux because those are the most widely-used platfoms, but we've been pretty diligent about trying to be portable to a wide variety of UNIX-like platforms. I think that's a good decision, and reversing it over the strength of cancel keys seems like letting the tail wag the dog. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Replace PostmasterRandom() with a stronger way of generating ran
On Mon, Oct 17, 2016 at 2:14 PM, Tom Lane wrote: > But in general, I think that being this picky about cancel keys on systems > that are too old to have /dev/random is not really helpful to anybody. > I don't recall any reports of anyone ever having a DOS situation from > weak cancel keys. It's fine to upgrade our practice where it's convenient > to do that, but taking away functionality on systems where it's not > convenient isn't improving anyone's life. Right. I strongly agree with that. If somebody's running on a platform where they don't have a good source of entropy, they are clearly going to still want query cancel to work. They are not going to want ^C to start doing nothing, and they are *definitely* not going to want PostgreSQL to fail to compile and/or start. pgcrypto is a different situation, but I think it's just crazy to say that the problems with cancel keys are so bad that we should just refuse to run at all. Anyone who is in this situation has this problem not just with PostgreSQL but with everything on their system that wishes it had cryptographically strong random numbers, which is probably quite a bit of stuff. We shouldn't take the position that a machine without a good PRNG is a brick. They just have to accept that random number generation will be weaker not only for PostgreSQL but for any software whatever that they run on that machine. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
On Fri, Oct 14, 2016 at 10:14 AM, Albe Laurenz wrote: > Tom Lane wrote: >>> Well, the buildfarm doesn't like that even a little bit. It seems that >>> the MSVC compiler does not like seeing both "extern Datum foo(...)" and >>> "extern PGDLLEXPORT Datum foo(...)", so anything that had an extern in >>> a .h file is failing. There is also quite a bit of phase-of-the-moon >>> behavior in here, because in some cases some functions are raising errors >>> and others that look entirely the same are not. >> >> I take back the latter comment --- I was comparing HEAD source code >> against errors reported on back branches, and at least some of the >> discrepancies are explained by additions/removals of separate "extern" >> declarations. But still, if we want to do this we're going to need to >> put PGDLLEXPORT in every V1 function extern declaration. We might be >> able to remove some of the externs as unnecessary instead, but any way >> you slice it it's going to be messy. >> >> For the moment I've reverted the change. > > Sorry for having caused the inconvenience. > > Actually I would say that the correct solution is to remove the function > declarations from all the header files in contrib, since from commit e7128e8d > on the functions are declared by PG_FUNCTION_INFO_V1 anyway, right? Right. Why isn't that already the case? Commit e7128e8d seems to have tried. Did it just miss a bunch of cases? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support
On 10/12/16 4:59 PM, Tom Lane wrote: > The larger picture here is that we got very little thanks when we squeezed > IPv6 into the pre-existing inet datatype; there's a large number of people > who just said "no thanks" and started using the add-on ip4r type instead. I don't think that is a correct account. People used the ip4r extension because it was faster, had more functionality, and didn't have those stupid network masks to worry about. ip4r does in fact also provide a type that can contain ip4 and ip6, which one ought to use nowadays. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] emergency outage requiring database restart
Merlin Moncure wrote: > We had several good backups since the previous outage so it's not > clear the events are related but after months of smooth operation I > find that coincidence highly suspicious. As always, we need to suspect > hardware problems but I'm highly abstracted from them -- using esx + > san. Ah, so you're subject not only to hardware flaws but also to virtualization layer bugs :-) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] emergency outage requiring database restart
On Mon, Oct 17, 2016 at 2:04 PM, Alvaro Herrera wrote: > Merlin Moncure wrote: > >> castaging=# CREATE OR REPLACE VIEW vw_ApartmentSample AS >> castaging-# SELECT ... >> ERROR: 42809: "pg_cast_oid_index" is an index >> LINE 11: FROM ApartmentSample s >> ^ >> LOCATION: heap_openrv_extended, heapam.c:1304 >> >> should I be restoring from backups? > > It's pretty clear to me that you've got catalog corruption here. You > can try to fix things manually as they emerge, but that sounds like a > fool's errand. agreed. current plan is to restore from backups, and recover as much data as I can. Also doing bugfix release and going to enable checksums. We had several good backups since the previous outage so it's not clear the events are related but after months of smooth operation I find that coincidence highly suspicious. As always, we need to suspect hardware problems but I'm highly abstracted from them -- using esx + san. merlin -- 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] emergency outage requiring database restart
Merlin Moncure wrote: > castaging=# CREATE OR REPLACE VIEW vw_ApartmentSample AS > castaging-# SELECT ... > ERROR: 42809: "pg_cast_oid_index" is an index > LINE 11: FROM ApartmentSample s > ^ > LOCATION: heap_openrv_extended, heapam.c:1304 > > should I be restoring from backups? It's pretty clear to me that you've got catalog corruption here. You can try to fix things manually as they emerge, but that sounds like a fool's errand. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] emergency outage requiring database restart
On Mon, Oct 17, 2016 at 1:39 PM, Merlin Moncure wrote: > On Thu, Oct 13, 2016 at 4:13 PM, Tom Lane wrote: >> Merlin Moncure writes: >>> Today I had an emergency production outage on a server. >>> ... >>> Adding all this up it smells like processes were getting stuck on a >>> spinlock. >> >> Maybe. If it happens again, probably the most useful debug data would >> be stack traces from some of the busy processes. > > Another odd datapoint on this server. Things were running pretty good > but an application crashed on a missing view. Trying to recreate the > view, I got: > > CREATE OR REPLACE VIEW vw_ApartmentQueueLastGood AS > SELECT ... > > ERROR: type "vw_apartmentqueuelastgood" already exists > HINT: A relation has an associated type of the same name, so you must > use a name that doesn't conflict with any existing type. > > ...which was pretty strange. I had to manually delete the pg_type > record in order to create the view. I'm getting more reports of > 'could not open relation with oid=X' errors so I could be facing data > corruption :(. castaging=# CREATE OR REPLACE VIEW vw_ApartmentSample AS castaging-# SELECT ... ERROR: 42809: "pg_cast_oid_index" is an index LINE 11: FROM ApartmentSample s ^ LOCATION: heap_openrv_extended, heapam.c:1304 should I be restoring from backups? merlin -- 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] emergency outage requiring database restart
On Thu, Oct 13, 2016 at 4:13 PM, Tom Lane wrote: > Merlin Moncure writes: >> Today I had an emergency production outage on a server. >> ... >> Adding all this up it smells like processes were getting stuck on a spinlock. > > Maybe. If it happens again, probably the most useful debug data would > be stack traces from some of the busy processes. Another odd datapoint on this server. Things were running pretty good but an application crashed on a missing view. Trying to recreate the view, I got: CREATE OR REPLACE VIEW vw_ApartmentQueueLastGood AS SELECT ... ERROR: type "vw_apartmentqueuelastgood" already exists HINT: A relation has an associated type of the same name, so you must use a name that doesn't conflict with any existing type. ...which was pretty strange. I had to manually delete the pg_type record in order to create the view. I'm getting more reports of 'could not open relation with oid=X' errors so I could be facing data corruption :(. merlin -- 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] COPY as a set returning function
On Fri, Sep 30, 2016 at 9:56 PM, Tom Lane wrote: > Craig Ringer writes: >> On 1 Oct. 2016 05:20, "Tom Lane" wrote: >>> I think the last of those suggestions has come up before. It has the >>> large advantage that you don't have to remember a different syntax for >>> copy-as-a-function. > >> That sounds fantastic. It'd help this copy variant retain festure parity >> with normal copy. And it'd bring us closer to being able to FETCH in non >> queries. > > On second thought, though, this couldn't exactly duplicate the existing > COPY syntax, because COPY relies heavily on the rowtype of the named > target table to tell it what it's copying. You'd need some new syntax > to provide the list of column names and types, which puts a bit of > a hole in the "syntax we already know" argument. A SRF-returning-record > would have a leg up on that, because we do have existing syntax for > defining the concrete rowtype that any particular call returns. One big disadvantage of SRF-returning-record syntax is that functions are basically unwrappable with generic wrappers sans major gymnastics such as dynamically generating the query and executing it. This is a major disadvantage relative to the null::type hack we use in the populate_record style functions and perhaps ought to make this (SRF-returning-record syntax) style of use discouraged for useful library functions. If there were a way to handle wrapping I'd withdraw this minor objection -- this has come up in dblink discussions a few times). merlin -- 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] Better logging of COPY queries if log_statement='all'
Aleksander Alekseev wrote: > According to my colleagues it would be very nice to have this feature. > For instance, if you are trying to optimize PostgreSQL for application > that uses COPY and you don't have access to or something like this. > It could also be useful in some other cases. Outside of the app, what can be already set up is an AFTER INSERT FOR EACH ROW trigger that essentially does: raise LOG, '%', NEW; The main drawback of this approach is that, for each line of data emitted to the log, there's another STATEMENT: copy... line added. But that might be not too bad for certain uses. Ideally we should be able to access the new rowset as a whole through a statement-level trigger. In that case, the data could be logged in a one-shot operation by that trigger. There's a related item in the TODO list: "Allow statement-level triggers to access modified rows" and an old thread on -hackers here: https://www.postgresql.org/message-id/20060522150647.ge24...@svana.org that discussed this topic in relation to MSSQL having this functionality. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- 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: Replace PostmasterRandom() with a stronger way of generating ran
Heikki Linnakangas writes: > I'm going to try implementing prngd support. It seems easy enough, and > prngd can be run on modern systems too, which is important for testing it. OK, if you feel like doing the work. However: > In addition to that, I'm going to see if we can make postmaster to work > sensibly without query cancel keys, if pg_strong_random() fails. This seems like a lot of work, with the "reward" that we'd start getting hard-to-debug reports about query cancel not working. Anytime anyone ever says "cancel didn't seem to work" we'd have to wonder whether there had been a transient failure of pg_strong_random. I think if we're going to refuse to generate weak cancel keys, we should just fail, not silently fall into a functionally degraded state. But in general, I think that being this picky about cancel keys on systems that are too old to have /dev/random is not really helpful to anybody. I don't recall any reports of anyone ever having a DOS situation from weak cancel keys. It's fine to upgrade our practice where it's convenient to do that, but taking away functionality on systems where it's not convenient isn't improving anyone's life. pg_crypto has a different set of tradeoffs and I don't necessarily make the same argument there. I don't feel that cancel keys have to act the same as pg_crypto keys. 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] Idempotency for all DDL statements
Hello everyone. This is my first time posting in -hackers, so please keep this in mind. I would like to formally request the addition of "IF EXISTS", "IF NOT EXISTS", "OR REPLACE", and any other CINE variants to DDL statements that currently do not feature an idempotent construct. The lack of an idempotent option in statements such as "CREATE TRIGGER" for example is particularly painful when performing upgrades on sizable schemas. I understand that there may be some difficulties in providing this to various database objects. That said, I believe IMHO that this would be a huge win for the community at large. Could I request DDL idempotency as an addition to the development roadmap? Thanks, Kenaniah
Re: [HACKERS] COPY as a set returning function
On Sun, Oct 16, 2016 at 9:01 AM, Craig Ringer wrote: > On 15 Oct. 2016 04:56, "Corey Huinker" wrote: > > > I would like to make COPY itself a SRF. That's a bit beyond my > capabilities, so if that is the route we want to go, I will need help. > > > > The syntax would probably look like this (new bits in bold): > > > >> WITH my_copy AS ( > >> COPY FROM 'example.csv' TO RESULT SET(c1 text, c2 integer, dummy1 > text, dummy2 text, c5 date) WITH (FORMAT CSV) > >> RETURNING c1, c2, c3 > >> ) > > Strong -1 from me on this approach. Our CTE implementation materializes > everything so this is no better than COPYing to a temp table. > > Not unless you plan to fix that (and figure out the backward compatibility > issues since the bug is documented as a feature) or implement RETURNING in > subqueries... I'd go for the function. > Well, it saves burning the oid and the pg_attribute rows. A few long running transactions can cause pg_attribute to bloat to 400GB on one of our systems - hence my wanting something like this function. If it does stay a function, we only need to implement 8 of the 12 options as parameters (FREEZE and FORCE* options don't apply). My guess is that future options added to COPY will be more about handling output or optimizing table inserts, neither of which mean more options for this proposed function. Would the best approach be to build in a core srf-returning function that might be deprecated once COPY is set-returning AND CTEs don't have to materialize, or to refactor what's in copy.c such that a contrib module can easily plug into it, and have copy_srf live there?
[HACKERS] Re: [COMMITTERS] pgsql: Replace PostmasterRandom() with a stronger way of generating ran
On 10/17/2016 06:21 PM, Tom Lane wrote:
Heikki Linnakangas writes:
On 10/17/2016 05:50 PM, Tom Lane wrote:
The real issue here is whether we are willing to say that
Postgres simply does not work anymore on machines without standard entropy
sources. Doesn't matter whether the user cares about the strength of
cancel keys, we're just blowing them off. That seems a bit extreme
from here. I think we should be willing to fall back to the old code
if we can't find a real entropy source.
I'm scared of having pg_strong_random() that is willing to fall back to
not-so-strong values. We can rename it, of course, but it seems
dangerous to use a weak random-number generator for authentication
purposes (query cancel, MD5 salts, SCRAM nonces).
I think that it's probably moot on all modern platforms, and even on
platforms as old as pademelon, the answer for people who care about
strong security is "--with-openssl". What I'm on about here is whether
we should make people who don't care about that jump through hoops.
Not caring is a perfectly reasonable stance for non-exposed postmasters;
otherwise we wouldn't have the "trust" auth method.
I would be satisfied with making it a non-default build option, eg
add this to pg_strong_random:
if (random_from_file("/dev/random", buf, len))
return true;
+#ifdef ALLOW_WEAK_SECURITY
+ ... old PostmasterRandom method here ...
+#endif
+
/* None of the sources were available. */
return false;
I also changed pgcrypto to use pg_strong_random(). Using a strong random
number generator is even more important for pgcrypto, since it might be
used for generating encryption keys. Would we allow a weak generator for
pgcrypto too, with ALLOW_WEAK_SECURITY? If we don't, then pgcrypto test
cases will still fail on pademelon. If we consider the option to be just
for testing, never for production use, then we could allow it, but it
feels a bit dangerous, and I'm not sure what's the point of testing a
configuration that you should never use in production.
I'm going to try implementing prngd support. It seems easy enough, and
prngd can be run on modern systems too, which is important for testing it.
In addition to that, I'm going to see if we can make postmaster to work
sensibly without query cancel keys, if pg_strong_random() fails. That
way, you could still run on platforms that don't have /dev/[u]random or
prngd, just without support for query cancels, MD5 authentication, and
pgcrypto key generation functions. I'd consider that similar to
--disable-spinlocks; it would be a helpful step when porting to a new
platform, but you wouldn't want to use it in production.
One more thing: in hindsight I think would be better to not fall back to
/dev/ random, if compiled with OpenSSL support. It really shouldn't
fail, and if it does, it seems better to complain loudly.
- Heikki
--
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] bit|varbit #, xor operator
Jim Nasby writes: > On 10/16/16 3:13 PM, Tom Lane wrote: >>> Related to this I'd also like to add a boolean XOR operator as that's a >>> relatively common request/question. >> We have boolean XOR; it's spelled "<>". > I always forget about that... Maybe it should be mentioned explicitly in the docs. > though, it doesn't work for boolean arrays. Define doesn't 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] bit|varbit #, xor operator
On 10/16/16 3:13 PM, Tom Lane wrote: As for counting bits in a bitstring, why do we have to make that an operator at all? Using a function would decrease the stress involved in choosing a name, and it's hard to believe that the requirement is so common that we need to shave a few keystrokes. But if you must have an operator there's not that much wrong with using prefix # for it. Fair enough. > Related to this I'd also like to add a boolean XOR operator as that's a > relatively common request/question. We have boolean XOR; it's spelled "<>". I always forget about that... though, it doesn't work for boolean arrays. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- 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: Replace PostmasterRandom() with a stronger way of generating ran
On Mon, Oct 17, 2016 at 8:21 AM, Tom Lane wrote: > Heikki Linnakangas writes: > > On 10/17/2016 05:50 PM, Tom Lane wrote: > >> The real issue here is whether we are willing to say that > >> Postgres simply does not work anymore on machines without standard > entropy > >> sources. Doesn't matter whether the user cares about the strength of > >> cancel keys, we're just blowing them off. That seems a bit extreme > >> from here. I think we should be willing to fall back to the old code > >> if we can't find a real entropy source. > > > I'm scared of having pg_strong_random() that is willing to fall back to > > not-so-strong values. We can rename it, of course, but it seems > > dangerous to use a weak random-number generator for authentication > > purposes (query cancel, MD5 salts, SCRAM nonces). > > I think that it's probably moot on all modern platforms, and even on > platforms as old as pademelon, the answer for people who care about > strong security is "--with-openssl". What I'm on about here is whether > we should make people who don't care about that jump through hoops. > Not caring is a perfectly reasonable stance for non-exposed postmasters; > otherwise we wouldn't have the "trust" auth method. > > I would be satisfied with making it a non-default build option, eg > add this to pg_strong_random: > +1 for that approach. I really wouldn't want to see it fall back completely transparently in case something stops working. But if it's a non-default build option, that's not a problem, and it should make it possible to make it work on older platforms. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [PATCH] Better logging of COPY queries if log_statement='all'
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Andrew Dunstan writes: > > On 10/17/2016 09:57 AM, Aleksander Alekseev wrote: > >> Sometimes it's useful to log content of files used in COPY ... TO ... and > >> COPY ... FROM ... queries. Unfortunately PostgreSQL doesn't allow to do > >> it, even if log_statement='all'. Suggested patch fixes this. > > > I'm not in favor of this, especially if it's not even optional. > > I'm not either. It sounds good when you're looking at toy examples, > but not when it means repeating gigabytes of COPY data into the log. This isn't new- I've seen many cases of people happily loading gigabytes of data via INSERT with log_statement='all' on. What I don't like is the idea of springing this change on people. * Aleksander Alekseev (a.aleks...@postgrespro.ru) wrote: > I understand your concern. Perhaps we could create and additional > parameter for enabling/disabling this feature or a new log_statement > value, or maybe both - i.e. rename log_statement and add a new possible > value? Ugh. Adding more options to log_statement is just an ugly route to go in, in my view. We really need a better solution here. > According to my colleagues it would be very nice to have this feature. > For instance, if you are trying to optimize PostgreSQL for application > that uses COPY and you don't have access to or something like this. > It could also be useful in some other cases. This use-case doesn't really make much sense to me. Can you explain it in more detail? Is the goal here to replicate all of the statements that are changing data in the database? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Replace PostmasterRandom() with a stronger way of generating ran
Heikki Linnakangas writes:
> On 10/17/2016 05:50 PM, Tom Lane wrote:
>> The real issue here is whether we are willing to say that
>> Postgres simply does not work anymore on machines without standard entropy
>> sources. Doesn't matter whether the user cares about the strength of
>> cancel keys, we're just blowing them off. That seems a bit extreme
>> from here. I think we should be willing to fall back to the old code
>> if we can't find a real entropy source.
> I'm scared of having pg_strong_random() that is willing to fall back to
> not-so-strong values. We can rename it, of course, but it seems
> dangerous to use a weak random-number generator for authentication
> purposes (query cancel, MD5 salts, SCRAM nonces).
I think that it's probably moot on all modern platforms, and even on
platforms as old as pademelon, the answer for people who care about
strong security is "--with-openssl". What I'm on about here is whether
we should make people who don't care about that jump through hoops.
Not caring is a perfectly reasonable stance for non-exposed postmasters;
otherwise we wouldn't have the "trust" auth method.
I would be satisfied with making it a non-default build option, eg
add this to pg_strong_random:
if (random_from_file("/dev/random", buf, len))
return true;
+#ifdef ALLOW_WEAK_SECURITY
+ ... old PostmasterRandom method here ...
+#endif
+
/* None of the sources were available. */
return false;
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: Replace PostmasterRandom() with a stronger way of generating ran
* Heikki Linnakangas (hlinn...@iki.fi) wrote: > On 10/17/2016 05:50 PM, Tom Lane wrote: > >Heikki Linnakangas writes: > >>Replace PostmasterRandom() with a stronger way of generating randomness. > > > >This patch broke padmeleon: > > > >016-10-17 09:57:17.782 EDT [5804d8bd.57c2:1] LOG: database system was shut > >down at 2016-10-17 09:57:17 EDT > >2016-10-17 09:57:17.790 EDT [5804d8bd.57c2:2] LOG: MultiXact member > >wraparound protections are now enabled > >2016-10-17 09:57:17.807 EDT [5804d8bd.57c6:1] LOG: autovacuum launcher > >started > >2016-10-17 09:57:17.820 EDT [5804d8bd.57bf:1] LOG: database system is ready > >to accept connections > >2016-10-17 09:57:18.516 EDT [5804d8bd.57bf:2] LOG: could not generate > >random query cancel key > >2016-10-17 09:57:19.544 EDT [5804d8bd.57bf:3] LOG: could not generate > >random query cancel key > >2016-10-17 09:57:20.563 EDT [5804d8bd.57bf:4] LOG: could not generate > >random query cancel key > >2016-10-17 09:57:21.584 EDT [5804d8bd.57bf:5] LOG: could not generate > >random query cancel key > >2016-10-17 09:57:22.604 EDT [5804d8bd.57bf:6] LOG: could not generate > >random query cancel key > > > >It's failing because this machine lacks /dev/random and /dev/urandom. > >It does have a non-kernel entropy daemon (prngd), which OpenSSL knows how > >to read from but the hard-wired code in pg_strong_random() does not. > > > >I'm not sure whether it's worth trying to make pg_strong_random() aware > >of prngd. > > Seems simple enough.. If it is, then that's certainly tempting, especially if that's the only platform we support where we don't have a better option, and we wish to continue supporting it. > > The real issue here is whether we are willing to say that > >Postgres simply does not work anymore on machines without standard entropy > >sources. Doesn't matter whether the user cares about the strength of > >cancel keys, we're just blowing them off. That seems a bit extreme > >from here. I think we should be willing to fall back to the old code > >if we can't find a real entropy source. > > I'm scared of having pg_strong_random() that is willing to fall back > to not-so-strong values. We can rename it, of course, but it seems > dangerous to use a weak random-number generator for authentication > purposes (query cancel, MD5 salts, SCRAM nonces). > > I think both for the MD5 salt and SCRAM, it doesn't have to be > unpredictable to attackers, as long the attacker cannot influence it > so that a particular value is chosen. For query cancel, it needs to > be unpredictable, but the query cancel key is quite short anyway, > and we haven't worried about it so far anyway, because an attacker > can't do very much damage by just canceling queries. > > One option would be to disable query-canceling and MD5 (and SCRAM) > authentication, if we don't have an entropy source. Wouldn't it be possible to make this a build-time question..? I don't particularly like the idea of pg_strong_random() falling back to not-strong values, but maybe we could have a '--use-weak-random' configure switch for platforms that don't provide a strong source. Then again, if we wish to support this platform, then perhaps we should put in the effort to support prngd. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Better logging of COPY queries if log_statement='all'
> > I'm not in favor of this, especially if it's not even optional. > > I'm not either. It sounds good when you're looking at toy examples, > but not when it means repeating gigabytes of COPY data into the log. I understand your concern. Perhaps we could create and additional parameter for enabling/disabling this feature or a new log_statement value, or maybe both - i.e. rename log_statement and add a new possible value? According to my colleagues it would be very nice to have this feature. For instance, if you are trying to optimize PostgreSQL for application that uses COPY and you don't have access to or something like this. It could also be useful in some other cases. This feature is very simple and easy to maintain. I'm sure we could find a solution that will make happy both developers and users. -- Best regards, Aleksander Alekseev signature.asc Description: PGP signature
[HACKERS] Re: [COMMITTERS] pgsql: Replace PostmasterRandom() with a stronger way of generating ran
On 10/17/2016 05:50 PM, Tom Lane wrote: Heikki Linnakangas writes: Replace PostmasterRandom() with a stronger way of generating randomness. This patch broke padmeleon: 016-10-17 09:57:17.782 EDT [5804d8bd.57c2:1] LOG: database system was shut down at 2016-10-17 09:57:17 EDT 2016-10-17 09:57:17.790 EDT [5804d8bd.57c2:2] LOG: MultiXact member wraparound protections are now enabled 2016-10-17 09:57:17.807 EDT [5804d8bd.57c6:1] LOG: autovacuum launcher started 2016-10-17 09:57:17.820 EDT [5804d8bd.57bf:1] LOG: database system is ready to accept connections 2016-10-17 09:57:18.516 EDT [5804d8bd.57bf:2] LOG: could not generate random query cancel key 2016-10-17 09:57:19.544 EDT [5804d8bd.57bf:3] LOG: could not generate random query cancel key 2016-10-17 09:57:20.563 EDT [5804d8bd.57bf:4] LOG: could not generate random query cancel key 2016-10-17 09:57:21.584 EDT [5804d8bd.57bf:5] LOG: could not generate random query cancel key 2016-10-17 09:57:22.604 EDT [5804d8bd.57bf:6] LOG: could not generate random query cancel key It's failing because this machine lacks /dev/random and /dev/urandom. It does have a non-kernel entropy daemon (prngd), which OpenSSL knows how to read from but the hard-wired code in pg_strong_random() does not. I'm not sure whether it's worth trying to make pg_strong_random() aware of prngd. Seems simple enough.. The real issue here is whether we are willing to say that Postgres simply does not work anymore on machines without standard entropy sources. Doesn't matter whether the user cares about the strength of cancel keys, we're just blowing them off. That seems a bit extreme from here. I think we should be willing to fall back to the old code if we can't find a real entropy source. I'm scared of having pg_strong_random() that is willing to fall back to not-so-strong values. We can rename it, of course, but it seems dangerous to use a weak random-number generator for authentication purposes (query cancel, MD5 salts, SCRAM nonces). I think both for the MD5 salt and SCRAM, it doesn't have to be unpredictable to attackers, as long the attacker cannot influence it so that a particular value is chosen. For query cancel, it needs to be unpredictable, but the query cancel key is quite short anyway, and we haven't worried about it so far anyway, because an attacker can't do very much damage by just canceling queries. One option would be to disable query-canceling and MD5 (and SCRAM) authentication, if we don't have an entropy source. - Heikki -- 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: Replace PostmasterRandom() with a stronger way of generating ran
On Mon, Oct 17, 2016 at 10:50 AM, Tom Lane wrote: > The real issue here is whether we are willing to say that > Postgres simply does not work anymore on machines without standard entropy > sources. Doesn't matter whether the user cares about the strength of > cancel keys, we're just blowing them off. That seems a bit extreme > from here. I think we should be willing to fall back to the old code > if we can't find a real entropy source. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support
> > The current macaddr datatype needs to be kept for some time by renaming > it without changing OID and use the newer one for further usage. > >From the point of view of a driver maintainer... Npgsql looks up data types by their name - upon first connection to a database it queries pg_type and maps its internal data type handlers based on name. This allows it to avoid hardcoding data type OIDs in source code, and easily handle user-defined data types as well (enums, composites...). So a sudden rename of a datatype would definitely cause a problem. Of course it's possible to first check the server version and act accordingly but it seems to complicate things needlessly.
Re: [HACKERS] [PATCH] Better logging of COPY queries if log_statement='all'
Andrew Dunstan writes: > On 10/17/2016 09:57 AM, Aleksander Alekseev wrote: >> Sometimes it's useful to log content of files used in COPY ... TO ... and >> COPY ... FROM ... queries. Unfortunately PostgreSQL doesn't allow to do >> it, even if log_statement='all'. Suggested patch fixes this. > I'm not in favor of this, especially if it's not even optional. I'm not either. It sounds good when you're looking at toy examples, but not when it means repeating gigabytes of COPY data into the log. 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] Add PGDLLEXPORT to PG_FUNCTION_INFO_V1
I wrote: > But I'd understand if you think that this is too much code churn for too > little > benefit, even if it could be considered a clean-up. > > In that case, I'd argue that in the sample in doc/src/sgml/xfunc.sgml > the function definitions should be changed to read > > PGDLLEXPORT Datum foo(PG_FUNCTION_ARGS) > > instead of > > Datum foo(PG_FUNCTION_ARGS) > > because without that the sample fails if you try to build it with MSVC > like the stackoverflow question did. Since I didn't hear from you, I assume that you are not in favour of removing the SQL function declarations from contrib. So I went ahead and wrote a patch to add PGDLLEXPORT to the C function sample. While at it, I noticed that there are no instructions for building and linking C functions with MSVC, so I have added a patch for that as well. Yours, Laurenz Albe 0001-Add-PGDLLEXPORT-to-sample-C-function.patch Description: 0001-Add-PGDLLEXPORT-to-sample-C-function.patch 0002-Add-instructions-for-building-C-functions-with-MSVC.patch Description: 0002-Add-instructions-for-building-C-functions-with-MSVC.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Replace PostmasterRandom() with a stronger way of generating ran
Heikki Linnakangas writes: > Replace PostmasterRandom() with a stronger way of generating randomness. This patch broke padmeleon: 016-10-17 09:57:17.782 EDT [5804d8bd.57c2:1] LOG: database system was shut down at 2016-10-17 09:57:17 EDT 2016-10-17 09:57:17.790 EDT [5804d8bd.57c2:2] LOG: MultiXact member wraparound protections are now enabled 2016-10-17 09:57:17.807 EDT [5804d8bd.57c6:1] LOG: autovacuum launcher started 2016-10-17 09:57:17.820 EDT [5804d8bd.57bf:1] LOG: database system is ready to accept connections 2016-10-17 09:57:18.516 EDT [5804d8bd.57bf:2] LOG: could not generate random query cancel key 2016-10-17 09:57:19.544 EDT [5804d8bd.57bf:3] LOG: could not generate random query cancel key 2016-10-17 09:57:20.563 EDT [5804d8bd.57bf:4] LOG: could not generate random query cancel key 2016-10-17 09:57:21.584 EDT [5804d8bd.57bf:5] LOG: could not generate random query cancel key 2016-10-17 09:57:22.604 EDT [5804d8bd.57bf:6] LOG: could not generate random query cancel key It's failing because this machine lacks /dev/random and /dev/urandom. It does have a non-kernel entropy daemon (prngd), which OpenSSL knows how to read from but the hard-wired code in pg_strong_random() does not. I'm not sure whether it's worth trying to make pg_strong_random() aware of prngd. The real issue here is whether we are willing to say that Postgres simply does not work anymore on machines without standard entropy sources. Doesn't matter whether the user cares about the strength of cancel keys, we're just blowing them off. That seems a bit extreme from here. I think we should be willing to fall back to the old code if we can't find a real entropy source. 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] Password identifiers, protocol aging and SCRAM protocol
On 10/17/2016 12:27 PM, Heikki Linnakangas wrote: On 10/17/2016 12:18 PM, Michael Paquier wrote: You removed the part of pgcrypto in charge of randomness, nice move. I was wondering about how to do with the perfc and the unix_std at some point, and ripping them off as you did is fine for me. Yeah. I didn't understand the need for the perfc stuff. Are there Windows systems that don't have the Crypto APIs? I doubt it, but the buildfarm will tell us in a moment if there are. And if we don't have a good source of randomness like /dev/random, I think it's better to fail, than try to collect entropy ourselves (which is what unix_std did). If there's a platform where that doesn't work, someone will hopefully send us a patch, rather than silently fall back to an iffy implementation. Looks like Tom's old HP-UX box, pademelon, is not happy about this. Does (that version of) HP-UX not have /dev/urandom? I think we're going to need a bit more logging if no randomness source is available. What we have now is just "could not generate random query cancel key", which isn't very informative. Perhaps we should also call pg_strong_random() once at postmaster startup, to check that it works, instead of starting up but not accepting any connections. - Heikki -- 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 EVP API pgcrypto encryption, dropping support for OpenSSL 0.9.6 and older
Committed this patch now. - Heikki -- 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] Better logging of COPY queries if log_statement='all'
On 10/17/2016 09:57 AM, Aleksander Alekseev wrote: Hello. Sometimes it's useful to log content of files used in COPY ... TO ... and COPY ... FROM ... queries. Unfortunately PostgreSQL doesn't allow to do it, even if log_statement='all'. Suggested patch fixes this. Log example: ``` LOG: statement: create table test (k int, v text); LOG: statement: insert into test values (111, 'aaa'), (222, 'bbb'); LOG: statement: copy test to '/tmp/copy.txt'; LOG: statement: 111aaa LOG: statement: 222bbb LOG: statement: delete from test; LOG: statement: copy test from '/tmp/copy.txt'; LOG: statement: 111aaa LOG: statement: 222bbb ``` I'm not in favor of this, especially if it's not even optional. log_statement is about logging, er, statements, not logging data. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Better logging of COPY queries if log_statement='all'
Hello.
Sometimes it's useful to log content of files used in COPY ... TO ... and
COPY ... FROM ... queries. Unfortunately PostgreSQL doesn't allow to do
it, even if log_statement='all'. Suggested patch fixes this.
Log example:
```
LOG: statement: create table test (k int, v text);
LOG: statement: insert into test values (111, 'aaa'), (222, 'bbb');
LOG: statement: copy test to '/tmp/copy.txt';
LOG: statement: 111aaa
LOG: statement: 222bbb
LOG: statement: delete from test;
LOG: statement: copy test from '/tmp/copy.txt';
LOG: statement: 111aaa
LOG: statement: 222bbb
```
--
Best regards,
Aleksander Alekseev
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 5947e72..82b3a18 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -331,6 +331,38 @@ static bool CopyGetInt16(CopyState cstate, int16 *val);
/*
+ * Logs file content during COPY ... FROM / COPY ... TO execution if
+ * log_statement = 'all'.
+ */
+static void
+CopyLogStatement(const char* str, bool flush)
+{
+ static StringInfo logString = NULL;
+
+ if(log_statement != LOGSTMT_ALL)
+ return;
+
+ if(logString == NULL)
+ {
+ MemoryContext oldctx = MemoryContextSwitchTo(TopMemoryContext);
+ logString = makeStringInfo();
+ MemoryContextSwitchTo(oldctx);
+ }
+
+ appendStringInfoString(logString, str);
+
+ if(flush)
+ {
+ ereport(LOG,
+(errmsg("statement: %s", logString->data),
+ errhidestmt(true),
+ errhidecontext(true)));
+
+ resetStringInfo(logString);
+ }
+}
+
+/*
* Send copy start/stop messages for frontend copies. These have changed
* in past protocol redesigns.
*/
@@ -2045,14 +2077,20 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
if (!cstate->binary)
{
if (need_delim)
+ {
CopySendChar(cstate, cstate->delim[0]);
+CopyLogStatement(cstate->delim, false);
+ }
need_delim = true;
}
if (isnull)
{
if (!cstate->binary)
+ {
CopySendString(cstate, cstate->null_print_client);
+CopyLogStatement(cstate->null_print_client, false);
+ }
else
CopySendInt32(cstate, -1);
}
@@ -2062,6 +2100,9 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
{
string = OutputFunctionCall(&out_functions[attnum - 1],
value);
+
+CopyLogStatement(string, false);
+
if (cstate->csv_mode)
CopyAttributeOutCSV(cstate, string,
cstate->force_quote_flags[attnum - 1],
@@ -2083,6 +2124,7 @@ CopyOneRowTo(CopyState cstate, Oid tupleOid, Datum *values, bool *nulls)
}
CopySendEndOfRow(cstate);
+ CopyLogStatement("", true);
MemoryContextSwitchTo(oldcontext);
}
@@ -2914,6 +2956,8 @@ NextCopyFromRawFields(CopyState cstate, char ***fields, int *nfields)
if (done && cstate->line_buf.len == 0)
return false;
+ CopyLogStatement(cstate->line_buf.data, true);
+
/* Parse the line into de-escaped field values */
if (cstate->csv_mode)
fldct = CopyReadAttributesCSV(cstate);
signature.asc
Description: PGP signature
Re: [HACKERS] postgres_fdw super user checks
On Mon, Oct 17, 2016 at 2:18 AM, Michael Paquier wrote: > On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes wrote: >> postgres_fdw has some checks to enforce that non-superusers must connect to >> the foreign server with a password-based method. The reason for this is to >> prevent the authentication to the foreign server from happening on the basis >> of the OS user who is running the non-foreign server. >> >> But I think these super user checks should be run against the userid of the >> USER MAPPING being used for the connection, not the userid of currently >> logged on user. > > So, if the user mapping user is a superuser locally, this would allow > any lambda user of the local server to attempt a connection to the > remote server. It looks dangerous rather dangerous to me to authorize > that, even if the current behavior is a bit inconsistent I agree. I don't know what "any lambda user" means. Did you mean to write "any random user"? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Gather Merge
On Mon, Oct 17, 2016 at 4:56 AM, Amit Kapila wrote: > + node->nreaders < 2) ... > I see there are small discrepancies in both the codes like I don't see > the use of single_copy flag, as it is present in gather node. single_copy doesn't make sense for GatherMerge, because the whole point is to merge a bunch of individually-sorted output streams into a single stream. If you have only one stream of tuples, you don't need to merge anything: you could have just used Gather for that. It does, however, make sense to merge one worker's output with the leader's output. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Question about behavior of snapshot too old feature
On Sun, Oct 16, 2016 at 9:26 PM, Masahiko Sawada wrote: > When I set old_snapshot_threshold = 0 I got error at step #3, which > means that the error is occurred without table pruning. The "snapshot too old" error can happen without pruning, but only because there is no way to tell the difference between a page that has been pruned since the snapshot was taken and a page which has had some other kind of modification since the snapshot was taken. Ignoring false positives for a moment (where the page is updated by something other than pruning), what is required for early pruning is that the snapshot has expired (which due to "rounding" and avoidance of locking could easily take up to a minute or two more than the old_snapshot_threshold setting) and then there is page pruning due to a vacuum or just HOT pruning from a page read. At some point after that, a read which is part of returning data to the user (e.g., not just positioning for index modification) can see that the snapshot is too old and that the LSN for the page is past the snapshot LSN. That is when you get the error. > We have regression test for this feature but it sets > old_snapshot_threshold = 0, I doubt about we can test it properly. > Am I missing something? This is a hard feature to test properly, and certainly hard to test without the test running for a long time. The zero setting is really not intended to be used in production, but only to allow some half-way decent testing that doesn't take extreme lengths of time. If you add some delays of a few minutes each at key points in a test, you should be able to get a test that works with a setting of 1min. It is not impossible that we might need to add a memory barrier to one or two places to get such tests to behave consistently, but I have not been able to spot where, if anywhere, that would be. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)
On Thu, Oct 13, 2016 at 12:35 AM, Peter Geoghegan wrote:
> On Wed, Oct 12, 2016 at 11:09 AM, Robert Haas wrote:
>
>> On the flip side, what if anything can queries hope to get out of
>> parallel sort that they can't get out of Gather Merge? One
>> possibility is that a parallel sort might end up being substantially
>> faster than Gather Merge-over-non-parallel sort. In that case, we
>> obviously should prefer it.
>
> I must admit that I don't know enough about it to comment just yet.
> Offhand, it occurs to me that the Gather Merge sorted input could come
> from a number of different types of paths/nodes, whereas adopting what
> I've done here could only work more or less equivalently to "Gather
> Merge -> Sort -> Seq Scan" -- a special case, really.
>
>> For example, it's possible that you might want to have all
>> workers participate in sorting some data and then divide the result of
>> the sort into equal ranges that are again divided among the workers,
>> or that you might want all workers to sort and then each worker to
>> read a complete copy of the output data. But these don't seem like
>> particularly mainstream needs, nor do they necessarily seem like
>> problems that parallel sort itself should be trying to solve.
>
> This project of mine is about parallelizing tuplesort.c, which isn't
> really what you want for parallel query -- you shouldn't try to scope
> the problem as "make the sort more scalable using parallelism" there.
> Rather, you want to scope it at "make the execution of the entire
> query more scalable using parallelism", which is really quite a
> different thing, which necessarily involves the executor having direct
> knowledge of partition boundaries.
>
Okay, but what is the proof or why do you think second is going to
better than first? One thing which strikes as a major difference
between your approach and Gather Merge is that in your approach leader
has to wait till all the workers have done with their work on sorting
whereas with Gather Merge as soon as first one is done, leader starts
with merging. I could be wrong here, but if I understood it
correctly, then there is a argument that Gather Merge kind of approach
can win in cases where some of the workers can produce sorted outputs
ahead of others and I am not sure if we can dismiss such cases.
+struct Sharedsort
+{
..
+ * Workers increment workersFinished to indicate having finished. If
+ * this is equal to state.launched within the leader, leader is ready
+ * to merge runs.
+ *
+ * leaderDone indicates if leader is completely done (i.e., was
+ * tuplesort_end called against the state through which parallel output
+ * was consumed?)
+ */
+ int currentWorker;
+ int workersFinished;
..
}
By looking at 'workersFinished' usage, it looks like you have devised
a new way for leader to know when workers have finished which might be
required for this patch. However, have you tried to use or
investigate if existing infrastructure which serves same purpose could
be used for it?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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] make coverage-html on OS X
On 10/14/16 11:50 AM, Jim Nasby wrote: > On 10/13/16 11:13 PM, Peter Eisentraut wrote: >> On 10/13/16 4:03 PM, Jim Nasby wrote: >>> I've slowly stripped away my normal build environment; I'm now using >>> baseline gcc (no ccache) with the following ./configure... anyone have >>> any ideas how to debug/fix this? >> >> Which gcc are you using? > > gcc --version > Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr > --with-gxx-include-dir=/usr/include/c++/4.2.1 > Apple LLVM version 8.0.0 (clang-800.0.38) I would try it with something that is not the system compiler. For example, a gcc build from Homebrew. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] postgres_fdw : altering foreign table not invalidating prepare statement execution plan.
On Fri, Oct 7, 2016 at 7:20 AM, Etsuro Fujita wrote: > On 2016/10/07 10:26, Amit Langote wrote: >> >> On 2016/10/06 21:55, Etsuro Fujita wrote: >>> >>> On 2016/10/06 20:17, Amit Langote wrote: On 2016/10/05 20:45, Etsuro Fujita wrote: > > >>> I noticed that we were wrong. Your patch was modified so that >>> dependencies on FDW-related objects would be extracted from a given plan >>> in create_foreignscan_plan (by Ashutosh) and then in >>> set_foreignscan_references by me, but that wouldn't work well for INSERT >>> cases. To fix that, I'd like to propose that we collect the dependencies >>> from the given rte in add_rte_to_flat_rtable, instead. > > >> I see. So, doing it from set_foreignscan_references() fails to capture >> plan dependencies in case of INSERT because it won't be invoked at all >> unlike the UPDATE/DELETE case. > > > Right. > > If some writable FDW consulted foreign table/server/FDW options in > AddForeignUpdateTarget, which adds the extra junk columns for > UPDATE/DELETE to the targetList of the given query tree, the rewritten > query tree would also need to be invalidated. But I don't think such > an > FDW really exists because that routine in a practical FDW wouldn't > change > such columns depending on those options. > > >>> I had second thoughts about that; since the possibility wouldn't be zero, >>> I added to extract_query_dependencies_walker the same code I added to >>> add_rte_to_flat_rtable. > > >> And here, since AddForeignUpdateTargets() could possibly utilize foreign >> options which would cause *query tree* dependencies. It's possible that >> add_rte_to_flat_rtable may not be called before an option change, causing >> invalidation of any cached objects created based on the changed options. >> So, must record dependencies from extract_query_dependencies as well. > > > Right. > >> I think this (v4) patch is in the best shape so far. +1, except one small change. The comment "... On relcache invalidation events or the relevant syscache invalidation events, ..." specifies relcache separately. I think, it should either needs to specify all the syscaches or just mention "On corresponding syscache invalidation events, ...". Attached patch uses the later version. Let me know if this looks good. If you are fine, I think we should mark this as "Ready for committer". The patch compiles cleanly, and make check in regress and that in postgres_fdw shows no failures. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company foreign_plan_cache_inval_v5.patch Description: invalid/octet-stream -- 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] FSM corruption leading to errors
On 10/10/2016 05:25 PM, Michael Paquier wrote:
On Fri, Oct 7, 2016 at 2:59 AM, Pavan Deolasee wrote:
I believe the fix is very simple. The FSM change during truncation is
critical and the buffer must be marked by MarkBufferDirty() i.e. those
changes must make to the disk. I think it's alright not to WAL log them
because XLOG_SMGR_TRUNCATE will redo() them if a crash occurs. But it must
not be lost across a checkpoint. Also, since it happens only during relation
truncation, I don't see any problem from performance perspective.
Agreed. I happen to notice that VM is similalry careful when it comes
to truncate it (visibilitymap_truncate).
visibilitymap_truncate is actually also wrong, in a different way. The
truncation WAL record is written only after the VM (and FSM) are
truncated. But visibilitymap_truncate() has already modified and dirtied
the page. If the VM page change is flushed to disk before the WAL
record, and you crash, you might have a torn VM page and a checksum failure.
Simply replacing the MarkBufferDirtyHint() call with MarkBufferDirty()
in FreeSpaceMapTruncateRel would have the same issue. If you call
MarkBufferDirty(), you must WAL-log the change, and also set the page's
LSN to make sure the WAL record is flushed first.
I think we need something like the attached.
- Heikki
>From 7b100e951236c21656eb7bd63b4631c0c32cc573 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas
Date: Mon, 17 Oct 2016 14:02:52 +0300
Subject: [PATCH 1/1] WIP: Fix "FSM corruption leading to errors"
---
src/backend/access/heap/visibilitymap.c | 9 ++-
src/backend/catalog/storage.c | 110 ++
src/backend/storage/freespace/freespace.c | 14 +++-
src/include/access/visibilitymap.h| 2 +-
src/include/storage/freespace.h | 3 +-
src/test/recovery/t/008_fsm_check.pl | 90
6 files changed, 179 insertions(+), 49 deletions(-)
create mode 100644 src/test/recovery/t/008_fsm_check.pl
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 3ad4a9f..377e511 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -457,9 +457,14 @@ visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_fro
* before they access the VM again.
*
* nheapblocks is the new size of the heap.
+ *
+ * The caller must have WAL-logged the truncation already (if the relation
+ * needs WAL-logging at all). 'lsn' is the LSN of the XLOG record. It is
+ * used to stamp the last remaining VM page, so that it doesn't get flushed
+ * to disk before the WAL record.
*/
void
-visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
+visibilitymap_truncate(Relation rel, BlockNumber nheapblocks, XLogRecPtr lsn)
{
BlockNumber newnblocks;
@@ -524,6 +529,8 @@ visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
map[truncByte] &= (1 << truncOffset) - 1;
MarkBufferDirty(mapBuffer);
+ if (lsn != InvalidXLogRecPtr)
+ PageSetLSN(page, lsn);
UnlockReleaseBuffer(mapBuffer);
}
else
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 0d8311c..5478953 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -28,6 +28,7 @@
#include "catalog/storage.h"
#include "catalog/storage_xlog.h"
#include "storage/freespace.h"
+#include "storage/proc.h"
#include "storage/smgr.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -228,6 +229,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
{
bool fsm;
bool vm;
+ XLogRecPtr lsn = InvalidXLogRecPtr;
/* Open it at the smgr level if not already done */
RelationOpenSmgr(rel);
@@ -239,56 +241,76 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
rel->rd_smgr->smgr_fsm_nblocks = InvalidBlockNumber;
rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
- /* Truncate the FSM first if it exists */
- fsm = smgrexists(rel->rd_smgr, FSM_FORKNUM);
- if (fsm)
- FreeSpaceMapTruncateRel(rel, nblocks);
-
- /* Truncate the visibility map too if it exists. */
- vm = smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
- if (vm)
- visibilitymap_truncate(rel, nblocks);
-
- /*
- * We WAL-log the truncation before actually truncating, which means
- * trouble if the truncation fails. If we then crash, the WAL replay
- * likely isn't going to succeed in the truncation either, and cause a
- * PANIC. It's tempting to put a critical section here, but that cure
- * would be worse than the disease. It would turn a usually harmless
- * failure to truncate, that might spell trouble at WAL replay, into a
- * certain PANIC.
- */
- if (RelationNeedsWAL(rel))
+ PG_TRY();
{
/*
- * Make an XLOG entry reporting the file truncation.
+ * We WAL-log the truncation before actually truncating, which means
+ * trouble if the truncation fails. If we then crash, the WAL replay
+ * likely isn't going to succeed in the
Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol
On 10/17/2016 12:18 PM, Michael Paquier wrote: You removed the part of pgcrypto in charge of randomness, nice move. I was wondering about how to do with the perfc and the unix_std at some point, and ripping them off as you did is fine for me. Yeah. I didn't understand the need for the perfc stuff. Are there Windows systems that don't have the Crypto APIs? I doubt it, but the buildfarm will tell us in a moment if there are. And if we don't have a good source of randomness like /dev/random, I think it's better to fail, than try to collect entropy ourselves (which is what unix_std did). If there's a platform where that doesn't work, someone will hopefully send us a patch, rather than silently fall back to an iffy implementation. - Heikki -- 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] Password identifiers, protocol aging and SCRAM protocol
On Mon, Oct 17, 2016 at 5:55 PM, Heikki Linnakangas wrote: > On 10/15/2016 04:26 PM, Michael Paquier wrote: >>> >>> * Now that we don't call random() in postmaster anymore, is there any >>> point >>> in calling srandom() there (i.e. where the above incorrect comment was)? >>> Should we remove it? random() might be used by pre-loaded extensions, >>> though. (Hopefully not for cryptographic purposes.) >> >> >> That's the business of the maintainers such modules, so my heart is >> telling me to rip it off, but my mind tells me that there is no point >> in making them unhappy either if they rely on it. I'd trust my mind on >> this one, other opinions are welcome. > > > I kept it for now. Doesn't do any harm either, even if it's unnecessary. > >>> * Should we backport this? Sorry if we discussed that already, but I >>> don't >>> remember. >> >> >> I think that we discussed quickly the point at last PGCon during the >> SCRAM-committee-unofficial meeting, and that we talked about doing >> that only for HEAD. > > > Ok, committed to HEAD. You removed the part of pgcrypto in charge of randomness, nice move. I was wondering about how to do with the perfc and the unix_std at some point, and ripping them off as you did is fine for me. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Gather Merge
On Wed, Oct 5, 2016 at 11:35 AM, Rushabh Lathia
wrote:
> Hi hackers,
>
> Attached is the patch to implement Gather Merge.
>
Couple of review comments:
1.
ExecGatherMerge()
{
..
+ /* No workers? Then never mind. */
+ if (!got_any_worker
||
+ node->nreaders < 2)
+ {
+
ExecShutdownGatherMergeWorkers(node);
+ node->nreaders = 0;
+
}
Are you planning to restrict the use of gather merge based on number
of workers, if there is a valid reason, then I think comments should
be updated for same?
2.
+ExecGatherMerge(GatherMergeState * node){
..
+ if (!node->initialized)
+ {
+ EState *estate = node->ps.state;
+
GatherMerge *gm = (GatherMerge *) node->ps.plan;
+
+ /*
+ * Sometimes we
might have to run without parallelism; but if parallel
+ * mode is active then we can try to
fire up some workers.
+ */
+ if (gm->num_workers > 0 && IsInParallelMode())
+
{
+ ParallelContext *pcxt;
+ bool got_any_worker =
false;
+
+ /* Initialize the workers required to execute Gather node. */
+
if (!node->pei)
+ node->pei = ExecInitParallelPlan(node-
>ps.lefttree,
+
estate,
+
gm->num_workers);
..
}
There is lot of common code between ExecGatherMerge and ExecGather.
Do you think it makes sense to have a common function to avoid the
duplicity?
I see there are small discrepancies in both the codes like I don't see
the use of single_copy flag, as it is present in gather node.
3.
+gather_merge_readnext(GatherMergeState * gm_state, int reader, bool force)
{
..
+ tup = gm_readnext_tuple(gm_state, reader, force, NULL);
+
+ /*
+
* try to read more tuple into nowait mode and store it into the tuple
+ * array.
+
*/
+ if (HeapTupleIsValid(tup))
+ fill_tuple_array(gm_state, reader);
How the above read tuple is stored in array? In anycase the above
interface seems slightly awkward to me. Basically, I think what you
are trying to do here is after reading first tuple in waitmode, you
are trying to fill the array by reading more tuples. So, can't we
push reading of this fist tuple into that function and name it as
form_tuple_array().
4.
+create_gather_merge_path(..)
{
..
+ 0 /* FIXME: pathnode->limit_tuples*/);
What exactly you want to fix in above code?
5.
+/* Tuple array size */
+#define MAX_TUPLE_STORE 10
Have you tried with other values of MAX_TUPLE_STORE? If yes, then
what are the results? I think it is better to add a comment why array
size is best for performance.
6.
+/* INTERFACE ROUTINES
+ * ExecInitGatherMerge - initialize the MergeAppend
node
+ * ExecGatherMerge - retrieve the next tuple from the node
+ *
ExecEndGatherMerge - shut down the MergeAppend node
+ *
ExecReScanGatherMerge - rescan the MergeAppend node
typo. /MergeAppend/GatherMerge
7.
+static TupleTableSlot *gather_merge_getnext(GatherMergeState * gm_state);
+static HeapTuple
gm_readnext_tuple(GatherMergeState * gm_state, int nreader, bool
force, bool *done);
Formatting near GatherMergeState doesn't seem to be appropriate. I
think you need to add GatherMergeState in typedefs.list and then run
pgindent again.
8.
+ /*
+ * Initialize funnel slot to same tuple descriptor as outer plan.
+ */
+ if
(!ExecContextForcesOids(&gm_state->ps, &hasoid))
I think in above comment, you mean Initialize GatherMerge slot.
9.
+ /* Does tuple array have any avaiable tuples? */
/avaiable/available
>
> Open Issue:
>
> - Commit af33039317ddc4a0e38a02e2255c2bf453115fd2 fixed the leak into
> tqueue.c by
> calling gather_readnext() into per-tuple context. Now for gather merge that
> is
> not possible, as we storing the tuple into Tuple array and we want tuple to
> be
> free only its get pass through the merge sort algorithm. One idea is, we can
> also call gm_readnext_tuple() under per-tuple context (which will fix the
> leak
> into tqueue.c) and then store the copy of tuple into tuple array.
>
Won't extra copy per tuple impact performance? Is the fix in
mentioned commit was for record or composite types, if so, does
GatherMerge support such types and if it support, does it provide any
benefit over Gather?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.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] Password identifiers, protocol aging and SCRAM protocol
On 10/15/2016 04:26 PM, Michael Paquier wrote: * Now that we don't call random() in postmaster anymore, is there any point in calling srandom() there (i.e. where the above incorrect comment was)? Should we remove it? random() might be used by pre-loaded extensions, though. (Hopefully not for cryptographic purposes.) That's the business of the maintainers such modules, so my heart is telling me to rip it off, but my mind tells me that there is no point in making them unhappy either if they rely on it. I'd trust my mind on this one, other opinions are welcome. I kept it for now. Doesn't do any harm either, even if it's unnecessary. * Should we backport this? Sorry if we discussed that already, but I don't remember. I think that we discussed quickly the point at last PGCon during the SCRAM-committee-unofficial meeting, and that we talked about doing that only for HEAD. Ok, committed to HEAD. Thanks! - Heikki -- 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] postgres_fdw super user checks
On Mon, Oct 17, 2016 at 11:48 AM, Michael Paquier wrote: > On Mon, Oct 17, 2016 at 3:33 AM, Jeff Janes wrote: >> postgres_fdw has some checks to enforce that non-superusers must connect to >> the foreign server with a password-based method. The reason for this is to >> prevent the authentication to the foreign server from happening on the basis >> of the OS user who is running the non-foreign server. >> >> But I think these super user checks should be run against the userid of the >> USER MAPPING being used for the connection, not the userid of currently >> logged on user. > > So, if the user mapping user is a superuser locally, this would allow > any lambda user of the local server to attempt a connection to the > remote server. It looks dangerous rather dangerous to me to authorize > that, even if the current behavior is a bit inconsistent I agree. A lambda user can use a user mapping same as a superuser if a. that user mapping is public and/or b. it uses a view owned by super user (RangeTblEntry::checkuser). When a is true but not b, the the user in UserMapping is set to lambda and not superuser, so this patch is correct here. If b is true, and lambda is able to access the view, the superuser has granted it permissions to do so and thus intends to let lambda use a super user user mapping. Since we trust super users to do the right thing, I don't see why it's unsafe. Any other objects accesses by lambda, will use a different user mapping based on the object being accessed. Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimization for lazy_scan_heap
On Mon, Oct 3, 2016 at 10:59 AM, Michael Paquier wrote: > On Mon, Sep 26, 2016 at 5:26 AM, Rahila Syed wrote: >> Some initial comments on optimize_lazy_scan_heap_v2.patch. > > Seems worth pursuing. Marking as returned with feedback because of > lack of activity and some basic reviews sent. Thank you for reviewing this patch, and sorry for my late reply. I've measured the performance improvement of this patch, but I got the result showing that it can improve vacuum freeze performance 30 sec with 32TB table. I don't think that this patch is worth to pursue any further, so I'd like to withdraw this. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- 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] Transactions involving multiple postgres foreign servers
On Thu, Oct 13, 2016 at 7:37 PM, Ashutosh Bapat wrote: >>> >>> If we are successful in COMMITTING foreign transactions during >>> post-commit phase, COMMIT message will be returned after we have >>> committed all foreign transactions. But in case we can not reach a >>> foreign server, and request times out, we can not revert back our >>> decision that we are going to commit the transaction. That's my answer >>> to the timeout based heuristic. >> >> IIUC 2PC is the protocol that assumes that all of the foreign server live. > > Do you have any references? Take a look at [1]. The first paragraph > itself mentions that 2PC can achieve its goals despite temporary > failures. I guess that It doesn't mention that 2PC can it by ignoring temporary failures. Even by waiting for the crashed server revives, 2PC can achieve its goals. >> In case we can not reach a foreign server during post-commit phase, >> basically the transaction and following transaction should stop until >> the crashed server revived. > > I have repeatedly given reasons why this is not correct. You and Amit > seem to repeat this statement again and again in turns without giving > any concrete reasons about why this is so. > >> This is the first place to implement 2PC >> for FDW, I think. The heuristically determination approach I mentioned >> is one of the optimization idea to avoid holding up transaction in >> case a foreign server crashed. >> >>> I don't see much point in holding up post-commit processing for a >>> non-responsive foreign server, which may not respond for days >>> together. Can you please elaborate a use case? Which commercial >>> transaction manager does that? >> >> For example, the client updates a data on foreign server and then >> commits. And the next transaction from the same client selects new >> data which was updated on previous transaction. In this case, because >> the first transaction is committed the second transaction should be >> able to see updated data, but it can see old data in your idea. Since >> these is obviously order between first transaction and second >> transaction I think that It's not problem of providing consistent >> view. > > 2PC doesn't guarantee this. For that you need other methods and > protocols. We have discussed this before. [2] > At any rate, I think that it would confuse the user that there is no guarantee that the latest data updated by previous transaction can be seen by following transaction. I don't think that it's worth enough to immolate in order to get better performance. Providing atomic visibility for concurrency transaction would be supported later. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- 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] FSM corruption leading to errors
On Mon, Oct 17, 2016 at 4:14 PM, Pavan Deolasee wrote: > I think this is a major bug and I would appreciate any ideas to get the > patch in a committable shape before the next minor release goes out. We > probably need a committer to get interested in this to make progress. Same opinion here, this is very annoying for some of my internal users.. I don't have more to offer though. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Mention column name in error messages
On Thu, Oct 6, 2016 at 2:58 PM, Franck Verrot wrote:
> Michael, please find attached a revised patch addressing, amongst some other
> changes, the testing issue (`make check` passes now) and the visibility of
> the ` TransformExprState` struct.
+ /* Set up callback to identify error line number. */
+ errcallback.callback = TransformExprCallback;
Er, no. You want to know at least column name here, not a line number
*** /Users/mpaquier/git/postgres/src/test/regress/expected/xml_2.out
Mon Oct 17 11:32:26 2016
--- /Users/mpaquier/git/postgres/src/test/regress/results/xml.out
Mon Oct 17 15:58:42 2016
***
*** 9,14
--- 9,15
LINE 1: INSERT INTO xmltest VALUES (3, 'p_is_insert for that. In short the context message could just
have this shape:
CONTEXT { INSERT | UPDATE } relname, column "col", type coltype.
Andres wrote:
> +/* Support for TransformExprCallback */
> +typedef struct TransformExprState
> +{
> + const char *column_name;
> + Oid expected_type;
> +} TransformExprState;
> I see no need for this to be a struct defined in the header. Given that
> TransformExprCallback isn't public, and the whole thing is specific to
> TransformExprState...
That's a matter of taste, really. Personally I find cleaner to declare
that with the other static declarations at the top of the fil, and
keep the comments of transformAssignedExpr clean of everything.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw super user checks
On Mon, Oct 17, 2016 at 12:03 AM, Jeff Janes wrote:
> postgres_fdw has some checks to enforce that non-superusers must connect to
> the foreign server with a password-based method. The reason for this is to
> prevent the authentication to the foreign server from happening on the basis
> of the OS user who is running the non-foreign server.
>
> But I think these super user checks should be run against the userid of the
> USER MAPPING being used for the connection, not the userid of currently
> logged on user.
>
> That is, I think the last line in this script should succeed: ('jjanes' is
> both a superuser, and a database):
>
>
> CREATE EXTENSION IF NOT EXISTS postgres_fdw WITH SCHEMA public;
> CREATE SERVER foo FOREIGN DATA WRAPPER postgres_fdw;
> CREATE USER MAPPING FOR jjanes SERVER foo;
> CREATE TABLE foobar1 ( x integer);
> CREATE FOREIGN TABLE foobar2 ( x integer) SERVER foo OPTIONS ( table_name
> 'foobar1');
> CREATE VIEW foobar3 AS SELECT foobar2.x FROM foobar2;
> CREATE USER test;
> GRANT SELECT ON TABLE foobar3 TO test;
> \c jjanes test
> select * from foobar3;
>
> It connects back to itself, simply for demonstration purposes.
>
> The attached patch implements this change in auth checking.
>
I agree with your analysis, that any passwordless foreign server
access with super user's user mapping should be allowed. If it's safe
to access a foreign server without password for a superuser, then it
should be safe to do so when corresponding user mapping is used even
when login user is non-superuser.
But there's one problem with the patch.
login as some useruser and run following commands.
create extension postgres_fdw;
create server foo foreign data wrapper postgres_fdw options (dbname 'postgres');
create user test;
grant USAGE ON FOREIGN server foo to test;
set role test;
create user mapping for test server foo;
create foreign table fpg_class (oid oid) server foo options
(table_name 'pg_class', schema_name 'pg_catalog');
create view fview as select * from fpg_class;
set role ;
select * from fview limit 0;
With your patch it gives error
ERROR: password is required
DETAIL: Non-superuser cannot connect if the server does not request a password.
HINT: Target server's authentication method must be changed.
Without the patch it does not give any error.
Is that intentional?
I guess, this is because of asymmetry in check_conn_params() and
connect_pg_server(). The first one does not check any params if the
logged in user is a superuser but the later checks if only the user in
the mapping is superuser.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FSM corruption leading to errors
On Tue, Oct 11, 2016 at 5:20 AM, Michael Paquier wrote: > > > > > Once the underlying bug is fixed, I don't see why it should break again. > I > > added the above code to mostly deal with already corrupt FSMs. May be we > can > > just document and leave it to the user to run some correctness checks > (see > > below), especially given that the code is not cheap and adds overheads > for > > everybody, irrespective of whether they have or will ever have corrupt > FSM. > > Yep. I'd leave it for the release notes to hold a diagnostic method. > That's annoying, but this has been done in the past like for the > multixact issues.. > I'm okay with that. It's annoying, especially because the bug may show up when your primary is down and you just failed over for HA, only to find that the standby won't work correctly. But I don't have ideas how to fix existing corruption without adding significant penalty to normal path. > > What if you restart the standby, and then do a diagnostic query? > Wouldn't that be enough? (Something just based on > pg_freespace(pg_relation_size(oid) / block_size) != 0) > > Yes, that will enough once the fix is in place. I think this is a major bug and I would appreciate any ideas to get the patch in a committable shape before the next minor release goes out. We probably need a committer to get interested in this to make progress. Thanks, Pavan -- Pavan Deolasee http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Quorum commit for multiple synchronous replication.
On Tue, Oct 11, 2016 at 4:18 PM, Masahiko Sawada wrote: > On Wed, Sep 28, 2016 at 5:14 PM, Michael Paquier > wrote: >> On Sat, Sep 24, 2016 at 5:37 PM, Masahiko Sawada >> wrote: >>> I still vote for changing behaviour of existing syntax 'k (n1, n2)' to >>> quorum commit. >>> That is, >>> 1. 'First k (n1, n2, n3)' means that the master server waits for ACKs >>> from k standby servers whose name appear earlier in the list. >>> 2. 'Any k (n1, n2, n3)' means that the master server waits for ACKs >>> from any k listed standby servers. >>> 3. 'n1, n2, n3' is the same as #1 with k=1. >>> 4. '(n1, n2, n3)' is the same as #2 with k=1. >> >> OK, so I have done a review of this patch keeping that in mind as >> that's the consensus. I am still getting familiar with the code... > > Thank you for reviewing! > >> -transactions will wait until all the standby servers which are >> considered >> +transactions will wait until the multiple standby servers which >> are considered >> There is no real need to update this sentence. >> >> +FIRST means to control the standby servers with >> +different priorities. The synchronous standbys will be those >> +whose name appear earlier in this list, and that are both >> +currently connected and streaming data in real-time(as shown >> +by a state of streaming in the >> + >> +pg_stat_replication view). Other standby >> +servers appearing later in this list represent potential >> +synchronous standbys. If any of the current synchronous >> +standbys disconnects for whatever reason, it will be replaced >> +immediately with the next-highest-priority standby. >> +For example, a setting of FIRST 3 (s1, s2, s3, s4) >> +makes transaction commits wait until their WAL records are received >> +by three higher-priority standbys chosen from standby servers >> +s1, s2, s3 and s4. >> It does not seem necessary to me to enter in this level of details: >> The keyword FIRST, coupled with an integer number N, chooses the first >> N higher-priority standbys and makes transaction commit when their WAL >> records are received. For example FIRST 3 (s1, s2, s3, s4) >> makes transaction commits wait until their WAL records are received by >> the three high-priority standbys chosen from standby servers s1, s2, >> s3 and s4. > > Will fix. > >> +ANY means to control all of standby servers with >> +same priority. The master sever will wait for receipt from >> +at least num_sync >> +standbys, which is quorum commit in the literature. The all of >> +listed standbys are considered as candidate of quorum commit. >> +For example, a setting of ANY 3 (s1, s2, s3, s4) makes >> +transaction commits wait until receiving receipts from at least >> +any three standbys of four listed servers s1, >> +s2, s3, s4. >> >> Similarly, something like that... >> The keyword ANY, coupled with an integer number N, chooses N standbys >> in a set of standbys with the same, lowest, priority and makes >> transaction commit when WAL records are received those N standbys. For >> example ANY 3(s1, s2, s3, s4) makes transaction commits wait until WAL >> records have been received from 3 servers in the set s1, s2, s3 and >> s4. > > Will fix. > >> It could be good also to mention that no keyword specified means ANY, >> which is incompatible with 9.6. The docs also miss the fact that if a >> simple list of servers is given, without parenthesis and keywords, >> this is equivalent to FIRST 1. > > Right. I will add those documentations. > >> -synchronous_standby_names = '2 (s1, s2, s3)' >> +synchronous_standby_names = 'First 2 (s1, s2, s3)' >> Nit here. It may be a good idea to just use upper-case characters in >> the docs, or just lower-case for consistency, but not mix both. >> Usually GUCs use lower-case characters. > > Agree. Will fix. > >> + when standby is considered as a condidate of quorum commit. >> s/condidate/candidate/ > > Will fix. > >> -syncrep_scanner.c: FLEXFLAGS = -CF -p >> +syncrep_scanner.c: FLEXFLAGS = -CF -p -i >> Hm... Is that actually a good idea? Now "NODE" and "node" are two >> different things for application_name, but with this patch both would >> have the same meaning. I am getting to think that we could just use >> the lower-case characters for the keywords any/first. Is this -i >> switch a problem for elements in standby_list? > > The string of standby name is not changed actually, only the parser > doesn't distinguish between "NODE" and "node". > The values used for checking application_name will still works fine. > If we want to name "first" or "any" as the standby name then it should > be double quoted. > >> + * Calculate the 'pos' newest Write, Flush and Apply positions among >> sync standbys. >> I don't understand this comment. >> >> + if (SyncRepConfig->sync_method == SYNC_REP_PRIORITY) >> + got_recpt
