Re: [HACKERS] WIP: Covering + unique indexes.
On Mon, Aug 15, 2016 at 8:15 PM, Anastasia Lubennikova wrote: @@ -590,7 +622,14 @@ _bt_buildadd(BTWriteState *wstate, BTPageState *state, IndexTuple itup) if (last_off == P_HIKEY) { Assert(state->btps_minkey == NULL); - state->btps_minkey = CopyIndexTuple(itup); + /* + * Truncate the tuple that we're going to insert + * into the parent page as a downlink + */ + if (indnkeyatts != indnatts && P_ISLEAF(pageop)) + state->btps_minkey = index_truncate_tuple(wstate->index, itup); + else + state->btps_minkey = CopyIndexTuple(itup); It seems that above code always ensure that for leaf pages, high key is a truncated tuple. What is less clear is if that is true, why you need to re-ensure it again for the old page in below code: @@ -510,6 +513,8 @@ _bt_buildadd(BTWriteState *wstate, BTPageState *state, IndexTuple itup) { .. + if (indnkeyatts != indnatts && P_ISLEAF(opageop)) + { + /* + * It's essential to truncate High key here. + * The purpose is not just to save more space on this particular page, + * but to keep whole b-tree structure consistent. Subsequent insertions + * assume that hikey is already truncated, and so they should not + * worry about it, when copying the high key into the parent page + * as a downlink. + * NOTE It is not crutial for reliability in present, + * but maybe it will be that in the future. + */ + keytup = index_truncate_tuple(wstate->index, oitup); + + /* delete "wrong" high key, insert keytup as P_HIKEY. */ + PageIndexTupleDelete(opage, P_HIKEY); + + if (!_bt_pgaddtup(opage, IndexTupleSize(keytup), keytup, P_HIKEY)) + elog(ERROR, "failed to rewrite compressed item in index \"%s\"", + RelationGetRelationName(wstate->index)); + } + .. .. -- 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] WAL consistency check facility
On Sat, Aug 27, 2016 at 9:47 PM, Amit Kapila wrote: > Right, I think there is no need to mask all the flags. However apart > from BTP_HAS_GARBAGE, it seems we should mask BTP_SPLIT_END as that is > just used to save some processing for vaccum and won't be set after > crash recovery or on standby after WAL replay. Right you are -- while BTP_INCOMPLETE_SPLIT is set during recovery, BTP_SPLIT_END is not. Still, most of the btpo_flags flags that are masked in the patch shouldn't be. -- 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] WAL consistency check facility
On Sun, Aug 28, 2016 at 6:26 AM, Peter Geoghegan wrote: > On Thu, Aug 25, 2016 at 9:41 AM, Kuntal Ghosh > wrote: >> 2. For Btree pages, I've masked BTP_HALF_DEAD, BTP_SPLIT_END, >> BTP_HAS_GARBAGE and BTP_INCOMPLETE_SPLIT flags. > > Why? I think that you should only perform this kind of masking where > it's clearly strictly necessary. > > It is true that nbtree can allow cases where LP_DEAD is set with only > a share lock (by read-only queries), so I can see why BTP_HAS_GARBAGE > might need to be excluded; this is comparable to the heapam's use of > hint bits. However, it is unclear why you need to mask the remaining > btpo_flags that you list, because the other flags have clear-cut roles > in various atomic operations that we WAL-log. > Right, I think there is no need to mask all the flags. However apart from BTP_HAS_GARBAGE, it seems we should mask BTP_SPLIT_END as that is just used to save some processing for vaccum and won't be set after crash recovery or on standby after WAL replay. -- 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] Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)
On 2016-08-27 14:48:29 -0700, Andres Freund wrote: > My next steps are to work on cleaning up the code a bit more, and > increase the regression coverage. Oh, there's one open item I actually don't really know how to handle well: A decent way of enforcing the join order between the subquery and the functionscan when there's no lateral dependencies. I've hacked up the lateral machinery to just always add a pointless dependency, but that seems fairly ugly. If somebody has a better idea, that'd be great. 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] WAL consistency check facility
On Thu, Aug 25, 2016 at 9:41 AM, Kuntal Ghosh wrote: > 2. For Btree pages, I've masked BTP_HALF_DEAD, BTP_SPLIT_END, > BTP_HAS_GARBAGE and BTP_INCOMPLETE_SPLIT flags. Why? I think that you should only perform this kind of masking where it's clearly strictly necessary. It is true that nbtree can allow cases where LP_DEAD is set with only a share lock (by read-only queries), so I can see why BTP_HAS_GARBAGE might need to be excluded; this is comparable to the heapam's use of hint bits. However, it is unclear why you need to mask the remaining btpo_flags that you list, because the other flags have clear-cut roles in various atomic operations that we WAL-log. -- 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
[HACKERS] src/include/catalog/pg_foreign_table.h still refers genbki.sh
Hi, I've noticed the comment in src/include/catalog/pg_foreign_table.h still talks about genbki.sh, but AFAIK it was replaced with genbki.pl. regards -- Tomas Vondra 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] WAL consistency check facility
On Fri, Aug 26, 2016 at 7:24 AM, Alvaro Herrera wrote: >> As the block numbers are different, I was getting the following warning: >> WARNING: Inconsistent page (at byte 8166) found for record >> 0/127F4A48, rel 1663/16384/16946, forknum 0, blkno 0, Backup Page >> Header : (pd_lower: 28 pd_upper: 8152 pd_special: 8192) Current Page >> Header: (pd_lower: 28 pd_upper: 8152 pd_special: 8192) >> CONTEXT: xlog redo at 0/127F4A48 for Heap/INSERT+INIT: off 1 >> >> In heap_xlog_insert, t_ctid is always set to blkno and xlrec->offnum. >> I think this is why I was getting the above warning. > > Umm, really? Then perhaps this *is* a bug. Peter? It's a matter of perspective, but probably not. The facts are: * heap_insert() treats speculative insertions differently. In particular, it does not set ctid in the caller-passed heap tuple itself, leaving the ctid field to contain a speculative token value -- a per-backend monotonically increasing identifier. This identifier represents some particular speculative insertion attempt within a backend. * On the redo side, heap_xlog_insert() was only changed mechanically when upsert went in. So, it doesn't actually care about the stuff that heap_insert() was made to care about to support speculative insertion. * Furthermore, heap_insert() does *not* WAL log ctid under any circumstances (that didn't change, either). Traditionally, the ctid field was completely redundant anyway (since, of course, we're only dealing with newly inserted tuples in heap_insert()). With speculative insertions, there is a token within ctid, whose value represents actual information that cannot be reconstructed after the fact (the speculative token value). Even still, that isn't WAL-logged (see comments above xl_heap_header struct). That's okay, because the speculative insertion token value is only needed due to obscure issues with "unprincipled deadlocks". The speculative token value itself is only of interest to other, conflicting inserters, and only for the instant in time immediately following physical insertion. The token doesn't matter in the slightest to crash recovery, nor to Hot Standby replicas. While this design had some really nice properties (ask me if you are unclear on this), it does break tools like the proposed WAL-checker tool. I would compare this speculative token situation to the situation with hint bits (when checksums are disabled, and wal_log_hints = off). I actually have a lot of sympathy for the idea that, in general, cases like this should be avoided. But, would it really be worth it to create a useless special case for speculative insertion (to WAL-log the virtually useless speculative insertion token value)? I'm certain that the answer must be "no": This tool ought to deal with speculative insertion as a special case, and not vice-versa. -- 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] Set log_line_prefix and application name in test drivers
Christoph Berg writes: > I've always been wondering why we don't set a log_line_prefix by > default. I think the odds of getting to something that everyone would agree on are nil, so I'm not excited about getting into that particular bikeshed-painting discussion. Look at the amount of trouble we're having converging on a default for the regression tests, which are a far narrower use-case than "everybody". > The above looks quite similar to what the Debian packages have been > setting as their default for some time, with standard stderr logging: I think Debian's choice was probably made by fiat, not by consensus. Packagers seem to be able to get away with quite a lot in that regard. 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] Bogus sizing parameters in some AllocSetContextCreate calls
Andres Freund writes: > On 2016-08-27 15:46:26 -0400, Tom Lane wrote: >> (Or in other words, the fact that "DefaultContextCreate" is spelled >> "AllocSetContextCreate" is a bit of a historical artifact, but I do >> not see why changing the spelling is a useful exercise.) > Well, if you're going through nearly all of the instances where it's > referenced *anyway*... I am not touching, and cannot touch, all the instances in third-party code. 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] Set log_line_prefix and application name in test drivers
Re: Fabien COELHO 2016-08-26 > So I would suggest something like the following, which is also a little bit > more compact: > > log_line_prefix = '%m [%p:%l] %q%a ' > > If you want to keep something with %a, maybe parentheses? > > Finally I'm wondering also whether a timestamp since the server has started > (which does not exists) would be more useful for a "make check", or at > default maybe %n? I've always been wondering why we don't set a log_line_prefix by default. Logs without timestamps and (pid or session id or equivalent) are useless. Of course in practise the log_line_prefix needs to be different depending on the log_destination (syslog adds its own timestamps, ...), but the current default of '' doesn't help anyone. The above looks quite similar to what the Debian packages have been setting as their default for some time, with standard stderr logging: log_line_prefix = '%t [%p-%l] %q%u@%d ' People who want a different log channel need to touch the config anyway and can update log_line_prefix as they go. The concrete value to be used needs to be discussed, but I think we'd end up with something like '%m [%p:%l] ' plus maybe some suffix. Christoph -- 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] Bogus sizing parameters in some AllocSetContextCreate calls
On 2016-08-27 15:46:26 -0400, Tom Lane wrote: > (Or in other words, the fact that "DefaultContextCreate" is spelled > "AllocSetContextCreate" is a bit of a historical artifact, but I do > not see why changing the spelling is a useful exercise.) Well, if you're going through nearly all of the instances where it's referenced *anyway*... -- 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] Bogus sizing parameters in some AllocSetContextCreate calls
Andres Freund writes: > On 2016-08-27 15:36:28 -0400, Tom Lane wrote: >> What is actually of interest, IMO, is making *some* contexts have a >> different allocator, and that is going to require case-by-case decisions >> anyway. A blanket switch seems completely useless to me. > Don't think aset.c is actually that good, so I'm not convinced of > that. And even if it's just to verify that a special case allocator > actually works with more coverage... I'm not exactly following. If you have a new allocator that dominates aset.c on all cases, why would we not simply replace aset.c with it? Mass substitution implemented at the caller end seems like the hard way. (Or in other words, the fact that "DefaultContextCreate" is spelled "AllocSetContextCreate" is a bit of a historical artifact, but I do not see why changing the spelling is a useful exercise.) 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] Bogus sizing parameters in some AllocSetContextCreate calls
On 2016-08-27 15:36:28 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2016-08-27 14:08:44 -0400, Tom Lane wrote: > >> Barring objection, I propose to make these changes in HEAD and 9.6. > > > I think we might also / instead want to think about replacing a lot of > > those AllocSetContextCreate with a wrapper function. Currently is really > > rather annoying to experiment with switching the default allocator > > out. And if we're touching all that code anyway ... > > I do not see why you'd think that "switching the default allocator out" > requires anything except making AllocSetContextCreate do something else. Well yea. But given it's explicitly tied to aset.c that doesn't seem like a sensible thing. DefaultContextCreate() or something (in mcxt.c), which then calls AllocSetContextCreate, seems better to me. > What is actually of interest, IMO, is making *some* contexts have a > different allocator, and that is going to require case-by-case decisions > anyway. A blanket switch seems completely useless to me. Don't think aset.c is actually that good, so I'm not convinced of that. And even if it's just to verify that a special case allocator actually works with more coverage... 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] Bogus sizing parameters in some AllocSetContextCreate calls
Andres Freund writes: > On 2016-08-27 14:08:44 -0400, Tom Lane wrote: >> Barring objection, I propose to make these changes in HEAD and 9.6. > I think we might also / instead want to think about replacing a lot of > those AllocSetContextCreate with a wrapper function. Currently is really > rather annoying to experiment with switching the default allocator > out. And if we're touching all that code anyway ... I do not see why you'd think that "switching the default allocator out" requires anything except making AllocSetContextCreate do something else. What is actually of interest, IMO, is making *some* contexts have a different allocator, and that is going to require case-by-case decisions anyway. A blanket switch seems completely useless to me. 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] Bogus sizing parameters in some AllocSetContextCreate calls
Hi, On 2016-08-27 14:08:44 -0400, Tom Lane wrote: > The standard calling pattern for AllocSetContextCreate is > Barring objection, I propose to make these changes in HEAD and 9.6. > I don't feel a great need to adjust the back branches --- although there > might be some argument for adding these new calling-pattern macros to the > back branches so as not to create a back-patching hazard for patches > that add new AllocSetContextCreate calls. I think we might also / instead want to think about replacing a lot of those AllocSetContextCreate with a wrapper function. Currently is really rather annoying to experiment with switching the default allocator out. And if we're touching all that code anyway ... 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] Renaming of pg_xlog and pg_clog
Alvaro Herrera writes: > I'm for renaming too, but I'd go with Peter E's suggestion: move pg_xlog > to something like $PGDATA/var/wal or $PGDATA/srv/wal or something like that. I think that would make sense if we were to relocate *everything* under PGDATA into some FHS-like subdirectory structure. But I'm against moving the config files for previously-stated reasons, and I doubt it makes sense to adopt an FHS-like structure only in part. 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] Bogus sizing parameters in some AllocSetContextCreate calls
The standard calling pattern for AllocSetContextCreate is cxt = AllocSetContextCreate(parent, name, ALLOCSET_DEFAULT_MINSIZE, ALLOCSET_DEFAULT_INITSIZE, ALLOCSET_DEFAULT_MAXSIZE); or for some contexts you might s/DEFAULT/SMALL/g if you expect the context to never contain very much data. I happened to notice that there are a few calls that get this pattern wrong. After a bit of grepping, I found: hba.c lines 389, 2196: ALLOCSET_DEFAULT_MINSIZE, ALLOCSET_DEFAULT_MINSIZE, ALLOCSET_DEFAULT_MAXSIZE); guc-file.l line 146: ALLOCSET_DEFAULT_MINSIZE, ALLOCSET_DEFAULT_MINSIZE, ALLOCSET_DEFAULT_MAXSIZE); brin.c line 857: ALLOCSET_SMALL_INITSIZE, ALLOCSET_SMALL_MINSIZE, ALLOCSET_SMALL_MAXSIZE); autovacuum.c line 2184: ALLOCSET_DEFAULT_INITSIZE, ALLOCSET_DEFAULT_MINSIZE, ALLOCSET_DEFAULT_MAXSIZE); typcache.c lines 757, 842: ALLOCSET_SMALL_INITSIZE, ALLOCSET_SMALL_MINSIZE, ALLOCSET_SMALL_MAXSIZE); In all of these cases, we're passing zero as the initBlockSize, which is invalid, but but AllocSetContextCreate silently forces it up to 1K. So there's no failure but there may be some inefficiency due to small block sizes of early allocation blocks. I seriously doubt that's intentional; in some of these cases it might be sane to use the SMALL parameters instead, but this isn't a good way to get that effect. The last four cases are also passing a nonzero value as the minContextSize, forcing the context to allocate at least that much instantly and hold onto it over resets. That's inefficient as well, though probably only minorly so. I can state confidently that the typcache.c cases are just typos, because I wrote them :-(. It seems unlikely that the others are intentional either; certainly none of these cases have any comments suggesting that any special behavior is meant. While we could just fix these cases, the fact that as many as seven of the only-140-or-so calls in our code are wrong suggests that we ought to think about a less typo-prone solution. My low-tech proposal is to define two new macros along the lines of #define ALLOCSET_DEFAULT_SIZES ALLOCSET_DEFAULT_MINSIZE, ALLOCSET_DEFAULT_INITSIZE, ALLOCSET_DEFAULT_MAXSIZE #define ALLOCSET_SMALL_SIZES ALLOCSET_SMALL_MINSIZE, ALLOCSET_SMALL_INITSIZE, ALLOCSET_SMALL_MAXSIZE so that a typical call now looks like cxt = AllocSetContextCreate(parent, name, ALLOCSET_DEFAULT_SIZES); This approach doesn't break any third-party code that doesn't get converted right away, and it also doesn't create a problem for the small number of places that are intentionally trying to obtain nonstandard effects. There are three or four places that use the pattern ALLOCSET_SMALL_MINSIZE, ALLOCSET_SMALL_INITSIZE, ALLOCSET_DEFAULT_MAXSIZE with the intention of starting small but allowing the context to grow big efficiently if it needs to. It might be worth defining a third macro for this pattern. The cases that fit into none of these patterns are few enough and special enough (eg, ErrorContext) that I don't think they need adjustment. Barring objection, I propose to make these changes in HEAD and 9.6. I don't feel a great need to adjust the back branches --- although there might be some argument for adding these new calling-pattern macros to the back branches so as not to create a back-patching hazard for patches that add new AllocSetContextCreate calls. Comments? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Renaming of pg_xlog and pg_clog
Michael Paquier wrote: > On Sat, Aug 27, 2016 at 6:34 AM, Andres Freund wrote: > > On 2016-08-26 17:31:14 -0400, Peter Eisentraut wrote: > >> I agree with all that. But the subject line is specifically about > >> moving pg_xlog. So if your opinion is that we shouldn't move pg_xlog, > >> then that is noted. But if we were to move it, we can think about a > >> good place to move it to. > > > > I think it's probably worth moving pg_xlog, because the benefit also > > includes preventing a few users from shooting themselves somewhere > > vital. That's imo much less the case for some of the other moves. But I > > still don't think think a largescale reorganization is a good idea, > > it'll just stall and nothing will happen. > > OK, so let's focus only on the renaming mentioned in $subject. So far > as I can see on this thread, here are the opinions of people who > clearly gave one: > - Rename them, hard break is OK: Michael P, Bruce, Stephen (depends on > David's input), Magnus > - Rename them, hard break not OK: Fujii-san (perhaps do nothing?) > - Do nothing: Simon (add a README), Tom, Peter E I'm for renaming too, but I'd go with Peter E's suggestion: move pg_xlog to something like $PGDATA/var/wal or $PGDATA/srv/wal or something like that. -- Álvaro Herrerahttp://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] Renaming of pg_xlog and pg_clog
Michael Paquier writes: > OK, so let's focus only on the renaming mentioned in $subject. So far > as I can see on this thread, here are the opinions of people who > clearly gave one: > - Rename them, hard break is OK: Michael P, Bruce, Stephen (depends on > David's input), Magnus > - Rename them, hard break not OK: Fujii-san (perhaps do nothing?) > - Do nothing: Simon (add a README), Tom, Peter E Hm, if you read me as voting against renaming pg_xlog, that wasn't the conclusion I meant to convey. I'm against moving/renaming the configuration files, because I think that will break a lot of users' scripts and habits without really buying much. But I'm for consolidating all the files that should not be copied by backup tools into one subdirectory, and I think that while we're doing that it would be sensible to rename pg_xlog and pg_clog to something that doesn't sound like it's scratch data. I'm on the fence about whether pg_logical ought to get renamed. > As far as I can see, there is a consensus to not rename pg_xlog to > pg_journal and avoid using a third meaning, but instead use pg_wal. Yeah, +1 for pg_wal, we do not need yet another name for that. > I guess that now the other renaming would be pg_clog -> pg_xact. We already have pg_subtrans, so it seems like pg_trans is an obvious suggestion. I'm not sure whether the other precedent of pg_multixact is a stronger one than that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] OpenSSL 1.1 breaks configure and more
On 8/26/16 9:26 PM, Andreas Karlsson wrote: > I have attached a patch which removes the < 0.9.8 compatibility code. > Should we also add a version check to configure? We do not have any such > check currently. I think that is not necessary. -- 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] OpenSSL 1.1 breaks configure and more
On Sat, Aug 27, 2016 at 2:04 AM, Heikki Linnakangas wrote: > On 08/26/2016 07:44 PM, Tom Lane wrote: >> Peter Eisentraut writes: >> Also, I get this on fully-up-to-date OS X (El Capitan): >> >> $ openssl version >> OpenSSL 0.9.8zh 14 Jan 2016 > > > Ok, sold, let's remove support for OpenSSL < 0.9.8. Yes I think it's a wiser plan to not brush up newer versions than that. >> Worth noting though is that without -Wno-deprecated-declarations, you >> find that Apple has sprinkled the entire OpenSSL API with deprecation >> warnings. That suggests that their plan for the future is to drop it >> rather than update it. Should we be thinking ahead to that? > > > Yeah, they want people to move to their own SSL library [1]. I doubt they > will actually remove it any time soon, but who knows. It would be a good > project for someone with an OS X system and some spare time, to write a > patch to build with OS X's native SSL library instead of OpenSSL. The code > is structured nicely to enable that now. > > [1] I couldn't find any official statement, but lots of blog posts saying > the same thing. As well on El Capitan: $ ssh -V OpenSSH_6.9p1, LibreSSL 2.1.8 So could it be possible that it would be a switch from openssl to libressl instead? -- 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] Missing checks when malloc returns NULL...
On Sat, Aug 27, 2016 at 12:33 AM, Aleksander Alekseev wrote: > Your patch [1] was marked as "Needs review" so I decided to take a look. Thanks for the input! > It looks good to me. However I found dozens of places in PostgreSQL code > that seem to have similar problem you are trying to fix [2]. As far as I > understand these are only places left that don't check malloc/realloc/ > strdup return values properly. I thought maybe you will be willing to > fix they too so we could forget about this problem forever. So my lookup was still incomplete. > If not I will be happy to do it. However in this case we need someone > familiar with affected parts of the code who will be willing to re-check > a new patch since I'm not filling particularly confident about how > exactly errors should be handled in all these cases. I'll do it and compress that in my patch. > By the way maybe someone knows other procedures besides malloc, realloc > and strdup that require special attention? I don't know how you did it, but this email has visibly broken the original thread. Did you change the topic name? ./src/backend/postmaster/postmaster.c: userDoption = strdup(optarg); [...] ./src/backend/bootstrap/bootstrap.c:userDoption = strdup(optarg); [...] ./src/backend/tcop/postgres.c: userDoption = strdup(optarg); We cannot use pstrdup here because memory contexts are not set up here, still it would be better than crashing, but I am not sure if that's worth complicating this code.. Other opinions are welcome. ./contrib/vacuumlo/vacuumlo.c: param.pg_user = strdup(optarg); [...] ./contrib/pg_standby/pg_standby.c: triggerPath = strdup(optarg); [...] ./src/bin/pg_archivecleanup/pg_archivecleanup.c: additional_ext = strdup(optarg);/* Extension to remove Right we can do something here with pstrdup(). ./contrib/spi/refint.c: plan->splan = (SPIPlanPtr *) malloc(sizeof(SPIPlanPtr)); Regarding refint.c, you can see upthread. Instead of reworking the code it would be better to just drop it from the tree. ./src/backend/utils/adt/pg_locale.c:grouping = strdup(extlconv->grouping); Here that would be a failure with an elog() as this is getting used by things like NUM_TOCHAR_finish and PGLC_localeconv. ./src/backend/utils/mmgr/mcxt.c:node = (MemoryContext) malloc(needed); You cannot do much here... ./src/timezone/zic.c: lcltime = strdup(optarg); This is upstream code, not worth worrying. ./src/pl/tcl/pltcl.c: prodesc->user_proname = strdup(NameStr(procStruct->proname)); ./src/pl/tcl/pltcl.c: prodesc->internal_proname = strdup(internal_proname); ./src/pl/tcl/pltcl.c- if (prodesc->user_proname == NULL || prodesc->internal_proname == NULL) ./src/pl/tcl/pltcl.c- ereport(ERROR, ./src/pl/tcl/pltcl.c- (errcode(ERRCODE_OUT_OF_MEMORY), ./src/pl/tcl/pltcl.c-errmsg("out of memory"))); Ah, yes. Here we need to do a free(prodesc) first. ./src/common/exec.c:putenv(strdup(env_path)); set_pglocale_pgservice() is used at the beginning of each process run, meaning that a failure here would be printf(stderr), followed by exit() for frontend, even ECPG as this compiles with -DFRONTEND. Backend can use elog(ERROR) btw. -- 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] WAL consistency check facility
On Sat, Aug 27, 2016 at 6:16 PM, Simon Riggs wrote: > On 27 August 2016 at 07:36, Amit Kapila wrote: >> On Fri, Aug 26, 2016 at 9:26 PM, Simon Riggs wrote: >>> >>> I think you should add this as part of the default testing for both >>> check and installcheck. I can't imagine why we'd have it and not use >>> it during testing. >>> >> >> The actual consistency checks are done during redo (replay), so not >> sure whats in you mind for enabling it with check or installcheck. I >> think we can run few recovery/replay tests with this framework. Also >> running the tests under this framework could be time-consuming as it >> logs the entire page for each WAL record we write and then during >> replay reads the same. > > I'd like to see an automated test added so we can be certain we don't > add things that break recovery. Don't mind much where or how. > > The main use is to maintain that certainty while in production. For developers, having extra checks with the new routines in WAL_DEBUG could also be useful for a code path producing WAL. Let's not forget that as well. -- 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] WAL consistency check facility
Hello Simon, I'm really sorry for the inconveniences. Next time, I'll attach the patch with proper documentation, test and comments. > I think you should add this as part of the default testing for both > check and installcheck. I can't imagine why we'd have it and not use > it during testing. Since, this is redo(replay) feature, we can surely add this in installcheck. But, as Amit mentioned, it could be time-consuming. >> * wal_consistency_mask = 511 /* Enable consistency check mask bit*/ > > What does this mean? (No docs) I was using this parameter as a masking integer to indicate the operations(rmgr list) for which we need this feature to be enabled. Since, this could be confusing, I've changed it accordingly so that it accepts a list of rmgrIDs. (suggested by Michael, Amit and Robert) >> 1. Add support for other Resource Managers. > > We probably need to have a discussion as to why you think this should > be Rmgr dependent? > Code comments would help there. > > If it does, then you should probably do this by extending RmgrTable > with an rm_check, so you can call it like this... > > RmgrTable[record->xl_rmid].rm_check +1. I'm modifying it accordingly. I'm calling this function after RmgrTable[record->xl_rmid].rm_redo. >> 5. Generalize the page type identification technique. > > Why not do this first? > At present, I'm using special page size and page ID to identify page type. But, I've noticed some cases where the entire page is initialized to zero (Ex: hash_xlog_squeeze_page). RmgrID and info bit can help us to identify those pages. -- Thanks & Regards, Kuntal Ghosh 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] WAL consistency check facility
On 27 August 2016 at 07:36, Amit Kapila wrote: > On Fri, Aug 26, 2016 at 9:26 PM, Simon Riggs wrote: >> >> I think you should add this as part of the default testing for both >> check and installcheck. I can't imagine why we'd have it and not use >> it during testing. >> > > The actual consistency checks are done during redo (replay), so not > sure whats in you mind for enabling it with check or installcheck. I > think we can run few recovery/replay tests with this framework. Also > running the tests under this framework could be time-consuming as it > logs the entire page for each WAL record we write and then during > replay reads the same. I'd like to see an automated test added so we can be certain we don't add things that break recovery. Don't mind much where or how. The main use is to maintain that certainty while in production. -- Simon Riggshttp://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] Renaming of pg_xlog and pg_clog
On 27/08/16 20:33, Michael Paquier wrote: On Sat, Aug 27, 2016 at 6:34 AM, Andres Freund wrote: On 2016-08-26 17:31:14 -0400, Peter Eisentraut wrote: I agree with all that. But the subject line is specifically about moving pg_xlog. So if your opinion is that we shouldn't move pg_xlog, then that is noted. But if we were to move it, we can think about a good place to move it to. I think it's probably worth moving pg_xlog, because the benefit also includes preventing a few users from shooting themselves somewhere vital. That's imo much less the case for some of the other moves. But I still don't think think a largescale reorganization is a good idea, it'll just stall and nothing will happen. OK, so let's focus only on the renaming mentioned in $subject. So far as I can see on this thread, here are the opinions of people who clearly gave one: - Rename them, hard break is OK: Michael P, Bruce, Stephen (depends on David's input), Magnus - Rename them, hard break not OK: Fujii-san (perhaps do nothing?) - Do nothing: Simon (add a README), Tom, Peter E As far as I can see, there is a consensus to not rename pg_xlog to pg_journal and avoid using a third meaning, but instead use pg_wal. I guess that now the other renaming would be pg_clog -> pg_xact. Other opinions? Forgot you here? I think if there are going to be things in pg that break software - for good reasons, like making future usage easier at the cost an initial sharp pain - then to do so in version '10.0.0' is very appropriate! IMHO And better to do so in 10.0.0 (especially if closely related), rather than 10.1.0 (or whatever the next version after that is named). So, if other things might cause breakages, do so IN 10.0.0 - rather than hold back - assuming that there won't be hundreds or more major breakages!!! Cheers, Gavin -- 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] Renaming of pg_xlog and pg_clog
On Sat, Aug 27, 2016 at 6:34 AM, Andres Freund wrote: > On 2016-08-26 17:31:14 -0400, Peter Eisentraut wrote: >> I agree with all that. But the subject line is specifically about >> moving pg_xlog. So if your opinion is that we shouldn't move pg_xlog, >> then that is noted. But if we were to move it, we can think about a >> good place to move it to. > > I think it's probably worth moving pg_xlog, because the benefit also > includes preventing a few users from shooting themselves somewhere > vital. That's imo much less the case for some of the other moves. But I > still don't think think a largescale reorganization is a good idea, > it'll just stall and nothing will happen. OK, so let's focus only on the renaming mentioned in $subject. So far as I can see on this thread, here are the opinions of people who clearly gave one: - Rename them, hard break is OK: Michael P, Bruce, Stephen (depends on David's input), Magnus - Rename them, hard break not OK: Fujii-san (perhaps do nothing?) - Do nothing: Simon (add a README), Tom, Peter E As far as I can see, there is a consensus to not rename pg_xlog to pg_journal and avoid using a third meaning, but instead use pg_wal. I guess that now the other renaming would be pg_clog -> pg_xact. Other opinions? Forgot you here? -- 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] shm_mq_set_sender() crash
On Fri, Aug 26, 2016 at 6:20 PM, Tom Lane wrote: > Latest from lorikeet: > http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2016-08-26%2008%3A37%3A27 > > TRAP: FailedAssertion("!(vmq->mq_sender == ((void *)0))", File: > "/home/andrew/bf64/root/REL9_6_STABLE/pgsql.build/../pgsql/src/backend/storage/ipc/shm_mq.c", > Line: 220) > Do you think, it is due to some recent change or we are just seeing now as it could be timing specific issue? So here what seems to be happening is that during worker startup, we are trying to set the sender for a shared memory queue and the same is already set. Now, one theoretical possibility of the same could be that the two workers get the same ParallelWorkerNumber which is then used to access the shm queue (refer ParallelWorkerMain/ExecParallelGetReceiver). We are setting the ParallelWorkerNumber in below code which seems to be doing what it is suppose to do: LaunchParallelWorkers() { .. for (i = 0; i < pcxt->nworkers; ++i) { memcpy(worker.bgw_extra, &i, sizeof(int)); if (!any_registrations_failed && RegisterDynamicBackgroundWorker(&worker, &pcxt->worker[i].bgwhandle)) .. } Can some reordering impact the above code? -- 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