[HACKERS] Comment and function argument names are mismatched in bugmgr.c.

[Masahiko Sawada]

Hi all,

By commit 428b1d6b, function WritebackContextInit is added but the
comment of this function seems to be incorrect.
*max_coalesce variable doesn't exist at anywhere.
Also, I think it should be fixed that the argument name of this
function does not match function declare in buf_internal.h.
Patch for that attached.

bufmgr.c:L4160
/*
 * Initialize a writeback context, discarding potential previous state.
 *
 * *max_coalesce is a pointer to a variable containing the current maximum
 * number of writeback requests that will be coalesced into a bigger one. A
 * value <= 0 means that no writeback control will be performed. max_pending
 * is a pointer instead of an immediate value, so the coalesce limits can
 * easily changed by the GUC mechanism, and so calling code does not have to
 * check the current configuration.
 */
void
WritebackContextInit(WritebackContext *context, int *max_pending)
{
Assert(*max_pending <= WRITEBACK_MAX_PENDING_FLUSHES);

context->max_pending = max_pending;
context->nr_pending = 0;
}

buf_internal.h:L303
/*
 * Internal routines: only called by bufmgr
 * Internal buffer management routines
 */
/* bufmgr.c */
extern void WritebackContextInit(WritebackContext *context, int *max_coalesce);


Regards,

--
Masahiko Sawada
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a67e518..76ade37 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4160,12 +4160,10 @@ ts_ckpt_progress_comparator(Datum a, Datum b, void *arg)
 /*
  * Initialize a writeback context, discarding potential previous state.
  *
- * *max_coalesce is a pointer to a variable containing the current maximum
- * number of writeback requests that will be coalesced into a bigger one. A
- * value <= 0 means that no writeback control will be performed. max_pending
- * is a pointer instead of an immediate value, so the coalesce limits can
- * easily changed by the GUC mechanism, and so calling code does not have to
- * check the current configuration.
+ * *max_pending is a pointer instead of an immediate value, so the coalesce
+ * limits can easily changed by the GUC mechanism, and so calling code does
+ * not have to check the current configuration. A value is 0 means that no
+ * writeback control will be performed.
  */
 void
 WritebackContextInit(WritebackContext *context, int *max_pending)
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 511d740..e0dfb2f 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -300,7 +300,7 @@ extern CkptSortItem *CkptBufferIds;
  * Internal buffer management routines
  */
 /* bufmgr.c */
-extern void WritebackContextInit(WritebackContext *context, int *max_coalesce);
+extern void WritebackContextInit(WritebackContext *context, int *max_pending);
 extern void IssuePendingWritebacks(WritebackContext *context);
 extern void ScheduleBufferTagForWriteback(WritebackContext *context, BufferTag *tag);
 

-- 
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] Bug in to_timestamp().

[amul sul]

On Monday, 20 June 2016 8:53 PM, Alex Ignatov  wrote:


>>On 13.06.2016 18:52, amul sul wrote:
>And it wont stop on some simple whitespace. By using to_timestamp you 
>can get any output results by providing illegal input parameters values:

>postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD 
>HH24:MI:SS');
>   to_timestamp
>
>  2016-01-06 14:40:39+03
>
> (1 row)

We do consume extra space from input string, but not if it is in format string, 
see below:

postgres=# SELECT TO_TIMESTAMP('2016-06-13  15:43:36', '/MM/DD 
HH24:MI:SS'); 
to_timestamp 

2016-06-13 15:43:36-07
(1 row)

We should have same treatment for format string too.

Thoughts? Comments?

Regards,
Amul Sul


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] PQconnectdbParams vs PQconninfoParse

[Craig Ringer]

Hi all

While writing some code that takes a connstring and adds some parameters, I
noticed that PQconninfoParse doesn't play well with PQconnectdbParams.

PQconnectdbParams takes a pair of equal-length arrays, one for keys and one
for values, each terminated by null elements.  But PQconninfoParse returns
a an array of PQconninfoOption .

This means the client has to do a bunch of fiddling to turn a parsed
conninfo into something that can be passed to PQconnectdbParams .  This
seems bizarre. Am I missing something obvious?

libpq  internally uses connectOptions1 which calls parse_connection_string,
the same function used by PQconninfoParse. But there's no connect variant
exposed to users to use it.

Anyone object to adding one? Like:

PQconnectStartInfo(PQconninfoOption options)
PQconnectdbInfo(PQconninfoOption options)

?

PQconnectStartParams(...) and PQconnectdbParams(...) would become thin
wrappers around it.

It's a pity that the name PQconnectdbParams is already taken, really.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

[HACKERS] Feature suggestions: "dead letter"-savepoint.

[Terje Elde]

Hi all,

I’d like to pitch the idea of supporting “dead letter”-savepoints, similar to 
the way you have “dead letter”-exchanges in message-queue systems, etc.  The 
idea is basically that a client can publish a message, but in such a away that 
it only ever actually gets published if the client dies, goes away suddenly, 
etc.  That allows for some nice logic, from simply announcing “Daemon X just 
died”, to more advanced logic, simplifying self-healing clusters and what not.

Different name would be “last will”.

The use-cases for PostgreSQL would be a bit different, so I’d like to draw up 
an example of where this could be very (imho) useful.


A very common usecase for PostgreSQL goes something like this:

1. Worker picks up a row.
2. Worker does something non-idempotent with an external service, where it’s 
important that it only gets done at most once.  Charge a credit card, send an 
email/sms, launch a rocket, and so on.
3. Worker marks the row as successful, or failed.

But you need only one worker to pick up the task, so you expand to lock the 
row, and implement the first point as something like:

SELECT * FROM targets WHERE status=‘scheduled’ FOR UPDATE SKIP LOCKED LIMIT 1;

That’ll give you a nice lock on the line, yet allow other workers to pick up 
other targets.

But what if there’s a bug making a call to the external service?  Most of the 
time, you’ll trap the error and set status to something sane, but what if 
there’s a crash-bug in the SDK implementing it, or some other situation where 
things go very bad?  The rocket might be fired, then the client dies, lock is 
released, another worker picks up the task, and repeats the process ad nausium.

Okay, so you’ll just update the row to say you’re trying to send it.  You set 
status=‘in-flight’ or some other status that’ll prevent the SELECT in other 
workers from picking up the task, and you commit that, so other workers won’t 
pick up the row if you die.  In the process though, you also loose the lock on 
the row.  You still want the row to be tied to you specifically, so you add a 
unique tag to the row, that later needs to be removed, so there’s more 
housekeeping.

This is pretty basic, it works, and it works well.  However, it could (imho) be 
improved, both in terms of developer comfort, and also more efficient.  The 
need for that extra commit – before doing the actual work – could also be 
avoided, potentially reducing the number of transaction by half.

Typically the flow would be something like:

BEGIN;
SELECT id FROM targets WHERE status=‘scheduled’ FOR UPDATE SKIP LOCKED LIMIT 1;
UPDATE targets SET status=‘in-flight’ WHERE id =%(id);
COMMIT;
— Do the work.
BEGIN;
UPDATE targets SET status=‘completed’ WHERE id = %(id); — or 
status=‘failed-foo’, if it fails for reason foo
COMMIT;


What I’m suggesting would be something along the lines of;

BEGIN;
SELECT id FROM targets WHERE status=‘scheduled’ FOR UPDATE SKIP LOCKED LIMIT 1;
UPDATE targets SET status=‘failed-unknown’ WHERE id =%(id);
SAVEPOINT deadletter ON FAILURE COMMIT;
— Do the work.
UPDATE targets SET status=‘completed’ WHERE id = %(id); — or status=‘failed-foo'
COMMIT;

Or, unless re-setting the columns changed before the savepoint, roll back to 
one prior to it.


The basic idea is to be able to say “If I go belly up, I want this stuff to 
happen”.  Depending on different needs, could be made persistent once the 
savepoint is taken, but for a lot of cases that wouldn’t really be needed.  
There’s some room for variation as well, such as having it support only dropped 
connections, or also support turning errors and/or rollbacks into rollback to 
and commits of the savepoint.  Ideally configurable at the point the snapshot 
it taken, to easily support pr. snapshot variation.


I did for a few moments wonder if prepared transactions would be a better place 
for something like this.  It could allow for named independent transactions, 
but there’s a fairly big mismatch between the two concepts.  It also wouldn’t 
be too hard to use multiple named savepoints for effectively the same logic for 
most cases.  One advantage of prepared transactions is that it could perhaps 
also cover the case of a postgresql child dying, but that’s not exactly a 
common problem.  A huge dealbreaker though, is that the prepared transaction 
would very likely keep conflicting locks with the work to be done.




I have to admit, I’m not sure if this is a big ask or not, but I’m hopeful that 
it’s not.  In part because so much is already there.  My hope is that it 
wouldn’t take a lot to turn an error into what is effectively pretty close to 
“ROLLBACK TO deadletter; COMMIT;”, combined with extending savepoints to 
include information about which failures they should “catch”, and the routing 
to use those.  There could be a host of issues I’m not aware of though.

Terje Elde



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postg

Re: [HACKERS] Feature suggestions: "dead letter"-savepoint.

[Marko Tiikkaja]

On 2016-06-23 12:34, Terje Elde wrote:

Typically the flow would be something like:

BEGIN;
SELECT id FROM targets WHERE status=‘scheduled’ FOR UPDATE SKIP LOCKED LIMIT 1;
UPDATE targets SET status=‘in-flight’ WHERE id =%(id);
COMMIT;
— Do the work.
BEGIN;
UPDATE targets SET status=‘completed’ WHERE id = %(id); — or 
status=‘failed-foo’, if it fails for reason foo
COMMIT;


What I’m suggesting would be something along the lines of;

BEGIN;
SELECT id FROM targets WHERE status=‘scheduled’ FOR UPDATE SKIP LOCKED LIMIT 1;
UPDATE targets SET status=‘failed-unknown’ WHERE id =%(id);
SAVEPOINT deadletter ON FAILURE COMMIT;
— Do the work.
UPDATE targets SET status=‘completed’ WHERE id = %(id); — or status=‘failed-foo'
COMMIT;


Comparing these two; how is the latter any better?  It's the same number 
of commands, except it's holding a transaction open for longer, it's 
using a non-standard concept and it's arguably more complex.



.m


--
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] Requesting external_pid_file with postgres -C when not initialized lead to coredump

[alain radix]

Testing that external_pid_file was not null didn't seem necessary to me
because it's initialized to '' and background workers could only start
after the startup piece of code.

If external_pid_file is set to null while the server run, then I prefer to
not hide the problem as it would only be reported at the server shutdown

I tested with ALTER SYSTEM SET but couldn't set external_pid_file to null,
only to ''
This another good reason to use '' instead of null, because it can be set
with ALTER SYSTEM SET even if I see no reason to ;-)

Using strlen(external_pid_file) instead of external_pid_file[0] seems more
readable to me.
I agree that it cost a few cpu cycles more.

I'm also unsure that testing if(A && B) would always be compiled to A being
tested before B so that it won't always protect from testing
external_pid_file[0] with external_pid_file being null.
To ensure that, I would prefer :
if(! external_pid_file){
if(external_pid_file[0])

But, I see it at useless protective code against an impossible situation.
Maybe I'm wrong, but I don't see how it could happen.

About parameters being null, as they're reported as '' in pg_settings and
ALTER SYSTEM SET can only set them to '', I consider this should be avoided.
I used all parameters reported to '' in pg_settings after initdb and found
only external_pid_file to crash postgres -C

Alain

PS : Thanks for pgBackRest ;-)


On 22 June 2016 at 18:51, Tom Lane  wrote:

> alain radix  writes:
> > Another effect of the patch is that it become possible to start a cluster
> > with external_pid_file='' in postgresql.conf
> > It would have that same behavior as many other parameters.
>
> I'd still be inclined to do that with something along the lines of
>
> -   if (external_pid_file)
> +   if (external_pid_file && external_pid_file[0])
>
> rather than changing the default per se.
>
> regards, tom lane
>



-- 
Alain Radix

Re: [HACKERS] Feature suggestions: "dead letter"-savepoint.

[Terje Elde]

> On 23 Jun 2016, at 11:50, Marko Tiikkaja  wrote:
> 
> Comparing these two; how is the latter any better?  It's the same number of 
> commands, except it's holding a transaction open for longer, it's using a 
> non-standard concept and it's arguably more complex.

Same number of commands, but half the number of transactions/commits.  It’s 
holding the transaction open for longer, but not necessarily very long.  It’s 
also not holding any contested locks (in the example at least).

It is arguably more complex, but also arguably (imho anyway) simpler.  It gives 
a generic pattern of saying “I want this to happen if things go bad”.  With the 
upper example, you spend time setting up a system or pattern specifically for 
the use-case.  With patterns such as using a column to mark ownership of a row 
to your process, you’d need that extra column.  With a large number of rows, 
and a low probability of actually needing to update the row, that could also 
result in significant update-traffic, that could be avoided.  Similar if you’d 
need to keep a lock on another table as well, you’d loose the lock between the 
first and second transaction.  You could work around that with using 
ownership-type columns on that table as well, but then you have update-traffic 
there too.  Not to mention that for any use of ownership-type columns, you run 
the risk of having to do housekeeping on them, versus the second example, where 
you’re sure that all locks get cleaned up.

I entirely agree though, that this does in no way allow you to solve any new 
problems, that you can’t solve today.

Part of the reason I like the idea, is that it lowers the threshold for and 
work involved in dropping in “If what I’m about to do goes badly, I want X to 
happen”.  Yeah, it’s often comparable to simply do X first, and revert or alter 
it later, but that easily leads to twice the commits, loss of locks, having to 
work around that, possibly introducing even more update-traffic in the process. 
 I just see a potential to both avoid that, and gain some developer-comfort at 
the same time, so I figured I should pitch the idea.

One could easily argue that session-level advisory locks could often be used to 
avoid the issues of loosing locks across the two transactions, and it wouldn’t 
be wrong.  It just moves the question back to complexity though, of which there 
would be more (imho).


Terje Elde



-- 
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] Feature suggestions: "dead letter"-savepoint.

[Craig Ringer]

On 23 June 2016 at 17:34, Terje Elde  wrote:


>
> But what if there’s a bug making a call to the external service?  Most of
> the time, you’ll trap the error and set status to something sane, but what
> if there’s a crash-bug in the SDK implementing it, or some other situation
> where things go very bad?  The rocket might be fired, then the client dies,
> lock is released, another worker picks up the task, and repeats the process
> ad nausium.
>
> Okay, so you’ll just update the row to say you’re trying to send it.  You
> set status=‘in-flight’ or some other status that’ll prevent the SELECT in
> other workers from picking up the task, and you commit that, so other
> workers won’t pick up the row if you die.  In the process though, you also
> loose the lock on the row.  You still want the row to be tied to you
> specifically, so you add a unique tag to the row, that later needs to be
> removed, so there’s more housekeeping.
>

It sounds like you're trying to re-invent distributed commit. Don't do
that. It's hard. Use 2PC and an external transaction co-ordinator that can
do in-doubt transaction resolution. On Java you want JTA. On Windows you
want MSDTC. In C you want to run screaming, um, I mean use XA.


> The basic idea is to be able to say “If I go belly up, I want this stuff
> to happen”.


This only papers over the problem rather weakly. What if the PostgreSQL
backend dies undexpectedly, not just the client? You've still got a
problem. Unless you're thinking of something that'd write to WAL then do
the work in some kind of critical section where if we fail we panic the
server and it gets done during WAL redo, or something like that.


> Depending on different needs, could be made persistent once the savepoint
> is taken, but for a lot of cases that wouldn’t really be needed.


It's starting to sound a lot like 2PC, you know.


> There’s some room for variation as well, such as having it support only
> dropped connections, or also support turning errors and/or rollbacks into
> rollback to and commits of the savepoint.  Ideally configurable at the
> point the snapshot it taken, to easily support pr. snapshot variation.
>

Very like 2PC.


> I did for a few moments wonder if prepared transactions would be a better
> place for something like this.  It could allow for named independent
> transactions, but there’s a fairly big mismatch between the two concepts.
> It also wouldn’t be too hard to use multiple named savepoints for
> effectively the same logic for most cases.  One advantage of prepared
> transactions is that it could perhaps also cover the case of a postgresql
> child dying, but that’s not exactly a common problem.  A huge dealbreaker
> though, is that the prepared transaction would very likely keep conflicting
> locks with the work to be done.
>

Actually, that's a benefit. It means you hold a lock on the row you're
working on until your coordinator determines whether the action was
actually performed or not, and commits or rolls back the 2PC prepared xact.
In the mean time nobody else tries to grab that same row and work on it.

Now, what I do think we need is to give the client the ability to determine
whether one of its xacts actually committed or not when it lost the session
after dispatching COMMIT but before getting a confirmation from the server
and persistently storing that knowledge. Right now if you want that you
have to do full 2PC. You shouldn't need to, you should be able to get the
xid when the xact is assigned it and store it somewhere locally. Then
later, if you're unsure if that xid committed or not due to a client crash
etc, you should be able to do some kind of SELECT pg_xact_is_committed(xid)
   to find out. Right now this is possible to write with a pretty simple
extension, but adds an extra roundtrip for a SELECT txid_current() call
(unless you pipeline it). I'd prefer that the server just tell you when an
xid is assigned. And yes, I think xid is the right identifier for this;
it's short, simple, and while it wraps around it takes long enough to do so
that it's very well suited for this job.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

[HACKERS] Extract Jsonb key and values

[hari.prasath]

Hi all,

I am having jsonb as C character string received by WAL decoding and want 
to extract all its key and value pairs. 
  
 
   Which is the best approach for extracting keys and its values? 
   
i)  Converting the C string to a PostgreSQL jsonb object
ii) Using open-source json-c library









cheers

- Harry

Re: [HACKERS] Question and suggestion about application binary compatibility policy

[Bruce Momjian]

On Thu, Jun 23, 2016 at 06:42:57AM +, Tsunakawa, Takayuki wrote:
> > From: Bruce Momjian [mailto:br...@momjian.us]
> > We have this text in src/tools/RELEASE_CHANGES:
> > ...
> > This is saying running against a mismatched minor version should be fine
> > for a binary.
> 
> Thanks for a good rationale.
> 
> 
> > I know this thread is old but it bounced around a lot of ideas.  I think
> > there are some open questions:
> > 
> > *  Will a new application link against an older minor-version libpq?
> > *  Will an old application link against a newer minor-version libpq?
> 
> The former does not always hold true, if the application uses a new libpq 
> function which is not in an old miner-version.  But I think the 
> backward-compatibility is enough.

Yes, I think that is correct, and I think that is covered in the file
posted:

Adding a new function should NOT force an increase in the major version
number.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
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] Bug in to_timestamp().

[Bruce Momjian]

On Thu, Jun 23, 2016 at 07:41:26AM +, amul sul wrote:
> On Monday, 20 June 2016 8:53 PM, Alex Ignatov  
> wrote:
> 
> 
> >>On 13.06.2016 18:52, amul sul wrote:
> >And it wont stop on some simple whitespace. By using to_timestamp you 
> >can get any output results by providing illegal input parameters values:
> 
> >postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD 
> >HH24:MI:SS');
> >   to_timestamp
> >
> >  2016-01-06 14:40:39+03
> >
> > (1 row)
> 
> We do consume extra space from input string, but not if it is in format 
> string, see below:
> 
> postgres=# SELECT TO_TIMESTAMP('2016-06-13  15:43:36', '/MM/DD 
> HH24:MI:SS'); 
> to_timestamp 
> 
> 2016-06-13 15:43:36-07
> (1 row)
> 
> We should have same treatment for format string too.
> 
> Thoughts? Comments?

Well, the user specifies the format string, while the input string comes
from the data, so I don't see having them behave the same as necessary.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
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] PQconnectdbParams vs PQconninfoParse

[Tom Lane]

Craig Ringer  writes:
> While writing some code that takes a connstring and adds some parameters, I
> noticed that PQconninfoParse doesn't play well with PQconnectdbParams.

> PQconnectdbParams takes a pair of equal-length arrays, one for keys and one
> for values, each terminated by null elements.  But PQconninfoParse returns
> a an array of PQconninfoOption .

> This means the client has to do a bunch of fiddling to turn a parsed
> conninfo into something that can be passed to PQconnectdbParams .  This
> seems bizarre. Am I missing something obvious?

Um, I don't see the connection.  Under what circumstances would you want
to pass the result of PQconninfoParse directly to PQconnectdbParams?
PQconninfoOption is intended to provide a lot of secondary information
about the available options, so it's more in the nature of an
informational record than something you would feed back into the library.

> libpq  internally uses connectOptions1 which calls parse_connection_string,
> the same function used by PQconninfoParse. But there's no connect variant
> exposed to users to use it.
> Anyone object to adding one?

This seems more like exposing library implementation details than adding
useful functionality.

In particular, I object to using PQconninfoOption as an input data
structure to the library, because then you'd have a bunch of definitional
questions about which fields of that struct the client app is expected to
make valid, plus the potential for well-hidden bugs if some part of the
library unintentionally relies on a field it shouldn't when looking at a
PQconninfoOption that came from outside the library rather than inside.

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] how is the WAL receiver process stopped and restarted when the network connection is broken and then restored?

[Rui Hai Jiang]

Thank you Craig for your suggestion.

I followed the clue and spent the whole day digging into the code.

Finally I figured out how the WAL receiver exits and restarts.

Question-1. How the WAL receiver process exits
===
When the network connection is broken, WAL receiver couldn't communicate
with the WAL sender.  For a long time (timer：wal_receiver_timeout), the WAL
receiver  gets nothing from the WAL sender, the WAL receiver process exits
by calling "ereport(ERROR,...)".

Calling ereport(ERROR,...) causes the current process exit, but calling
ereport(LOG,...) doesn't.

WalReceiverMain(void)
{
len = walrcv_receive(NAPTIME_PER_CYCLE, &buf);
if (len != 0)
{
}
else
{
if (wal_receiver_timeout > 0)
{
if (now >= timeout)
ereport(ERROR,
(errmsg("terminating walreceiver
due to timeout")));
}
}
}


Question-2. How  WAL receiver process starts again
=

At the Standby side, the startup process is responsible for recovery
processing. If streaming replication is configured and the startup process
finds that the WAL receiver process is not running, it notify the
Postmaster to start the WAL receiver process.Note: This is also how the WAL
receiver process starts for the first time!

(1) startup process notify Postmaster to start the WAL receiver by sending
a SIGUSR1.

RequestXLogStreaming()
{
if (launch)
SendPostmasterSignal(PMSignalReason
reason=PMSIGNAL_START_WALRECEIVER)
{
kill(PostmasterPid, SIGUSR1);
}
}

(2) Postmaster gets SIGUSR1 and starts the WAL receiver process.

sigusr1_handler(SIGNAL_ARGS)
{
WalReceiverPID = StartWalReceiver();
}


Please let me know if my understanding is incorrect.

thanks,
Rui Hai

Re: [HACKERS] Parallelized polymorphic aggs, and aggtype vs aggoutputtype

[Tom Lane]

David Rowley  writes:
> On 23 June 2016 at 11:22, Tom Lane  wrote:
>> The behavior that I'd expect (and that I documented) for a deserialization
>> function is that it just allocates its result in the current, short-lived
>> memory context, since it will be the combine function's responsibility to
>> merge that into the long-lived transition state.  But it looks to me like
>> the deserialization functions in numeric.c are allocating their results
>> in the aggregate context, which will mean a leak.  (For example,
>> numeric_avg_deserialize creates its result using makeNumericAggState
>> which forces the result into the agg context.)

> Yes, you're right.

> In the end I decided to add a makeNumericAggStateCurrentContext()
> function which does not perform any memory context switching at all.
> It seems like this can be used for the combine functions too, since
> they've already switched to the aggregate memory context. This should
> save a few cycles during aggregate combine, and not expend any extra
> as some alternatives, like adding a flag to makeNumericAggState().

You missed the ones using makePolyNumAggState --- I fixed that and
pushed 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] Bug in to_timestamp().

[Alex Ignatov]

On 23.06.2016 16:30, Bruce Momjian wrote:

On Thu, Jun 23, 2016 at 07:41:26AM +, amul sul wrote:

On Monday, 20 June 2016 8:53 PM, Alex Ignatov  wrote:



On 13.06.2016 18:52, amul sul wrote:

And it wont stop on some simple whitespace. By using to_timestamp you
can get any output results by providing illegal input parameters values:
postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD
HH24:MI:SS');
   to_timestamp

  2016-01-06 14:40:39+03

(1 row)

We do consume extra space from input string, but not if it is in format string, 
see below:

postgres=# SELECT TO_TIMESTAMP('2016-06-13  15:43:36', '/MM/DD 
HH24:MI:SS');
to_timestamp

2016-06-13 15:43:36-07
(1 row)

We should have same treatment for format string too.

Thoughts? Comments?

Well, the user specifies the format string, while the input string comes
from the data, so I don't see having them behave the same as necessary.



To be honest they not just behave differently.  to_timestamp is just 
incorrectly  handles input data and nothing else.There is no excuse for 
such behavior:


postgres=# SELECT TO_TIMESTAMP('20:-16-06:13: 15_43:!36', '/MM/DD 
HH24:MI:SS');

 to_timestamp
--
 0018-08-05 13:15:43+02:30:17
(1 row)



Alex Ignatov
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

[HACKERS] Rethinking representation of partial-aggregate steps

[Tom Lane]

I really dislike this aspect of the partial-aggregate patch:

typedef struct Agg
{
...
boolcombineStates;  /* input tuples contain transition states */
boolfinalizeAggs;   /* should we call the finalfn on agg states? */
boolserialStates;   /* should agg states be (de)serialized? */
...
} Agg;

Representing the various partial-aggregate behaviors as three bools seems
like a bad idea to me.  In the first place, this makes it look like there
are as many as eight possible behaviors, whereas only a subset of those
flag combinations are or ever will be supported (not that the comments
anywhere tell you that, much less which ones).  In the second place,
it leads to unreadable code like this:

/* partial phase */
count_agg_clauses(root, (Node *) partial_grouping_target->exprs,
  &agg_partial_costs, false, false, true);

/* final phase */
count_agg_clauses(root, (Node *) target->exprs, &agg_final_costs,
  true, true, true);
count_agg_clauses(root, parse->havingQual, &agg_final_costs, true,
  true, true);

Good luck telling whether those falses and trues actually do what they
should.

I'm inclined to think that we should aggressively simplify here, perhaps
replacing all three of these flags with a three-way enum, say

PARTIALAGG_NORMAL   /* simple aggregation */
PARTIALAGG_PARTIAL  /* lower phase of partial aggregation */
PARTIALAGG_FINAL/* upper phase of partial aggregation */

This embodies a couple of judgments that maybe someone would quibble with:
* we don't have any use for intermediate partial aggregation steps;
* we don't have any use for partial aggregation in which data transferred
up needn't be serialized.

But I can't really see how either of those would make sense for any
practical use-case, and I don't think we need to have a confusing and
bug-prone API in order to hold the door open to support them.

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] Bug in to_timestamp().

[David G. Johnston]

On Thu, Jun 23, 2016 at 12:16 PM, Alex Ignatov 
wrote:

>
> On 23.06.2016 16:30, Bruce Momjian wrote:
>
>> On Thu, Jun 23, 2016 at 07:41:26AM +, amul sul wrote:
>>
>>> On Monday, 20 June 2016 8:53 PM, Alex Ignatov 
>>> wrote:
>>>
>>>
>>> On 13.06.2016 18:52, amul sul wrote:
>
 And it wont stop on some simple whitespace. By using to_timestamp you
 can get any output results by providing illegal input parameters values:
 postgres=# SELECT TO_TIMESTAMP('2016-06-13 99:99:99', 'MMDD
 HH24:MI:SS');
to_timestamp
 
   2016-01-06 14:40:39+03

 (1 row)

>>> We do consume extra space from input string, but not if it is in format
>>> string, see below:
>>>
>>> postgres=# SELECT TO_TIMESTAMP('2016-06-13  15:43:36', '/MM/DD
>>> HH24:MI:SS');
>>> to_timestamp
>>> 
>>> 2016-06-13 15:43:36-07
>>> (1 row)
>>>
>>> We should have same treatment for format string too.
>>>
>>> Thoughts? Comments?
>>>
>> Well, the user specifies the format string, while the input string comes
>> from the data, so I don't see having them behave the same as necessary.
>>
>>
> To be honest they not just behave differently.  to_timestamp is just
> incorrectly  handles input data and nothing else.There is no excuse for
> such behavior:
>
> postgres=# SELECT TO_TIMESTAMP('20:-16-06:13: 15_43:!36', '/MM/DD
> HH24:MI:SS');
>  to_timestamp
> --
>  0018-08-05 13:15:43+02:30:17
> (1 row)
>

T
​o be honest I don't see how this is relevant to quoted content.  And
you've already made this point quite clearly - repeating it isn't
constructive.  This behavior has existed for a long time and I don't see
that changing it is a worthwhile endeavor.  I believe a new function is
required that has saner behavior.  Otherwise given good input and a
well-formed parse string the function does exactly what it is designed to
do.  Avoid giving it garbage and you will be fine.  Maybe wrap the call to
the in a function that also checks for the expected layout and RAISE
EXCEPTION if it doesn't match.

​David J.
​
​

Re: [HACKERS] Bug in to_timestamp().

[Alex Ignatov]

On 23.06.2016 19:37, David G. Johnston wrote:
On Thu, Jun 23, 2016 at 12:16 PM, Alex Ignatov 
mailto:a.igna...@postgrespro.ru>>wrote:



On 23.06.2016 16:30, Bruce Momjian wrote:

On Thu, Jun 23, 2016 at 07:41:26AM +, amul sul wrote:

On Monday, 20 June 2016 8:53 PM, Alex Ignatov
mailto:a.igna...@postgrespro.ru>> wrote:


On 13.06.2016 18:52, amul sul wrote:

And it wont stop on some simple whitespace. By using
to_timestamp you
can get any output results by providing illegal input
parameters values:
postgres=# SELECT TO_TIMESTAMP('2016-06-13 99
:99:99', 'MMDD
HH24:MI:SS');
   to_timestamp

  2016-01-06 14:40:39+03

(1 row)

We do consume extra space from input string, but not if it
is in format string, see below:

postgres=# SELECT TO_TIMESTAMP('2016-06-13 15:43:36',
'/MM/DD HH24:MI:SS');
to_timestamp

2016-06-13 15:43:36-07
(1 row)

We should have same treatment for format string too.

Thoughts? Comments?

Well, the user specifies the format string, while the input
string comes
from the data, so I don't see having them behave the same as
necessary.


To be honest they not just behave differently. to_timestamp is
just incorrectly  handles input data and nothing else.There is no
excuse for such behavior:

postgres=# SELECT TO_TIMESTAMP('20:-16-06:13: 15_43:!36',
'/MM/DD HH24:MI:SS');
 to_timestamp
--
 0018-08-05 13:15:43+02:30:17
(1 row)


T
​o be honest I don't see how this is relevant to quoted content.  And 
you've already made this point quite clearly - repeating it isn't 
constructive.  This behavior has existed for a long time and I don't 
see that changing it is a worthwhile endeavor.  I believe a new 
function is required that has saner behavior. Otherwise given good 
input and a well-formed parse string the function does exactly what it 
is designed to do.  Avoid giving it garbage and you will be fine. 
Maybe wrap the call to the in a function that also checks for the 
expected layout and RAISE EXCEPTION if it doesn't match.


​David J.
​
​
Arguing just like that one can say that we don't even need exception 
like "division by zero". Just use well-formed numbers in denominator...
Input data  sometimes can be generated automagically. Without exception 
throwing debugging stored function containing to_timestamp can be painful.

Re: [HACKERS] Rethinking representation of partial-aggregate steps

[Robert Haas]

On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane  wrote:
> I really dislike this aspect of the partial-aggregate patch:
>
> typedef struct Agg
> {
> ...
> boolcombineStates;  /* input tuples contain transition states */
> boolfinalizeAggs;   /* should we call the finalfn on agg states? 
> */
> boolserialStates;   /* should agg states be (de)serialized? */
> ...
> } Agg;
>
> Representing the various partial-aggregate behaviors as three bools seems
> like a bad idea to me.  In the first place, this makes it look like there
> are as many as eight possible behaviors, whereas only a subset of those
> flag combinations are or ever will be supported (not that the comments
> anywhere tell you that, much less which ones).  In the second place,
> it leads to unreadable code like this:
>
> /* partial phase */
> count_agg_clauses(root, (Node *) partial_grouping_target->exprs,
>   &agg_partial_costs, false, false, true);
>
> /* final phase */
> count_agg_clauses(root, (Node *) target->exprs, &agg_final_costs,
>   true, true, true);
> count_agg_clauses(root, parse->havingQual, &agg_final_costs, true,
>   true, true);
>
> Good luck telling whether those falses and trues actually do what they
> should.
>
> I'm inclined to think that we should aggressively simplify here, perhaps
> replacing all three of these flags with a three-way enum, say
>
> PARTIALAGG_NORMAL   /* simple aggregation */
> PARTIALAGG_PARTIAL  /* lower phase of partial aggregation */
> PARTIALAGG_FINAL/* upper phase of partial aggregation */
>
> This embodies a couple of judgments that maybe someone would quibble with:
> * we don't have any use for intermediate partial aggregation steps;
> * we don't have any use for partial aggregation in which data transferred
> up needn't be serialized.
>
> But I can't really see how either of those would make sense for any
> practical use-case, and I don't think we need to have a confusing and
> bug-prone API in order to hold the door open to support them.

Hmm, well I guess I would have to disagree with the idea that those
other modes aren't useful. I seem to recall that David had some
performance results showing that pushing partial aggregate steps below
an Append node resulted in a noticeable speed-up even in the absence
of any parallelism, presumably because it avoids whatever projection
the Append might do, and also improves icache and dcache locality.  So
it seems quite possible that you might want to have something like
this:

FinalizeAggregate
-> Gather
  -> IntermediateAggregate
-> Append
  -> PartialAggregate
-> Parallel SeqScan
  -> PartialAggregate
-> Parallel SeqScan
  -> PartialAggregate
-> Parallel SeqScan
  -> PartialAggregate
-> Parallel SeqScan

The partial aggregate plans need not serialize, because they are
passing data to the same process, and the intermediate aggregate is
there, too.

I do agree, however, that the three Boolean flags don't make the code
entirely easy to read.  What I might suggest is that we replace the
three Boolean flags with integer flags, something like this:

#define AGGOP_COMBINESTATES   0x1
#define AGGOP_SERIALIZESTATES  0x2
#define AGGOP_FINALIZEAGGS 0x4

#define AGGTYPE_SIMPLE (AGGOP_FINALIZEAGGS)
#define AGGTYPE_PARTIAL_SERIAL (AGGOP_SERIALIZESTATES)
#define AGGTYPE_FINALIZE_SERIAL
(AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS|AGG_SERIALIZESTATES)

Code that requests behavior can do so with the AGGTYPE_* constants,
but code that implements behavior can test AGGOP_* constants.  Then if
we decide we need new combinations in the future, we can just add
those --- e.g. AGGTYPE_PARTIAL = 0, AGGTYPE_FINALIZE =
AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS, AGGTYPE_INTERMEDIATE =
AGGOP_COMBINESTATES --- and have some hope that it will mostly work.

I actually think nearly all of the combinations are sensible, here,
although I think it may have been a mistake to overload the "serialize
states" flag to mean both "does this combining aggregate expect to
receive the serial type rather than the transition type?" and also
"does this partial aggregate output the serial type rather than the
transition type?".  In the example above, you'd want the
IntermediateAggregate to expect the transition type but output the
serial type.  Oops.

-- 
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] Bug in to_timestamp().

[David G. Johnston]

On Thursday, June 23, 2016, Alex Ignatov  wrote:

> Arguing just like that one can say that we don't even need exception like
> "division by zero". Just use well-formed numbers in denominator...
> Input data  sometimes can be generated automagically. Without exception
> throwing debugging stored function containing to_timestamp can be painful.
>

to_timestamp with its present behavior is, IMO, a poorly designed function
that would never be accepted today.  Concrete proposals for either fixing
or deprecating (or both) are welcome.  Fixing it should not
cause unnecessary errors to be raised.

My main point is that I'm inclined to deprecate it.

My second point is if you are going to use this badly designed function you
need to protect yourself.

My understanding is that is not going to change for 9.6.

David J.

Re: [HACKERS] Bug in to_timestamp().

[Robert Haas]

On Thu, Jun 23, 2016 at 12:37 PM, David G. Johnston
 wrote
> To be honest I don't see how this is relevant to quoted content.  And you've
> already made this point quite clearly - repeating it isn't constructive.
> This behavior has existed for a long time and I don't see that changing it
> is a worthwhile endeavor.  I believe a new function is required that has
> saner behavior.  Otherwise given good input and a well-formed parse string
> the function does exactly what it is designed to do.  Avoid giving it
> garbage and you will be fine.  Maybe wrap the call to the in a function that
> also checks for the expected layout and RAISE EXCEPTION if it doesn't match.

Well, I think other people are allowed to disagree about whether
changing it is a worthwhile endeavor.  I think there's an excellent
argument that the current behavior is stupid and broken and probably
nobody is intentionally relying on it.

Obviously, if somebody is relying on the existing behavior and we
change it and it breaks things, then that's bad, and a good argument
for worrying about backward-compatibility - e.g. by adding a new
function.  But who would actually like the behavior that an extra
space in the format string causes non-whitespace characters to get
skipped?  Next you'll be arguing that we can't fix a server crash
that's triggered by a certain query because somebody might be relying
on that query to restart the server...

-- 
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] Bug in to_timestamp().

[Robert Haas]

On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston
 wrote:
> to_timestamp with its present behavior is, IMO, a poorly designed function
> that would never be accepted today.  Concrete proposals for either fixing or
> deprecating (or both) are welcome.  Fixing it should not cause unnecessary
> errors to be raised.

Sheesh.  Who put you in charge of this?  You basically seem like you
are trying to shut up anyone who supports this change, and I don't
think that's right.  Alex's opinion is just as valid as yours -
neither more nor less - and he has every right to express it without
being told by you that his emails are "not constructive".

> My main point is that I'm inclined to deprecate it.

I can almost guarantee that would make a lot of users very unhappy.
This function is widely used.

> My second point is if you are going to use this badly designed function you
> need to protect yourself.

I agree that anyone using this function should test their format
strings carefully.

> My understanding is that is not going to change for 9.6.

That's exactly what is under discussion here.

-- 
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] Rethinking representation of partial-aggregate steps

[Tom Lane]

Robert Haas  writes:
> On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane  wrote:
>> I'm inclined to think that we should aggressively simplify here, perhaps
>> replacing all three of these flags with a three-way enum, say
>> 
>> PARTIALAGG_NORMAL   /* simple aggregation */
>> PARTIALAGG_PARTIAL  /* lower phase of partial aggregation */
>> PARTIALAGG_FINAL/* upper phase of partial aggregation */
>> 
>> This embodies a couple of judgments that maybe someone would quibble with:
>> * we don't have any use for intermediate partial aggregation steps;
>> * we don't have any use for partial aggregation in which data transferred
>> up needn't be serialized.

> Hmm, well I guess I would have to disagree with the idea that those
> other modes aren't useful. I seem to recall that David had some
> performance results showing that pushing partial aggregate steps below
> an Append node resulted in a noticeable speed-up even in the absence
> of any parallelism, presumably because it avoids whatever projection
> the Append might do, and also improves icache and dcache locality.

I don't believe that for one second, because introducing another layer of
intermediate aggregation implies another projection step, plus all the
other overhead of a Plan node.

Also, if we are going to support intermediate partial aggregation, the
current representation is broken anyway, since it lacks any ability
to distinguish serialization for the input states from serialization
for the output states, which is something you'd certainly need to
distinguish in this type of plan structure.

> I do agree, however, that the three Boolean flags don't make the code
> entirely easy to read.  What I might suggest is that we replace the
> three Boolean flags with integer flags, something like this:

Yeah, that's another way we could go.  I had been considering a variant
of that, which was to assign specific code values to the enum constants
and then invent macros that did bit-anding tests on them.  That ends up
being just about what you propose except that the compiler understands
the enum-ness of the behavioral alternatives, which seems like a good
thing.

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] Hash Indexes

[Robert Haas]

On Wed, Jun 22, 2016 at 10:13 PM, Amit Kapila  wrote:
>>  A scan that has seen the flag won't look at the
>> tuple in any case.
>
> Why so?  Assume that scan started on new bucket where
> split-in-progress flag was set, now it will not look at tuples that
> are marked as moved-by-split in this bucket, as it will assume to find
> all such tuples in old bucket.  Now, if allow Vacuum or someone else
> to remove tuples from old with just an Exclusive lock, it is quite
> possible that scan miss the tuple in old bucket which got removed by
> vacuum.

Oh, you're right.  So we really need to CLEAR the split-in-progress
flag before removing any tuples from the old bucket.  Does that sound
right?

-- 
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] Bug in to_timestamp().

[Alvaro Herrera]

David G. Johnston wrote:
> On Thursday, June 23, 2016, Alex Ignatov  wrote:
> 
> > Arguing just like that one can say that we don't even need exception like
> > "division by zero". Just use well-formed numbers in denominator...
> > Input data  sometimes can be generated automagically. Without exception
> > throwing debugging stored function containing to_timestamp can be painful.
> 
> to_timestamp with its present behavior is, IMO, a poorly designed function
> that would never be accepted today.

I'm not sure about that.

to_timestamp was added to improve compatibility with Oracle, by commit
b866d2e2d794.  I suppose the spec should follow what's documented here,

http://docs.oracle.com/cd/B19306_01/server.102/b14200/functions193.htm
http://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#i34924

and that wherever we deviate from that, is a bug that should be fixed.

-- 
Á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] Rethinking representation of partial-aggregate steps

[Robert Haas]

On Thu, Jun 23, 2016 at 1:25 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Jun 23, 2016 at 12:26 PM, Tom Lane  wrote:
>>> I'm inclined to think that we should aggressively simplify here, perhaps
>>> replacing all three of these flags with a three-way enum, say
>>>
>>> PARTIALAGG_NORMAL   /* simple aggregation */
>>> PARTIALAGG_PARTIAL  /* lower phase of partial aggregation */
>>> PARTIALAGG_FINAL/* upper phase of partial aggregation */
>>>
>>> This embodies a couple of judgments that maybe someone would quibble with:
>>> * we don't have any use for intermediate partial aggregation steps;
>>> * we don't have any use for partial aggregation in which data transferred
>>> up needn't be serialized.
>
>> Hmm, well I guess I would have to disagree with the idea that those
>> other modes aren't useful. I seem to recall that David had some
>> performance results showing that pushing partial aggregate steps below
>> an Append node resulted in a noticeable speed-up even in the absence
>> of any parallelism, presumably because it avoids whatever projection
>> the Append might do, and also improves icache and dcache locality.
>
> I don't believe that for one second, because introducing another layer of
> intermediate aggregation implies another projection step, plus all the
> other overhead of a Plan node.

Sure, but aggregating as early as possible will often have the effect
of dramatically reducing the number of tuples that have to pass
through upper levels of the plan tree, which seems it would frequently
far outweigh those considerations.  Anyway, you can go back and find
what he posted about this previously and look at it for yourself;
that's probably better than having an argument with me about his test
results.

> Also, if we are going to support intermediate partial aggregation, the
> current representation is broken anyway, since it lacks any ability
> to distinguish serialization for the input states from serialization
> for the output states, which is something you'd certainly need to
> distinguish in this type of plan structure.

I mentioned that in the very same email to which you were replying
when you wrote this, which makes me wonder whether you bothered to
read the whole thing.

>> I do agree, however, that the three Boolean flags don't make the code
>> entirely easy to read.  What I might suggest is that we replace the
>> three Boolean flags with integer flags, something like this:
>
> Yeah, that's another way we could go.  I had been considering a variant
> of that, which was to assign specific code values to the enum constants
> and then invent macros that did bit-anding tests on them.  That ends up
> being just about what you propose except that the compiler understands
> the enum-ness of the behavioral alternatives, which seems like a good
> thing.

I don't object to that.

-- 
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] Rethinking representation of partial-aggregate steps

[Alvaro Herrera]

Tom Lane wrote:
> Robert Haas  writes:

> > I do agree, however, that the three Boolean flags don't make the code
> > entirely easy to read.  What I might suggest is that we replace the
> > three Boolean flags with integer flags, something like this:
> 
> Yeah, that's another way we could go.  I had been considering a variant
> of that, which was to assign specific code values to the enum constants
> and then invent macros that did bit-anding tests on them.  That ends up
> being just about what you propose except that the compiler understands
> the enum-ness of the behavioral alternatives, which seems like a good
> thing.

Isn't that what you said not to do in 
https://www.postgresql.org/message-id/13345.1462383...@sss.pgh.pa.us ?
ISTM that in Robert's proposal it is allowed for a "flags" value to
have an OR'ed combination of multiple individual flags.  Unless you're
proposing to enumerate each allowed combination?

-- 
Á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] Bug in to_timestamp().

[Tom Lane]

Robert Haas  writes:
> On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston
>  wrote:
>> My understanding is that is not going to change for 9.6.

> That's exactly what is under discussion here.

I would definitely agree with David on that point.  Making to_timestamp
noticeably better on this score seems like a nontrivial project, and
post-beta is not the time for that sort of thing, even if we had full
consensus on what to do.  I'd suggest somebody work on a patch and put
it up for review in the next cycle.

Now, if you were to narrowly define the problem as "whether to skip
non-spaces for a space in the format", maybe that could be fixed
post-beta, but I think that's a wrongheaded approach.  to_timestamp's
issues with input that doesn't match the format are far wider than that.
IMO we should try to resolve the whole problem with one coherent change,
not make incremental incompatible changes at the margins.

At the very least I'd want to see a thought-through proposal that
addresses all three of these interrelated points:

* what should a space in the format match
* what should a non-space, non-format-code character in the format match
* how should we handle fields that are not exactly the width suggested
by the format

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] Bug in to_timestamp().

[Robert Haas]

On Thu, Jun 23, 2016 at 1:40 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston
>>  wrote:
>>> My understanding is that is not going to change for 9.6.
>
>> That's exactly what is under discussion here.
>
> I would definitely agree with David on that point.  Making to_timestamp
> noticeably better on this score seems like a nontrivial project, and
> post-beta is not the time for that sort of thing, even if we had full
> consensus on what to do.  I'd suggest somebody work on a patch and put
> it up for review in the next cycle.
>
> Now, if you were to narrowly define the problem as "whether to skip
> non-spaces for a space in the format", maybe that could be fixed
> post-beta, but I think that's a wrongheaded approach.  to_timestamp's
> issues with input that doesn't match the format are far wider than that.
> IMO we should try to resolve the whole problem with one coherent change,
> not make incremental incompatible changes at the margins.
>
> At the very least I'd want to see a thought-through proposal that
> addresses all three of these interrelated points:
>
> * what should a space in the format match
> * what should a non-space, non-format-code character in the format match
> * how should we handle fields that are not exactly the width suggested
> by the format

I'm not averse to some further study of those issues, and I think the
first two are closely related.  The third one strikes me as a somewhat
separate consideration that doesn't need to be addressed by the same
patch.

-- 
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] Bug in to_timestamp().

[David G. Johnston]

On Thursday, June 23, 2016, Robert Haas  wrote:

> On Thu, Jun 23, 2016 at 1:12 PM, David G. Johnston
> > wrote:
> > to_timestamp with its present behavior is, IMO, a poorly designed
> function
> > that would never be accepted today.  Concrete proposals for either
> fixing or
> > deprecating (or both) are welcome.  Fixing it should not cause
> unnecessary
> > errors to be raised.
>
> Sheesh.  Who put you in charge of this?  You basically seem like you
> are trying to shut up anyone who supports this change, and I don't
> think that's right.


>
I'm all for a change in this area - though I'm not impacted enough to
actually work on a design proposal.  And I'm not sure how asking for ideas
constitutes trying to shut people up.  Especially since if no one does
float a proposal we'll simply have this discussion next year when someone
new discovers how badly behaved this function is.


>  My main point is that I'm inclined to deprecate it.
>

> I can almost guarantee that would make a lot of users very unhappy.
> This function is widely used.
>
>
Tell people not to use.  We'd leave it in, unchanged, on backward
compatibility grounds.  This seems like acceptable behavior for the
project.  Am I mistaken?


> > My second point is if you are going to use this badly designed function
> you
> > need to protect yourself.
>
> I agree that anyone using this function should test their format
> strings carefully.
>
> > My understanding is that is not going to change for 9.6.
>
> That's exactly what is under discussion here.
>
>
Ok.  I'm having trouble seeing this justified as a bug fix - the docs
clearly state our behavior is intentional.  Improved behavior, yes, but
that's a feature and we're in beta2.  Please be specific if you believe
I've misinterpreted project policies on this matter.

David J.

Re: [HACKERS] Rethinking representation of partial-aggregate steps

[Simon Riggs]

On 23 June 2016 at 18:31, Robert Haas  wrote:

>
> Sure, but aggregating as early as possible will often have the effect
> of dramatically reducing the number of tuples that have to pass
> through upper levels of the plan tree, which seems it would frequently
> far outweigh those considerations.
>

Agreed

I can imagine plans where a useful aggregation occurs before every join, so
N > 2 is easily possible.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Bug in to_timestamp().

[Robert Haas]

On Thu, Jun 23, 2016 at 1:46 PM, David G. Johnston
 wrote:
>> Sheesh.  Who put you in charge of this?  You basically seem like you
>> are trying to shut up anyone who supports this change, and I don't
>> think that's right.
>
> I'm all for a change in this area - though I'm not impacted enough to
> actually work on a design proposal.

You weren't for a change in this area a few emails ago; back then, you
said "I don't see that changing it is a worthwhile endeavor".

> And I'm not sure how asking for ideas
> constitutes trying to shut people up.  Especially since if no one does float
> a proposal we'll simply have this discussion next year when someone new
> discovers how badly behaved this function is.

In my opinion, telling people that their emails are not constructive
and that no change is possible for 9.6 is pretty much the same thing
as trying to shut people up.  And that's what you did.

>>  My main point is that I'm inclined to deprecate it.
>>
>> I can almost guarantee that would make a lot of users very unhappy.
>> This function is widely used.
>
> Tell people not to use.  We'd leave it in, unchanged, on backward
> compatibility grounds.  This seems like acceptable behavior for the project.
> Am I mistaken?

Yes.  We're not going to deprecate widely-used functionality.  We
might, however, fix it, contrary to your representations upthread.

>> > My second point is if you are going to use this badly designed function
>> > you
>> > need to protect yourself.
>>
>> I agree that anyone using this function should test their format
>> strings carefully.
>>
>> > My understanding is that is not going to change for 9.6.
>>
>> That's exactly what is under discussion here.
>>
>
> Ok.  I'm having trouble seeing this justified as a bug fix - the docs
> clearly state our behavior is intentional.  Improved behavior, yes, but
> that's a feature and we're in beta2.  Please be specific if you believe I've
> misinterpreted project policies on this matter.

I would not vote to back-patch a change in this area because I think
that could break SQL code that works today.  But I think the current
behavior is pretty well indefensible.  It's neither intuitive nor
compatible with Oracle.  And there is plenty of existing precedent for
making this sort of change during the beta period.  We regard it as a
bug fix which is too dangerous to back-patch; therefore, it is neither
subject to feature freeze nor does it go into existing stable
releases.  Whether to treat this particular issue in that particular
way is something that needs to be hammered out in discussion.  Tom
raises the valid point that we need to make sure that we've thoroughly
studied this issue and fixed all of the related cases before
committing anything -- we don't want to change the behavior in
9.6beta, release 9.6, and then have to change it again for 9.7.  But
there is certainly no project policy which says we can't change this
now for 9.6 if we decide that (1) the current behavior is wrong and
(2) we are quite sure we know how to fix it.

-- 
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] Rethinking representation of partial-aggregate steps

[Alvaro Herrera]

Tom Lane wrote:

> What I'm imagining is, say,
> 
> #define AGGOP_COMBINESTATES   0x1
> #define AGGOP_SERIALIZESTATES  0x2
> #define AGGOP_DESERIALIZESTATES  0x4
> #define AGGOP_FINALIZEAGGS 0x8
> 
> typedef enum AggPartialMode
> {
>   AGGPARTIAL_SIMPLE = AGGOP_FINALIZEAGGS,
>   AGGPARTIAL_PARTIAL = AGGOP_SERIALIZESTATES,
>   AGGPARTIAL_FINAL = AGGOP_COMBINESTATES | AGGOP_DESERIALIZESTATES | 
> AGGOP_FINALIZEAGGS
> } AggPartialMode;
> 
> #define DO_AGGPARTIAL_COMBINE(apm)  (((apm) & AGGOP_COMBINESTATES) != 0)
> #define DO_AGGPARTIAL_SERIALIZE(apm)  (((apm) & AGGOP_SERIALIZESTATES) != 0)
> #define DO_AGGPARTIAL_DESERIALIZE(apm)  (((apm) & AGGOP_DESERIALIZESTATES) != 
> 0)
> #define DO_AGGPARTIAL_FINALIZE(apm)  (((apm) & AGGOP_FINALIZEAGGS) != 0)
> 
> 
> These enum constants satisfy the properties I mentioned before, but their
> assigned values are chosen to make the macros cheap.

Ah, sure, that makes sense.

-- 
Á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] Bug in to_timestamp().

[Tom Lane]

Robert Haas  writes:
> On Thu, Jun 23, 2016 at 1:40 PM, Tom Lane  wrote:
>> At the very least I'd want to see a thought-through proposal that
>> addresses all three of these interrelated points:
>> 
>> * what should a space in the format match
>> * what should a non-space, non-format-code character in the format match
>> * how should we handle fields that are not exactly the width suggested
>> by the format

> I'm not averse to some further study of those issues, and I think the
> first two are closely related.  The third one strikes me as a somewhat
> separate consideration that doesn't need to be addressed by the same
> patch.

If you think those issues are not interrelated, you have not thought
about it carefully enough.

As an example, what we can do to handle not-expected-width fields is
very different if the format is "DDMMYY" versus if it is "DD-MM-YY".
In the first case we have little choice but to believe that each
field is exactly two digits wide.  In the second case, depending on
how we decide to define matching of "-", we might be able to allow
the field widths to vary so that they're effectively "whatever is
between the dashes".  But that would require insisting that "-"
match a "-", or at least a non-alphanumeric, which is not how it
behaves today.

I don't want to twiddle these behaviors in 9.6 and then again next year.

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] Rethinking representation of partial-aggregate steps

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Yeah, that's another way we could go.  I had been considering a variant
>> of that, which was to assign specific code values to the enum constants
>> and then invent macros that did bit-anding tests on them.  That ends up
>> being just about what you propose except that the compiler understands
>> the enum-ness of the behavioral alternatives, which seems like a good
>> thing.

> Isn't that what you said not to do in 
> https://www.postgresql.org/message-id/13345.1462383...@sss.pgh.pa.us ?

No.  What I'm imagining is, say,


#define AGGOP_COMBINESTATES   0x1
#define AGGOP_SERIALIZESTATES  0x2
#define AGGOP_DESERIALIZESTATES  0x4
#define AGGOP_FINALIZEAGGS 0x8

typedef enum AggPartialMode
{
  AGGPARTIAL_SIMPLE = AGGOP_FINALIZEAGGS,
  AGGPARTIAL_PARTIAL = AGGOP_SERIALIZESTATES,
  AGGPARTIAL_FINAL = AGGOP_COMBINESTATES | AGGOP_DESERIALIZESTATES | 
AGGOP_FINALIZEAGGS
} AggPartialMode;

#define DO_AGGPARTIAL_COMBINE(apm)  (((apm) & AGGOP_COMBINESTATES) != 0)
#define DO_AGGPARTIAL_SERIALIZE(apm)  (((apm) & AGGOP_SERIALIZESTATES) != 0)
#define DO_AGGPARTIAL_DESERIALIZE(apm)  (((apm) & AGGOP_DESERIALIZESTATES) != 0)
#define DO_AGGPARTIAL_FINALIZE(apm)  (((apm) & AGGOP_FINALIZEAGGS) != 0)


These enum constants satisfy the properties I mentioned before, but their
assigned values are chosen to make the macros cheap.

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] Bug in to_timestamp().

[Jim Nasby]

On 6/23/16 12:29 PM, Alvaro Herrera wrote:

David G. Johnston wrote:

On Thursday, June 23, 2016, Alex Ignatov  wrote:


Arguing just like that one can say that we don't even need exception like
"division by zero". Just use well-formed numbers in denominator...
Input data  sometimes can be generated automagically. Without exception
throwing debugging stored function containing to_timestamp can be painful.


to_timestamp with its present behavior is, IMO, a poorly designed function
that would never be accepted today.


I'm not sure about that.

to_timestamp was added to improve compatibility with Oracle, by commit
b866d2e2d794.  I suppose the spec should follow what's documented here,

http://docs.oracle.com/cd/B19306_01/server.102/b14200/functions193.htm
http://docs.oracle.com/cd/B19306_01/server.102/b14200/sql_elements004.htm#i34924

and that wherever we deviate from that, is a bug that should be fixed.


+1

I'm also in favor of a parsing function that actually follows the format 
specifier, but not enough to write a patch.


One thing that I think could happen for 9.6 is documenting how you could 
get the desired results with one of the regex functions. Not the nicest 
way to handle this problem, but it is workable and having a regex 
example available for people to start with would be very helpful.

--
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] Bug in to_timestamp().

[David G. Johnston]

On Thu, Jun 23, 2016 at 2:00 PM, Robert Haas  wrote:

> On Thu, Jun 23, 2016 at 1:46 PM, David G. Johnston
>  wrote:
> >> Sheesh.  Who put you in charge of this?  You basically seem like you
> >> are trying to shut up anyone who supports this change, and I don't
> >> think that's right.
> >
> > I'm all for a change in this area - though I'm not impacted enough to
> > actually work on a design proposal.
>
> You weren't for a change in this area a few emails ago; back then, you
> said "I don't see that changing it is a worthwhile endeavor".
>

​I still don't see changing to_timestamp as being the right approach...but
that's just my opinion.  I do think that this area "text to timestamp
conversions" could stand to be improved.  I may not have been clear on that
distinction.​  But changing the behavior of a function that has been around
since 2000 seems to be just asking for trouble.


> > And I'm not sure how asking for ideas
> > constitutes trying to shut people up.  Especially since if no one does
> float
> > a proposal we'll simply have this discussion next year when someone new
> > discovers how badly behaved this function is.
>
> In my opinion, telling people that their emails are not constructive
> and that no change is possible for 9.6 is pretty much the same thing
> as trying to shut people up.  And that's what you did.
>

​I guess I should be a bit more careful to make sure that two separate
responses ​on different aspects of a topic cannot be so easily construed as
"this thread is pointless".  To be clear I did and still do believe that
getting a change in this area into 10.0 is worthwhile; and that our policy
(and present circumstances) appears to preclude this changing in 9.6.  But
as noted below this is just an observation.


> >>  My main point is that I'm inclined to deprecate it.
> >>
> >> I can almost guarantee that would make a lot of users very unhappy.
> >> This function is widely used.
> >
> > Tell people not to use.  We'd leave it in, unchanged, on backward
> > compatibility grounds.  This seems like acceptable behavior for the
> project.
> > Am I mistaken?
>
> Yes.  We're not going to deprecate widely-used functionality.  We
> might, however, fix it, contrary to your representations upthread.
>

​At this point I feel you are cherry-picking my words to fit your present
feelings.  I stand by everything I've written upthread regarding
deprecation and fixing.  I'm personally in favor of the former.  I'll add
that you are a committer, I am not.  The one thing going for change is if
we indeed exactly match the reference behavior and then document it as
being a compatibility function.  I hadn't fully pondered this goal and how
its plays into changing 16 year old behavior.  Obviously a new name for the
function doesn't cut it in this scenario.


> >> > My second point is if you are going to use this badly designed
> function
> >> > you
> >> > need to protect yourself.
> >>
> >> I agree that anyone using this function should test their format
> >> strings carefully.
> >>
> >> > My understanding is that is not going to change for 9.6.
> >>
> >> That's exactly what is under discussion here.
> >>
> >
> > Ok.  I'm having trouble seeing this justified as a bug fix - the docs
> > clearly state our behavior is intentional.  Improved behavior, yes, but
> > that's a feature and we're in beta2.  Please be specific if you believe
> I've
> > misinterpreted project policies on this matter.
>
> I would not vote to back-patch a change in this area because I think
> that could break SQL code that works today.  But I think the current
> behavior is pretty well indefensible.  It's neither intuitive nor
> compatible with Oracle.  And there is plenty of existing precedent for
> making this sort of change during the beta period.  We regard it as a
> bug fix which is too dangerous to back-patch; therefore, it is neither
> subject to feature freeze nor does it go into existing stable
> releases.  Whether to treat this particular issue in that particular
> way is something that needs to be hammered out in discussion.  Tom
> raises the valid point that we need to make sure that we've thoroughly
> studied this issue and fixed all of the related cases before
> committing anything -- we don't want to change the behavior in
> 9.6beta, release 9.6, and then have to change it again for 9.7.  But
> there is certainly no project policy which says we can't change this
> now for 9.6 if we decide that (1) the current behavior is wrong and
> (2) we are quite sure we know how to fix it.
>
>
Thank You.

I still conclude that given the general lack of complaints, the fact we are
at beta2, the age of the feature and nature of the "bug", and the
non-existence of a working patch even for HEAD, that 9.6 is not going to
see this behavior changed and you will be loath to back-patch a fix once
9.6 becomes stable.  I'll admit the possibility does exist but am not all
that keen on couching every statement of this form I make.  If you find my
i

Re: [HACKERS] Protocol buffer support for Postgres

[Flavius Anton]

On Tue, Apr 26, 2016 at 2:40:49PM -0700, David Fetter wrote:
> Should we see about making a more flexible serialization
> infrastructure?  What we have is mostly /ad hoc/, and has already
> caused real pain to the PostGIS folks, this even after some pretty
> significant and successful efforts were made in this direction.

Hi all. Is anybody working on this right now? I would like to pick
this task for the summer. First of all, what do you think about what
David said? Should we try and design a generic infrastructure for
similar serialization datatypes? If so, will we need to refactor some
pieces from the JSON/XML implementation? I looked over the code and it
seems nicely decoupled, but I am not sure what this would involve.
I've done this before for MySQL[1] (not yet completed), but I'd love
to try it for PostgreSQL too.

On Tue, Apr 26, 2016 at 11:23:11AM -0700, José Luis Tallón wrote:
> Have you investigated JSONB vs ProtoBuf space usage ?
>(the key being  the "B" -- Postgres' own binary JSON
> implementation)

This is something I can further investigate, but another (possibly
major) benefit of the Protocol Buffers over JSON is that they *still*
have a schema. I think they combine advantages from both structured
and schemaless data.

My best guess is that we shouldn't focus on abstracting *any*
serialization paradigm, but only the ones that have a schema (like
Thrift or Protocol Buffers). Speaking of schemas, where is the best
place to keep that? For MySQL I opted for a plain text file similar to
.trg files (the ones used by MySQL for keeping triggers).

I'd love to talk more about this.

Thank you.
Flavius Anton

[1] https://github.com/google/mysql-protobuf


-- 
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] Protocol buffer support for Postgres

[Greg Stark]

On Thu, Jun 23, 2016 at 8:15 PM, Flavius Anton  wrote:
>
> I'd love to talk more about this.

I thought quite a bit about this a few years ago but never really
picked up it to work on.

There are different use cases where this could be useful and different
approaches that could be useful for the different use cases.

One idea was to have a protocol buffer data type which used the typmod
as an identifier to specify the schema. Perhaps as an oid primary key
from some other table, or something like how enums are handled. That
would be used much like JSON is used by languages where that's natural
but might be useful if you're in an environment where data is being
provided in protobufs and you need to send them on in protobufs.

Another option would be to allow the output of your select query to be
read in a protobuf. That might be a feature to add in libpq rather
than in the server, or perhaps as a new protocol feature that libpq
would then just switch to which might make it easier to use from other
languages. That might make it easier to use Postgres as a data store
for an environment where everything is in protobufs without losing the
full power of SQL schemas in the database.

As an aside, have you seen Cap’n Proto? It looks more interesting to
me than protobufs. It fixes a lot of the corner cases where protobufs
end up with unbounded computational complexity and seems a lot
cleaner. I haven't thought about it much but I wonder if it would be
possible to actually use Cap'n Proto capabilities to grant access to
Postgres RLS labels too.

-- 
greg


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Umair Shahid]

On Fri, Jun 24, 2016 at 2:14 AM, Umair Shahid 
wrote:

>
> -- Forwarded message --
> From: Tom Lane 
> Date: Thu, Jun 23, 2016 at 9:32 PM
> Subject: Re: [pgsql-packagers] PG 9.6beta2 tarballs are ready
> To: Magnus Hagander 
> Cc: Umair Shahid , Dave Page <
> dp...@postgresql.org>, PostgreSQL Packagers <
> pgsql-packag...@postgresql.org>
>
>
> Magnus Hagander  writes:
> > That makes more sense as the joinrel stuff *has* been changed between the
> > two betas. I'm sure someone who's touched that code (Tom?) can comment on
> > that part..
>
> It still makes little sense to me, as the previous reports say that the
> problem happened during bootstrap, and the planner does not run
> during bootstrap.
>
> Could we get a look at debug_query_string in the coredump, to possibly
> narrow down where the crash is really happening?
>

Moving thread to -hackers ...

debug_query_string is

* "INSERT INTO pg_description  SELECT t.objoid, c.oid, t.objsubid,
t.description   FROM tmp_pg_description t, pg_class c WHERE c.relname =
t.classname;"*

Happening in "setup_description"


>
> > It's still strange that it doesn't affect woodlouse.
>
> Or any of the other Windows critters...
>
> regards, tom lane
>
>
>
> --
> Umair Shahid
> 2ndQuadrant - The PostgreSQL Support Company
> http://www.2ndQuadrant.com/
>

Re: [HACKERS] Protocol buffer support for Postgres

[Flavius Anton]

On Thu, Jun 23, 2016 at 1:50 PM, Greg Stark  wrote:
> On Thu, Jun 23, 2016 at 8:15 PM, Flavius Anton  wrote:
>>
>> I'd love to talk more about this.
>
> I thought quite a bit about this a few years ago but never really
> picked up it to work on.
>
> Another option would be to allow the output of your select query to be
> read in a protobuf. That might be a feature to add in libpq rather
> than in the server, or perhaps as a new protocol feature that libpq
> would then just switch to which might make it easier to use from other
> languages. That might make it easier to use Postgres as a data store
> for an environment where everything is in protobufs without losing the
> full power of SQL schemas in the database.

I agree on this one, I think it's the most /natural/ way of doing things.

> As an aside, have you seen Cap’n Proto? It looks more interesting to
> me than protobufs. It fixes a lot of the corner cases where protobufs
> end up with unbounded computational complexity and seems a lot
> cleaner.

I've seen it around for quite some time, but my fear is that it is not
(yet?) widely adopted. Protocol Buffers themselves are not that
popular, let alone Cap'n Proto. By the way, there's also the newer
FlatBuffers[1] project, from Google too. They seem a lot like Cap'n
Proto, though. I think it's doable to provide some sort of abstraction
for these protocols in PostgreSQL, so that it can support all of them
eventually, with minimum effort for adding a new one. However, I am
skeptical about the practical advantages of having /all/ of them
supported.

--
Flavius

[1] https://github.com/google/flatbuffers


-- 
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] Questionabl description in datatype.sgml

[Tatsuo Ishii]

> On Sat, Jun 18, 2016 at 11:58:58AM -0400, Tom Lane wrote:
>> Tatsuo Ishii  writes:
>> > In "8.13.2. Encoding Handling"
>> >
>> > When using binary mode to pass query parameters to the server
>> > and query results back to the client, no character set conversion
>> > is performed, so the situation is different.  In this case, an
>> > encoding declaration in the XML data will be observed, and if it
>> > is absent, the data will be assumed to be in UTF-8 (as required by
>> > the XML standard; note that PostgreSQL does not support UTF-16).
>> > On output, data will have an encoding declaration
>> > specifying the client encoding, unless the client encoding is
>> > UTF-8, in which case it will be omitted.
>> >
>> 
>> > In the first sentence shouldn't "no character set conversion" be "no
>> > encoding conversion"? PostgreSQL is doing client/server encoding
>> > conversion, rather than character set conversion.
>> 
>> I think the text is treating "character set conversion" as meaning
>> the same thing as "encoding conversion"; certainly I've never seen
>> any place in our docs that draws a distinction between those terms.
>> If you think there is a difference, maybe we need to define those
>> terms somewhere.
> 
> Uh, I think Unicode is a character set, and UTF8 is an encoding.  I
> think Tatsuo is right here.

Yes, a character set is different from an encoding. I though it's a
common understanding among people.

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] Reviewing freeze map code

[Andres Freund]

On 2016-06-21 16:32:03 -0400, Robert Haas wrote:
> On Tue, Jun 21, 2016 at 3:46 PM, Andres Freund  wrote:
> > On 2016-06-21 15:38:25 -0400, Robert Haas wrote:
> >> On Tue, Jun 21, 2016 at 1:49 PM, Andres Freund  wrote:
> >> >> I'm also a bit dubious that LockAcquire is safe to call in general
> >> >> with interrupts held.
> >> >
> >> > Looks like we could just acquire the tuple-lock *before* doing the
> >> > toast_insert_or_update/RelationGetBufferForTuple, but after releasing
> >> > the buffer lock. That'd allow us to do avoid doing the nested locking,
> >> > should make the recovery just a goto l2;, ...
> >>
> >> Why isn't that racey?  Somebody else can grab the tuple lock after we
> >> release the buffer content lock and before we acquire the tuple lock.
> >
> > Sure, but by the time the tuple lock is released, they'd have updated
> > xmax. So once we acquired that we can just do
> > if (xmax_infomask_changed(oldtup.t_data->t_infomask,
> >   infomask) 
> > ||
> > 
> > !TransactionIdEquals(HeapTupleHeaderGetRawXmax(oldtup.t_data),
> >  xwait))
> > goto l2;
> > which is fine, because we've not yet done the toasting.
> 
> I see.
> 
> > I'm not sure wether this approach is better than deleting potentially
> > toasted data though. It's probably faster, but will likely touch more
> > places in the code, and eat up a infomask bit (infomask & HEAP_MOVED
> > == HEAP_MOVED in my prototype).
> 
> Ugh.  That's not very desirable at all.

I'm looking into three approaches right now:

1) Flag approach from above
2) Undo toasting on concurrent activity, retry
3) Use WAL logging for the already_marked = true case.


1) primarily suffers from a significant amount of complexity. I still
have a bug in there that sometimes triggers "attempted to update
invisible tuple" ERRORs.  Otherwise it seems to perform decently
performancewise - even on workloads with many backends hitting the same
tuple, the retry-rate is low.

2) Seems to work too, but due to the amount of time the tuple is not
locked, the retry rate can be really high. As we perform significant
amount of work (toast insertion & index manipulation or extending a
file) , while the tuple is not locked, it's quite likely that another
session tries to modify the tuple inbetween.  I think it's possible to
essentially livelock.

3) This approach so far seems the best. It's possible to reuse the
xl_heap_lock record (in an afaics backwards compatible manner), and in
most cases the overhead isn't that large.  It's of course annoying to
emit more WAL, but it's not that big an overhead compared to extending a
file, or to toasting.  It's also by far the simplest fix.


Comments?


-- 
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] Rethinking representation of partial-aggregate steps

[David Rowley]

On 24 June 2016 at 05:25, Tom Lane  wrote:
> Robert Haas  writes:
>> Hmm, well I guess I would have to disagree with the idea that those
>> other modes aren't useful. I seem to recall that David had some
>> performance results showing that pushing partial aggregate steps below
>> an Append node resulted in a noticeable speed-up even in the absence
>> of any parallelism, presumably because it avoids whatever projection
>> the Append might do, and also improves icache and dcache locality.
>
> I don't believe that for one second, because introducing another layer of
> intermediate aggregation implies another projection step, plus all the
> other overhead of a Plan node.

It's certainly not difficult to mock up a test to prove it is faster.

create table t1 (a int not null);
insert into t1 select generate_series(1,100);
create table t2 (a int not null);
insert into t2 select generate_series(1,100);

select sum(c) from (select count(*) c from t1 union all select
count(*) from t2) t;
   sum
-
 200
(1 row)

Time: 82.038 ms
select count(*) from (select * from t1 union all select * from t2) t;
  count
-
 200
(1 row)

Time: 180.540 ms

-- 
 David Rowley   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] Reviewing freeze map code

[Alvaro Herrera]

Andres Freund wrote:

> I'm looking into three approaches right now:
> 
> 3) Use WAL logging for the already_marked = true case.


> 3) This approach so far seems the best. It's possible to reuse the
> xl_heap_lock record (in an afaics backwards compatible manner), and in
> most cases the overhead isn't that large.  It's of course annoying to
> emit more WAL, but it's not that big an overhead compared to extending a
> file, or to toasting.  It's also by far the simplest fix.

I suppose it's fine if we crash midway from emitting this wal record and
the actual heap_update one, since the xmax will appear to come from an
aborted xid, right?

I agree that the overhead is probably negligible, considering that this
only happens when toast is invoked.  It's probably not as great when the
new tuple goes to another page, though.

-- 
Á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] Reviewing freeze map code

[Andres Freund]

On 2016-06-23 18:59:57 -0400, Alvaro Herrera wrote:
> Andres Freund wrote:
> 
> > I'm looking into three approaches right now:
> > 
> > 3) Use WAL logging for the already_marked = true case.
> 
> 
> > 3) This approach so far seems the best. It's possible to reuse the
> > xl_heap_lock record (in an afaics backwards compatible manner), and in
> > most cases the overhead isn't that large.  It's of course annoying to
> > emit more WAL, but it's not that big an overhead compared to extending a
> > file, or to toasting.  It's also by far the simplest fix.
> 
> I suppose it's fine if we crash midway from emitting this wal record and
> the actual heap_update one, since the xmax will appear to come from an
> aborted xid, right?

Yea, that should be fine.


> I agree that the overhead is probably negligible, considering that this
> only happens when toast is invoked.  It's probably not as great when the
> new tuple goes to another page, though.

I think it has to happen in both cases unfortunately. We could try to
add some optimizations (e.g. only release lock & WAL log if the target
page, via fsm, is before the current one), but I don't really want to go
there in the back branches.

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] Rethinking representation of partial-aggregate steps

[David Rowley]

On 24 June 2016 at 05:12, Robert Haas  wrote:
> I do agree, however, that the three Boolean flags don't make the code
> entirely easy to read.  What I might suggest is that we replace the
> three Boolean flags with integer flags, something like this:
>
> #define AGGOP_COMBINESTATES   0x1
> #define AGGOP_SERIALIZESTATES  0x2
> #define AGGOP_FINALIZEAGGS 0x4
>
> #define AGGTYPE_SIMPLE (AGGOP_FINALIZEAGGS)
> #define AGGTYPE_PARTIAL_SERIAL (AGGOP_SERIALIZESTATES)
> #define AGGTYPE_FINALIZE_SERIAL
> (AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS|AGG_SERIALIZESTATES)
>
> Code that requests behavior can do so with the AGGTYPE_* constants,
> but code that implements behavior can test AGGOP_* constants.  Then if
> we decide we need new combinations in the future, we can just add
> those --- e.g. AGGTYPE_PARTIAL = 0, AGGTYPE_FINALIZE =
> AGGOP_COMBINESTATES|AGGOP_FINALIZEAGGS, AGGTYPE_INTERMEDIATE =
> AGGOP_COMBINESTATES --- and have some hope that it will mostly work.
>
> I actually think nearly all of the combinations are sensible, here,
> although I think it may have been a mistake to overload the "serialize
> states" flag to mean both "does this combining aggregate expect to
> receive the serial type rather than the transition type?" and also
> "does this partial aggregate output the serial type rather than the
> transition type?".  In the example above, you'd want the
> IntermediateAggregate to expect the transition type but output the
> serial type.  Oops.

I did consider using bit flags for this before, and it's a little bit
of an accident that it didn't end up that way. Originally there were
just two bool flags to control finalize and combine. A later patch
added the serialisation stuff which required the 3rd flag. It then
became a little untidy, but I hesitated to change it as I really just
wanted to keep the patch as easy to review as possible. In hindsight,
if I should've probably used bit flags from day one.

As for the above proposal, I do agree that it'll be cleaner with bit
flags, I just really don't see the need for the AGGTYPE_* alias
macros. For me it's easier to read if each option is explicitly listed
similar to how pull_var_clause() is done, e.g:

List   *vars = pull_var_clause((Node *) cur_em->em_expr,
  PVC_RECURSE_AGGREGATES |
  PVC_RECURSE_WINDOWFUNCS |
  PVC_INCLUDE_PLACEHOLDERS);

I'll start by writing the code to do that much, and if the consensus
is to add the alias macros too, then it's a small addition.

-- 
 David Rowley   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] Rethinking representation of partial-aggregate steps

[Tom Lane]

David Rowley  writes:
> As for the above proposal, I do agree that it'll be cleaner with bit
> flags, I just really don't see the need for the AGGTYPE_* alias
> macros. For me it's easier to read if each option is explicitly listed
> similar to how pull_var_clause() is done, e.g:

That does not sound to me like it does anything to address the issue of
documenting which combinations of flags are sensible/supported.  I prefer
something like the enum approach I suggested further downthread.

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] Feature suggestions: "dead letter"-savepoint.

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Craig Ringer
Now, what I do think we need is to give the client the ability to determine 
whether one of its xacts actually committed or not when it lost the session 
after dispatching COMMIT but before getting a confirmation from the server and 
persistently storing that knowledge. Right now if you want that you have to do 
full 2PC. You shouldn't need to, you should be able to get the xid when the 
xact is assigned it and store it somewhere locally. Then later, if you're 
unsure if that xid committed or not due to a client crash etc, you should be 
able to do some kind of SELECT pg_xact_is_committed(xid)to find out. Right 
now this is possible to write with a pretty simple extension, but adds an extra 
roundtrip for a SELECT txid_current() call (unless you pipeline it). I'd prefer 
that the server just tell you when an xid is assigned. And yes, I think xid is 
the right identifier for this; it's short, simple, and while it wraps around it 
takes long enough to do so that it's very well suited for this job.


This is interesting.  Oracle provides Transaction Guard for this.  Our 
customers also sometimes encounter the trouble of duplicate records in the 
database when, when their apps get disconnected during commit and reconnect to 
insert the same record again.

Regards
Takayuki Tsunakawa

Re: [HACKERS] Rethinking representation of partial-aggregate steps

[David Rowley]

On 24 June 2016 at 05:55, Tom Lane  wrote:
> #define AGGOP_COMBINESTATES   0x1
> #define AGGOP_SERIALIZESTATES  0x2
> #define AGGOP_DESERIALIZESTATES  0x4
> #define AGGOP_FINALIZEAGGS 0x8
>
> typedef enum AggPartialMode
> {
>   AGGPARTIAL_SIMPLE = AGGOP_FINALIZEAGGS,
>   AGGPARTIAL_PARTIAL = AGGOP_SERIALIZESTATES,
>   AGGPARTIAL_FINAL = AGGOP_COMBINESTATES | AGGOP_DESERIALIZESTATES | 
> AGGOP_FINALIZEAGGS
> } AggPartialMode;
>
> #define DO_AGGPARTIAL_COMBINE(apm)  (((apm) & AGGOP_COMBINESTATES) != 0)
> #define DO_AGGPARTIAL_SERIALIZE(apm)  (((apm) & AGGOP_SERIALIZESTATES) != 0)
> #define DO_AGGPARTIAL_DESERIALIZE(apm)  (((apm) & AGGOP_DESERIALIZESTATES) != 
> 0)
> #define DO_AGGPARTIAL_FINALIZE(apm)  (((apm) & AGGOP_FINALIZEAGGS) != 0)

The attached implements this, with the exception that I didn't really
think AggPartialMode was the best name for the enum. I've named this
AggregateMode instead, as the aggregate is only partial in some cases.
I also appended _SERIAL and _DESERIAL to the end of the partial and
final modes for now, as likely in 10.0 we'll be performing some
partial non-serialized aggregation and we'll likely want to keep those
other names for that instead.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


aggmode_bitflags.patch
Description: Binary data

-- 
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] Reviewing freeze map code

[Amit Kapila]

On Fri, Jun 24, 2016 at 4:33 AM, Andres Freund  wrote:
> On 2016-06-23 18:59:57 -0400, Alvaro Herrera wrote:
>> Andres Freund wrote:
>>
>> > I'm looking into three approaches right now:
>> >
>> > 3) Use WAL logging for the already_marked = true case.
>>
>>
>> > 3) This approach so far seems the best. It's possible to reuse the
>> > xl_heap_lock record (in an afaics backwards compatible manner), and in
>> > most cases the overhead isn't that large.  It's of course annoying to
>> > emit more WAL, but it's not that big an overhead compared to extending a
>> > file, or to toasting.  It's also by far the simplest fix.
>>

+1 for proceeding with Approach-3.

>> I suppose it's fine if we crash midway from emitting this wal record and
>> the actual heap_update one, since the xmax will appear to come from an
>> aborted xid, right?
>
> Yea, that should be fine.
>
>
>> I agree that the overhead is probably negligible, considering that this
>> only happens when toast is invoked.  It's probably not as great when the
>> new tuple goes to another page, though.
>
> I think it has to happen in both cases unfortunately. We could try to
> add some optimizations (e.g. only release lock & WAL log if the target
> page, via fsm, is before the current one), but I don't really want to go
> there in the back branches.
>

You are right, I think we can try such an optimization in Head and
that too if we see a performance hit with adding this new WAL in
heap_update.


-- 
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

[HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

Commit 9f03ca915 removed the only COPYTUP() call that could reach
copytup_index() in practice, making copytup_index() dead code.

The attached patch removes this dead code, in line with the existing
copytup_datum() case, where tuplesort.c also doesn't directly support
COPYTUP() (due to similar considerations around memory management).

-- 
Peter Geoghegan
From 8b2c0d3b1844caaf144971f488201e806c7fcc5e Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Thu, 23 Jun 2016 18:51:37 -0700
Subject: [PATCH] Remove dead COPYTUP routine in tuplesort.c

Commit 9f03ca915 made copytup_index() dead code.  Remove its body, and
leave only a defensive stub implementation.
---
 src/backend/utils/sort/tuplesort.c | 63 ++
 1 file changed, 2 insertions(+), 61 deletions(-)

diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 7878660..26eeab6 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -4492,67 +4492,8 @@ comparetup_index_hash(const SortTuple *a, const SortTuple *b,
 static void
 copytup_index(Tuplesortstate *state, SortTuple *stup, void *tup)
 {
-	IndexTuple	tuple = (IndexTuple) tup;
-	unsigned int tuplen = IndexTupleSize(tuple);
-	IndexTuple	newtuple;
-	Datum		original;
-
-	/* copy the tuple into sort storage */
-	newtuple = (IndexTuple) MemoryContextAlloc(state->tuplecontext, tuplen);
-	memcpy(newtuple, tuple, tuplen);
-	USEMEM(state, GetMemoryChunkSpace(newtuple));
-	stup->tuple = (void *) newtuple;
-	/* set up first-column key value */
-	original = index_getattr(newtuple,
-			 1,
-			 RelationGetDescr(state->indexRel),
-			 &stup->isnull1);
-
-	if (!state->sortKeys->abbrev_converter || stup->isnull1)
-	{
-		/*
-		 * Store ordinary Datum representation, or NULL value.  If there is a
-		 * converter it won't expect NULL values, and cost model is not
-		 * required to account for NULL, so in that case we avoid calling
-		 * converter and just set datum1 to zeroed representation (to be
-		 * consistent, and to support cheap inequality tests for NULL
-		 * abbreviated keys).
-		 */
-		stup->datum1 = original;
-	}
-	else if (!consider_abort_common(state))
-	{
-		/* Store abbreviated key representation */
-		stup->datum1 = state->sortKeys->abbrev_converter(original,
-		 state->sortKeys);
-	}
-	else
-	{
-		/* Abort abbreviation */
-		int			i;
-
-		stup->datum1 = original;
-
-		/*
-		 * Set state to be consistent with never trying abbreviation.
-		 *
-		 * Alter datum1 representation in already-copied tuples, so as to
-		 * ensure a consistent representation (current tuple was just
-		 * handled).  It does not matter if some dumped tuples are already
-		 * sorted on tape, since serialized tuples lack abbreviated keys
-		 * (TSS_BUILDRUNS state prevents control reaching here in any case).
-		 */
-		for (i = 0; i < state->memtupcount; i++)
-		{
-			SortTuple  *mtup = &state->memtuples[i];
-
-			tuple = (IndexTuple) mtup->tuple;
-			mtup->datum1 = index_getattr(tuple,
-		 1,
-		 RelationGetDescr(state->indexRel),
-		 &stup->isnull1);
-		}
-	}
+	/* Not currently needed */
+	elog(ERROR, "copytup_index() should not be called");
 }
 
 static void
-- 
2.7.4


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Odd behavior with domains

[Joshua D. Drake]

Hey,

So this came across my twitter feed:

https://pbs.twimg.com/media/ClqIJtmXEAA5IGt.png

I have verified the oddness with a newer version:

psql -U postgres
psql (9.5.3)
Type "help" for help.

postgres=# create domain text char(3);
CREATE DOMAIN
postgres=# create domain text char(2);
ERROR:  type "text" already exists
postgres=# \dD
 List of domains
 Schema | Name | Type | Modifier | Check
+--+--+--+---
(0 rows)

postgres=# create domain textd char(2);
CREATE DOMAIN
postgres=# \dD
 List of domains
 Schema | Name  | Type | Modifier | Check
+---+--+--+---
 public | textd | character(2) |  |
(1 row)






--
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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 05:17, Umair Shahid  wrote:

> On Fri, Jun 24, 2016 at 2:14 AM, Umair Shahid <
> umair.sha...@2ndquadrant.com> wrote:
>
>>
>> -- Forwarded message --
>> From: Tom Lane 
>> Date: Thu, Jun 23, 2016 at 9:32 PM
>> Subject: Re: [pgsql-packagers] PG 9.6beta2 tarballs are ready
>> To: Magnus Hagander 
>> Cc: Umair Shahid , Dave Page <
>> dp...@postgresql.org>, PostgreSQL Packagers <
>> pgsql-packag...@postgresql.org>
>>
>>
>> Magnus Hagander  writes:
>> > That makes more sense as the joinrel stuff *has* been changed between
>> the
>> > two betas. I'm sure someone who's touched that code (Tom?) can comment
>> on
>> > that part..
>>
>> It still makes little sense to me, as the previous reports say that the
>> problem happened during bootstrap, and the planner does not run
>> during bootstrap.
>>
>> Could we get a look at debug_query_string in the coredump, to possibly
>> narrow down where the crash is really happening?
>>
>
> Moving thread to -hackers ...
>
> debug_query_string is
>
> * "INSERT INTO pg_description  SELECT t.objoid, c.oid, t.objsubid,
> t.description   FROM tmp_pg_description t, pg_class c WHERE c.relname =
> t.classname;"*
>
> Happening in "setup_description"
>
>



I was helping Haroon with this last night. I don't have access to the
original thread and he's not around so I don't know how much he said. I'll
repeat our findings here.

During debugging I found that:

* A VS 2013 build (perfomed by Haroon and copied to the test host) crashes
consistently with the reported symptoms - "performing post-bootstrap
initialization ... child process was terminated by exception 0xC005"

* The issue doesn't happen in a VS 2015 build done on the test host

* I couldn't use just-in-time debugging because the restricted execution
token setup isolated the process. For the same reason, breakpoints stop
working in initdb.c after line 3557.

* To get a backtrace, I had to:

  * Launch a VS x86 command prompt
  * devenv /debugexe bin\initdb.exe -D test
  * Set a breakpoint in initdb.c:3557 and initdb.c:3307
  * Run
  * When it traps at get_restricted_token(), manually move the execution
pointer over the setup of the restricted execution token by dragging &
dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
comment it out and rebuild, but I was working with a supplied binary.
  * Continue until next breakpoint
  * Launch process explorer and find the pid of the postgres child process
  * Debug->attach to process, attach to the child postgres. This doesn't
detach the parent, VS does multiprocess debugging.
  * Continue execution
  * vs will trap on the child when it crashes

* It is an access violation (segfault) in postgres.exe when attempting to
read memory at 0x in calc_joinrel_size_estimate() at
costsize.c:3940

fkselec = get_foreign_key_join_selectivity(root,
  outer_rel->relids,
  inner_rel->relids,
  sjinfo,
  &restrictlist);

with debug_query_string:

0x09bf6140 "INSERT INTO pg_description  SELECT t.objoid, c.oid,
t.objsubid, t.description   FROM tmp_pg_description t, pg_class c WHERE
c.relname = t.classname;\n"



Backtrace:

Exception thrown at 0x0001401A5A81 in postgres.exe: 0xC005:
Access violation reading location 0x.

> postgres.exe!calc_joinrel_size_estimate(PlannerInfo * root, RelOptInfo *
outer_rel, RelOptInfo * inner_rel, double outer_rows, double inner_rows,
SpecialJoinInfo * sjinfo, List * restrictlist) Line 3944 C
  postgres.exe!set_joinrel_size_estimates(PlannerInfo * root, RelOptInfo *
rel, RelOptInfo * outer_rel, RelOptInfo * inner_rel, SpecialJoinInfo *
sjinfo, List * restrictlist) Line 3852 C
  postgres.exe!build_join_rel(PlannerInfo * root, Bitmapset * joinrelids,
RelOptInfo * outer_rel, RelOptInfo * inner_rel, SpecialJoinInfo * sjinfo,
List * * restrictlist_ptr) Line 521 C
  postgres.exe!make_join_rel(PlannerInfo * root, RelOptInfo * rel1,
RelOptInfo * rel2) Line 721 C
  postgres.exe!make_rels_by_clause_joins(PlannerInfo * root, RelOptInfo *
old_rel, ListCell * other_rels) Line 266 C
  postgres.exe!join_search_one_level(PlannerInfo * root, int level) Line 69
C
  postgres.exe!standard_join_search(PlannerInfo * root, int levels_needed,
List * initial_rels) Line 2172 C
  postgres.exe!query_planner(PlannerInfo * root, List * tlist,
void(*)(PlannerInfo *, void *) qp_callback, void * qp_extra) Line 255 C
  postgres.exe!grouping_planner(PlannerInfo * root, char
inheritance_update, double tuple_fraction) Line 1695 C
  postgres.exe!subquery_planner(PlannerGlobal * glob, Query * parse,
PlannerInfo * parent_root, char hasRecursion, double tuple_fraction) Line
775 C
  postgres.exe!standard_planner(Query * parse, int cursorOptions,
ParamListInfoData * boundParams) Line 312 C
  postgres.exe!pg_plan_query(Query * querytree, int cursorOptions,
ParamListInfoData * boundParams) Line 800 C
  postgres.exe!exec_simple_query(const char * query_string) Line 1023 C
  postgres.exe!PostgresMain(i

Re: [HACKERS] Hash Indexes

[Amit Kapila]

On Thu, Jun 23, 2016 at 10:56 PM, Robert Haas  wrote:
> On Wed, Jun 22, 2016 at 10:13 PM, Amit Kapila  wrote:
>>>  A scan that has seen the flag won't look at the
>>> tuple in any case.
>>
>> Why so?  Assume that scan started on new bucket where
>> split-in-progress flag was set, now it will not look at tuples that
>> are marked as moved-by-split in this bucket, as it will assume to find
>> all such tuples in old bucket.  Now, if allow Vacuum or someone else
>> to remove tuples from old with just an Exclusive lock, it is quite
>> possible that scan miss the tuple in old bucket which got removed by
>> vacuum.
>
> Oh, you're right.  So we really need to CLEAR the split-in-progress
> flag before removing any tuples from the old bucket.
>

I think that alone is not sufficient, we also need to out-wait any
scan that has started when the flag is set and till it is cleared.
Before vacuum starts cleaning particular bucket, we can certainly
detect if it has to clean garbage tuples (the patch sets has_garbage
flag in old bucket for split operation) and only for that case we can
out-wait the scans.   So probably, how it can work is during vacuum,
take Exclusive lock on bucket, check if has_garbage flag is set and
split-in-progress flag is cleared on bucket, if so then wait till the
pin-count on bucket is 1, else if has_garbage is not set, then just
proceed with clearing dead tuples from bucket.  This will reduce the
requirement of having cleanup lock only when it is required (namely
when bucket has garbage tuples).

-- 
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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Michael Paquier]

On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer  wrote:
>   * Launch a VS x86 command prompt
>   * devenv /debugexe bin\initdb.exe -D test
>   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
>   * Run
>   * When it traps at get_restricted_token(), manually move the execution
> pointer over the setup of the restricted execution token by dragging &
> dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
> comment it out and rebuild, but I was working with a supplied binary.
>   * Continue until next breakpoint
>   * Launch process explorer and find the pid of the postgres child process
>   * Debug->attach to process, attach to the child postgres. This doesn't
> detach the parent, VS does multiprocess debugging.
>   * Continue execution
>   * vs will trap on the child when it crashes

Do you think a crash dump could have been created by creating
crashdumps/ in PGDATA as part of initdb before this query is run?
-- 
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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 10:21, Craig Ringer  wrote:


> * To get a backtrace, I had to:
>
>   * Launch a VS x86 command prompt
>   * devenv /debugexe bin\initdb.exe -D test
>   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
>   * Run
>   * When it traps at get_restricted_token(), manually move the execution
> pointer over the setup of the restricted execution token by dragging &
> dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
> comment it out and rebuild, but I was working with a supplied binary.
>   * Continue until next breakpoint
>   * Launch process explorer and find the pid of the postgres child process
>   * Debug->attach to process, attach to the child postgres. This doesn't
> detach the parent, VS does multiprocess debugging.
>   * Continue execution
>   * vs will trap on the child when it crashes
>
>
Also, to save anyone else this hassle, I have saved a process dump (windows
core file) and the debug symbols to gdrive. You can get them at:

Note that you will need a Visual Studio version installed. VS Community
2015 works fine. You only need to install the C++ devenv and C++ headers,
you don't need MFC or any of the rest. The default install is fine if you
don't mind a bigger download.  Once installed, open postgres.dmp, then go
to debug->options, symbols. There, enable the Microsoft Symbol Server, and
also add a new entry for the absolute path to the symbols directory for the
archive you unpacked. You should enable the symbol cache directory too,
make a directory in your user dir and put it there.

If Haroon shared some gdrive links earlier on the thread I don't have
access to, this is the same data just efficiently compressed (32MB instead
of 180MB) and packaged up in a single convenient archive with the matching
sources and a full working install. You'll need 7zip to unpack it, but that
should be on your "install as soon as you install Windows" list anyway.

https://drive.google.com/open?id=0B7JKjZdzBUo1aE5DQnZ5VEpBUEk


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 10:28, Michael Paquier  wrote:

> On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer 
> wrote:
> >   * Launch a VS x86 command prompt
> >   * devenv /debugexe bin\initdb.exe -D test
> >   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
> >   * Run
> >   * When it traps at get_restricted_token(), manually move the execution
> > pointer over the setup of the restricted execution token by dragging &
> > dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
> > comment it out and rebuild, but I was working with a supplied binary.
> >   * Continue until next breakpoint
> >   * Launch process explorer and find the pid of the postgres child
> process
> >   * Debug->attach to process, attach to the child postgres. This doesn't
> > detach the parent, VS does multiprocess debugging.
> >   * Continue execution
> >   * vs will trap on the child when it crashes
>
> Do you think a crash dump could have been created by creating
> crashdumps/ in PGDATA as part of initdb before this query is run?
>

I see what you did there ;)

Yes, quite possibly, actually. I should've just got Haroon to build me a
new initdb without the priv setting and with creation of crashdumps/ .

It might be worth testing that out and adding an initdb startup flag to
create the directory, since initdb is such a PITA to debug.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Michael Paquier]

On Fri, Jun 24, 2016 at 11:33 AM, Craig Ringer  wrote:
> Yes, quite possibly, actually. I should've just got Haroon to build me a new
> initdb without the priv setting and with creation of crashdumps/ .
>
> It might be worth testing that out and adding an initdb startup flag to
> create the directory, since initdb is such a PITA to debug.

I was more thinking about putting that under -DDEBUG for example.
-- 
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] Odd behavior with domains

[Corey Huinker]

On Thu, Jun 23, 2016 at 10:16 PM, Joshua D. Drake 
wrote:

> Hey,
>
> So this came across my twitter feed:
>
> https://pbs.twimg.com/media/ClqIJtmXEAA5IGt.png
>
> I have verified the oddness with a newer version:
>
> psql -U postgres
> psql (9.5.3)
> Type "help" for help.
>
> postgres=# create domain text char(3);
> CREATE DOMAIN
> postgres=# create domain text char(2);
> ERROR:  type "text" already exists
> postgres=# \dD
>  List of domains
>  Schema | Name | Type | Modifier | Check
> +--+--+--+---
> (0 rows)
>
> postgres=# create domain textd char(2);
> CREATE DOMAIN
> postgres=# \dD
>  List of domains
>  Schema | Name  | Type | Modifier | Check
> +---+--+--+---
>  public | textd | character(2) |  |
> (1 row)
>
>
>
It's there.

 # create domain text char(3);
CREATE DOMAIN
labels_search=# \dD public.text
 List of domains
 Schema | Name | Type | Modifier | Check
+--+--+--+---
 public | text | character(3) |  |
(1 row)

I've noticed the same thing when creating types that mask an existing
catalog type.

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Tsunakawa, Takayuki]

> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> Sent: Friday, June 24, 2016 11:37 AM
> On Fri, Jun 24, 2016 at 11:33 AM, Craig Ringer 
> wrote:
> It might be worth testing that out and adding an initdb startup flag
> > to create the directory, since initdb is such a PITA to debug.
> 
> I was more thinking about putting that under -DDEBUG for example.
> 

I think just the existing option -d (--debug) and/or -n (--no-clean) would be 
OK.

Regards
Takayuki Tsunakawa



-- 
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] tuplesort.c's copytup_index() is dead code

[Tom Lane]

Peter Geoghegan  writes:
> Commit 9f03ca915 removed the only COPYTUP() call that could reach
> copytup_index() in practice, making copytup_index() dead code.

> The attached patch removes this dead code,

I think this may be premature in view of bug #14210.  Even if we
don't reinstate use of this function to fix that, I'm not really
convinced we want to get rid of it; it seems likely to me that
we might want it again.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Odd behavior with domains

[Alvaro Herrera]

Joshua D. Drake wrote:
> Hey,
> 
> So this came across my twitter feed:
> 
> https://pbs.twimg.com/media/ClqIJtmXEAA5IGt.png
> 
> I have verified the oddness with a newer version:

Well, it's not specifically related to domains -- it's related to the
fact that pg_catalog objects mask the domain you created in the public
schema, because pg_catalog is by default in front of all other schemas
unless you explicitely put it elsewhere.

alvherre=# create domain text char(3);
CREATE DOMAIN
alvherre=# \dD
  Listado de dominios
 Esquema | Nombre | Tipo | Modificador | Check 
-++--+-+---
(0 filas)

alvherre=# set search_path to 'public', 'pg_catalog';
SET
alvherre=# \dD
  Listado de dominios
 Esquema | Nombre | Tipo | Modificador | Check 
-++--+-+---
 public  | text   | character(3) | | 
(1 fila)

alvherre=# reset search_path;
RESET
alvherre=# \dD
  Listado de dominios
 Esquema | Nombre | Tipo | Modificador | Check 
-++--+-+---
(0 filas)

-- 
Á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] Odd behavior with domains

[Tom Lane]

"Joshua D. Drake"  writes:
> So this came across my twitter feed:
> https://pbs.twimg.com/media/ClqIJtmXEAA5IGt.png

public.text can exist in parallel with pg_catalog.text.

Nothing to see here, move along.

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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Michael Paquier]

On Fri, Jun 24, 2016 at 11:51 AM, Tsunakawa, Takayuki
 wrote:
>> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
>> Sent: Friday, June 24, 2016 11:37 AM
>> On Fri, Jun 24, 2016 at 11:33 AM, Craig Ringer 
>> wrote:
>> It might be worth testing that out and adding an initdb startup flag
>> > to create the directory, since initdb is such a PITA to debug.
>>
>> I was more thinking about putting that under -DDEBUG for example.
>>
>
> I think just the existing option -d (--debug) and/or -n (--no-clean) would be 
> OK.

If the majority thinks that an option switch is more adapted, I won't
fight it strongly. Just please let's not mess up with the behavior of
the existing options.
-- 
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] Odd behavior with domains

[Justin Dearing]

I was the one that reported that on twitter. I have a more detailed message
on the general list that I sent before subscribing and probably needs to be
moderated (or if it went to /dev/null let me know).

On Thu, Jun 23, 2016 at 11:01 PM Tom Lane  wrote:

> "Joshua D. Drake"  writes:
> > So this came across my twitter feed:
> > https://pbs.twimg.com/media/ClqIJtmXEAA5IGt.png
>
> public.text can exist in parallel with pg_catalog.text.
>
> It just doesn't seem right to me to be able to do:

CREATE DOMAIN int AS varchar(50);

Justin

Re: [HACKERS] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

On Thu, Jun 23, 2016 at 7:59 PM, Tom Lane  wrote:
> I think this may be premature in view of bug #14210.  Even if we
> don't reinstate use of this function to fix that, I'm not really
> convinced we want to get rid of it; it seems likely to me that
> we might want it again.

Oh, yes; that involves the same commit I mentioned. I'll look into #14210.

FWIW, I think that that bug tells us a lot about hash index usage in
the field. It took many months for someone to complain about what
ought to have been a really obvious bug. Clearly, hardly anybody is
using hash indexes. I broke hash index tuplesort builds in a similar
way at one point, too. The slightest bit of regression test coverage
would have caught either bug, I believe. I think that some minimal
regression tests should be added, because evidently they are needed.

-- 
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] tuplesort.c's copytup_index() is dead code

[Tom Lane]

Peter Geoghegan  writes:
> FWIW, I think that that bug tells us a lot about hash index usage in
> the field. It took many months for someone to complain about what
> ought to have been a really obvious bug. Clearly, hardly anybody is
> using hash indexes. I broke hash index tuplesort builds in a similar
> way at one point, too. The slightest bit of regression test coverage
> would have caught either bug, I believe.

We *do* have regression test coverage, but that code is set up to not
kick in at any index scale that would be sane to test in the regression
tests.  See
https://www.postgresql.org/message-id/12194.1466724...@sss.pgh.pa.us

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] tuplesort.c's copytup_index() is dead code

[Peter Geoghegan]

On Thu, Jun 23, 2016 at 8:35 PM, Tom Lane  wrote:
> We *do* have regression test coverage, but that code is set up to not
> kick in at any index scale that would be sane to test in the regression
> tests.  See
> https://www.postgresql.org/message-id/12194.1466724...@sss.pgh.pa.us

I'm well aware of that issue. This is the same reason why we don't
have any regression test coverage of external sorts. I don't think
that that's good enough.


-- 
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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 05:17, Umair Shahid  wrote:


>
>> > It's still strange that it doesn't affect woodlouse.
>>
>> Or any of the other Windows critters...
>> 
>>
>
Given that it's only been seen in VS 2013, it's particularly odd that it's
not biting woodlouse.

I'd like more details from those whose installs are crashing. What exact
vcvars env did you run under, with which exact cl.exe version?



-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Michael Paquier]

On Fri, Jun 24, 2016 at 1:28 PM, Craig Ringer  wrote:
> Given that it's only been seen in VS 2013, it's particularly odd that it's
> not biting woodlouse.
>
> I'd like more details from those whose installs are crashing. What exact
> vcvars env did you run under, with which exact cl.exe version?

Which OS did you use for the compilation? I don't think that this
matters much but woodloose is using Win7.
-- 
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...

[Michael Paquier]

On Wed, Jun 22, 2016 at 10:41 AM, Michael Paquier
 wrote:
> OK, there is not much that we can do here then. What about the rest?
> Those seem like legit concerns to me.

Registered this one to the next CF as a bug fix:
https://commitfest.postgresql.org/10/653/
It would be better not to crash if we can avoid it.
-- 
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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 12:31, Michael Paquier  wrote:

> On Fri, Jun 24, 2016 at 1:28 PM, Craig Ringer 
> wrote:
> > Given that it's only been seen in VS 2013, it's particularly odd that
> it's
> > not biting woodlouse.
> >
> > I'd like more details from those whose installs are crashing. What exact
> > vcvars env did you run under, with which exact cl.exe version?
>
> Which OS did you use for the compilation? I don't think that this
> matters much but woodloose is using Win7.
>



I'll have to wait for Haroon for that info for the crashing builds he did,
but I've now reproduced it with:

Windows server 2012 R2, VS 2013 Community Update 5, cross compile tools for
x86 to amd64.  cl 18.00.40629 for x64, env:

  %comspec% /k  ""C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\vcvarsall.bat" x86_amd64"

"where cl" reports

  C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\bin\x86_amd64\cl.exe

Note that cross compilation is a typical configuration on Windows, where
you routinely use 32bit x86 compilers to build 64bit code, except in the
newest SDKs.

I see the same symptoms, with the segfault.

This host is a clean install, an AWS instance created for the purpose.



It looks like woodlouse probably runs an older VS2013 and uses the native
x64 toolchain; its env includes:

  C:\\Program Files (x86)\\Microsoft Visual Studio 12.0\\VC\\BIN\\amd64

and does not have x86_amd64 in it.




BTW, I suggested to Haroon that he clone beta2 from git, then do a
git-bisect between beta1 (works) and beta2 (fails) to see if he can
identify the commit that causes things to start failing. I don't know how
far he got with that yesterday.


By comparison, I had no problems on the same host with VS Community 2015,
cl 19.00.23918, env "VS2015 x64 Native Tools Command Prompt":

   %comspec% /k ""C:\Program Files (x86)\Microsoft Visual Studio
14.0\VC\vcvarsall.bat"" amd64



On a side note I'm unable to build with vs2013 community u5 native tools (
for some reason. Link errors, unresolved external symbol _ischartype_l . cl
18.00.42629 for x64, env:

   %comspec% /k  ""C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\vcvarsall.bat" amd64"

"where cl" reports:

   C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\bin\amd64\cl.exe








-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 13:00, Craig Ringer  wrote:



>  I've now reproduced it with:
>


I can also confirm that it _doesn't_ crash with the same SDK using a 32-bit
build (running under WoW on x64). cl 18.00.40629 for x86, env:

  %comspec% /k  ""C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\vcvarsall.bat" x86"


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] [BUG] pg_basebackup from disconnected standby fails

[Michael Paquier]

On Thu, Jun 23, 2016 at 3:51 PM, Michael Paquier
 wrote:
> By the way, do you mind if I add this patch to the next CF? Better not
> to lose track of it...

Well, I have added an entry here at the end:
https://commitfest.postgresql.org/10/654/
Better doing it now before I forget about it...
-- 
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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 24 June 2016 at 10:28, Michael Paquier  wrote:

> On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer 
> wrote:
> >   * Launch a VS x86 command prompt
> >   * devenv /debugexe bin\initdb.exe -D test
> >   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
> >   * Run
> >   * When it traps at get_restricted_token(), manually move the execution
> > pointer over the setup of the restricted execution token by dragging &
> > dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
> > comment it out and rebuild, but I was working with a supplied binary.
> >   * Continue until next breakpoint
> >   * Launch process explorer and find the pid of the postgres child
> process
> >   * Debug->attach to process, attach to the child postgres. This doesn't
> > detach the parent, VS does multiprocess debugging.
> >   * Continue execution
> >   * vs will trap on the child when it crashes
>
> Do you think a crash dump could have been created by creating
> crashdumps/ in PGDATA as part of initdb before this query is run?
>


The answer is "yes" btw. Add "crashdumps" to the static array of
directories created by initdb and it works great.

Sigh. It'd be less annoying if I hadn't written most of the original patch.

For convenience I also commented out the check_root call in
src/backend/main.c and the get_restricted_token(progname) call in initdb.c,
so I could run it easily under an admin account where I can also install
tools etc without hassle. Not recommended on a non-throwaway machine of
course.

The generated crashdump shows the same crash in the same location.

I have absolutely no idea why it's trying to access memory at what looks
like   (uint64)(-1) though.  Nothing in the auto vars list:

+ &restrictlist 0x0043f7b0 {0x09e32600 {type=T_List (656)
length=1 head=0x09e325e0 {data={ptr_value=...} ...} ...}} List * *
+ inner_rel 0x09e7ad68 {type=T_EquivalenceClass (537)
reloptkind=RELOPT_BASEREL (0) relids=0x09e30520 {...} ...} RelOptInfo
*
+ inner_rel->relids 0x09e30520 {nwords=658 words=0x09e30524
{...} } Bitmapset *
+ outer_rel 0x0001401dec98
{postgres.exe!build_joinrel_tlist(PlannerInfo * root, RelOptInfo * joinrel,
RelOptInfo * input_rel), Line 646} {...} RelOptInfo *
+ outer_rel->relids 0xe808498b48d78b48 {nwords=??? words=0xe808498b48d78b4c
{...} } Bitmapset *
+ sjinfo 0x0043f870 {type=T_SpecialJoinInfo (543)
min_lefthand=0x09e7abd0 {nwords=1 words=0x09e7abd4 {...} }
...} SpecialJoinInfo *

or locals:

+ inner_rel 0x09e7ad68 {type=T_EquivalenceClass (537)
reloptkind=RELOPT_BASEREL (0) relids=0x09e30520 {...} ...} RelOptInfo
*
inner_rows 270.00 double
+ outer_rel 0x0001401dec98
{postgres.exe!build_joinrel_tlist(PlannerInfo * root, RelOptInfo * joinrel,
RelOptInfo * input_rel), Line 646} {...} RelOptInfo *
outer_rows 2.653351978175e-314#DEN double
+ restrictlist 0x09e32600 {type=T_List (656) length=1
head=0x09e325e0 {data={ptr_value=0x09e31788 ...} ...} ...} List
*
+ root 0x09e7b3f8 {type=1 parse=0x00504ad0
{type=T_AllocSetContext (601) commandType=CMD_UNKNOWN (0) ...} ...} PlannerInfo
*
+ sjinfo 0x0043f870 {type=T_SpecialJoinInfo (543)
min_lefthand=0x09e7abd0 {nwords=1 words=0x09e7abd4 {...} }
...} SpecialJoinInfo *

seems to fit. Though outer_rel->relids is a pretty weird address -
0xe808498b48d78b48? Really?

I'd point DrMemory at it, but unfortunately it only supports 32-bit
applications so far. I don't have access to any of the commerical tools
like Purify. Maybe someone at EDB can help out with that, if you guys do?

Register states are:

RAX = 0043F7B0 RBX = 09E32218 RCX = 09E78510 RDX =
09E7ABD0 RSI = 09E78510 RDI = 09E32218 R8  =
09E7B3F8 R9  = 09E7B1E8 R10 = 09E7A9C0 R11 =
0001 R12 = 09E32200 R13 =  R14 =
09E7B1E8 R15 =  RIP = 0001401A59D1 RSP =
0043F6E0 RBP = 09E7A9C0 EFL = 00010202

and the exact crash site is

fkselec = get_foreign_key_join_selectivity(root,
  outer_rel->relids,
  inner_rel->relids,
  sjinfo,
  &restrictlist);
0001401A59AB  mov r8,qword ptr [r8+8]
0001401A59AF  mov rdx,qword ptr [rdx+8]
0001401A59B3  movaps  xmmword ptr [rax-28h],xmm6
0001401A59B7  movaps  xmmword ptr [rax-38h],xmm7
0001401A59BB  movaps  xmmword ptr [rax-48h],xmm8
0001401A59C0  movaps  xmmword ptr [rax-58h],xmm9
0001401A59C5  lea rax,[rax+38h]
0001401A59C9  movaps  xmm7,xmm3
0001401A59CC  mov qword ptr [rsp+20h],rax
0001401A59D1  movaps  xmmword ptr [rax-68h],xmm10 < here
0001401A59D6  mov qword ptr [rax-48h],r14
0001401A59DA  mov r14,qword ptr [sjinfo]
0001401A59E2  mov ebp,dword ptr [r14+28h]
0001401A59E6  mov qword ptr [rax-50h]

Re: [HACKERS] Postgres_fdw join pushdown - wrong results with whole-row reference

[Etsuro Fujita]

On 2016/06/22 19:37, Ashutosh Bapat wrote:

On Wed, Jun 22, 2016 at 3:57 PM, Etsuro Fujita
Maybe I'm confused, but I think that in the system-column case it's
the user's responsibility to specify system columns for foreign
tables in a local query only when that makes sense on the remote
end, as shown in the below counter example:

postgres=# create foreign table fv1 (a int, b int) server myserver
options (table_name 'v1');
CREATE FOREIGN TABLE
postgres=# select ctid, * from fv1;
ERROR:  column "ctid" does not exist
CONTEXT:  Remote SQL command: SELECT a, b, ctid FROM public.v1



But a ctid, when available, would rightly get nullified when referenced
as a column of table.


Really?


What happens with the other system columns is we 0
them out locally, whether they are available at the foreign server or
not. We never try to check whether they are available at the foreign
server or not. If we use such column's column name to decide whether to
report 0 or null and if that column is not available at the foreign
table, we will get error. I think we should avoid that. Those column
anyway do not make any sense.


Agreed except for oid.  I think we should handle oid in the same way as 
ctid, which I'll work on in the next CF.


I think the proposed idea of applying record::text explicit coercion to 
a whole-row reference in the IS NOT NULL condition in the CASE WHEN 
conversion would work as expected as you explained, but I'm concerned 
that the cost wouldn't be negligible when the foreign table has a lot of 
columns.


Best regards,
Etsuro Fujita




--
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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Michael Paquier]

On Fri, Jun 24, 2016 at 3:22 PM, Craig Ringer  wrote:
>
>
> On 24 June 2016 at 10:28, Michael Paquier  wrote:
>>
>> On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer 
>> wrote:
>> >   * Launch a VS x86 command prompt
>> >   * devenv /debugexe bin\initdb.exe -D test
>> >   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
>> >   * Run
>> >   * When it traps at get_restricted_token(), manually move the execution
>> > pointer over the setup of the restricted execution token by dragging &
>> > dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
>> > comment it out and rebuild, but I was working with a supplied binary.
>> >   * Continue until next breakpoint
>> >   * Launch process explorer and find the pid of the postgres child
>> > process
>> >   * Debug->attach to process, attach to the child postgres. This doesn't
>> > detach the parent, VS does multiprocess debugging.
>> >   * Continue execution
>> >   * vs will trap on the child when it crashes
>>
>> Do you think a crash dump could have been created by creating
>> crashdumps/ in PGDATA as part of initdb before this query is run?
>
>
>
> The answer is "yes" btw. Add "crashdumps" to the static array of directories
> created by initdb and it works great.

As simple as attached..

> Sigh. It'd be less annoying if I hadn't written most of the original patch.

You mean the patch that created the crashdumps/ trick? This has saved
me a couple of months back to analyze a problem TBH.
-- 
Michael


dbg-initdb.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] Postgres_fdw join pushdown - wrong results with whole-row reference

[Ashutosh Bapat]

>
> I think the proposed idea of applying record::text explicit coercion to a
> whole-row reference in the IS NOT NULL condition in the CASE WHEN
> conversion would work as expected as you explained, but I'm concerned that
> the cost wouldn't be negligible when the foreign table has a lot of columns.
>

That's right, if the foreign server doesn't optimize the case for IS NOT
NULL, which it doesn't :)

I am happy to use any cheaper means e.g a function which counts number of
columns in a record. All we need here is a way to correctly identify when a
record is null and not null in the way we want (as described upthread). I
didn't find any quickly. Do you have any suggestions?


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company