Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Andres Freund]

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] WAL consistency check facility

[Peter Geoghegan]

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] shm_mq_set_sender() crash

[Amit Kapila]

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=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, , sizeof(int));
if (!any_registrations_failed &&
RegisterDynamicBackgroundWorker(,
>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

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Michael Paquier]

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] OpenSSL 1.1 breaks configure and more

[Peter Eisentraut]

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] Renaming of pg_xlog and pg_clog

[Tom Lane]

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] WAL consistency check facility

[Simon Riggs]

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] WAL consistency check facility

[Michael Paquier]

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] Renaming of pg_xlog and pg_clog

[Michael Paquier]

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] Renaming of pg_xlog and pg_clog

[Gavin Flower]

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] WAL consistency check facility

[Kuntal Ghosh]

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] Missing checks when malloc returns NULL...

[Michael Paquier]

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

[HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Tom Lane]

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

[Tom Lane]

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

Re: [HACKERS] Bogus sizing parameters in some AllocSetContextCreate calls

[Andres Freund]

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

[Tom Lane]

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

[Andres Freund]

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] Set log_line_prefix and application name in test drivers

[Christoph Berg]

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

[Tom Lane]

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

[Tom Lane]

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] Renaming of pg_xlog and pg_clog

[Alvaro Herrera]

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] Bogus sizing parameters in some AllocSetContextCreate calls

[Tom Lane]

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] WAL consistency check facility

[Peter Geoghegan]

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

[HACKERS] src/include/catalog/pg_foreign_table.h still refers genbki.sh

[Tomas Vondra]

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

[Peter Geoghegan]

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

Re: [HACKERS] Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

[Andres Freund]

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

[Amit Kapila]

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