Re: bytea bitwise logical operations implementation (xor / and / or / not)

[Fabien COELHO]

Hello Christian,


Currently, `bytea` does not have any bitwise logical operations yet.
This issue came up in an old thread from 2006 [1], but nobody seemed to
have picked this issue so far.


I remember this one because I needed them for checksuming set of rows. 
There is a whole set of missing (from my point of view) operators, casts 
and aggregates.


See https://github.com/zx80/pg_comparator where I packaged the subset I 
needed as an extension. In particular, there is a bitxor which can be used 
for bytea if casting is allowed.



Tested on PG 9.6. I hope you find this useful.


I think that the probability of getting these useful things into pg is 
alas small. In the mean time, you may package and register it as an 
extension?


--
Fabien.

Re: [HACKERS] postgres_fdw bug in 9.6

[Etsuro Fujita]

(2018/01/12 10:41), Thomas Munro wrote:

On Mon, Dec 11, 2017 at 8:25 PM, Etsuro Fujita
  wrote:

Now, if you're still super-concerned about this breaking something, we
could commit it only to master, where it will have 9 months to bake
before it gets released.  I think that's overly conservative, but I
think it's still better than waiting for the rewrite you'd like to see
happen.  We don't know when or if anyone is going to undertake that,
and if we wait, we may easing release a v11 that's got the same defect
as v9.6 and now v10.  And I don't see that we lose much by committing
this now even if that rewrite does happen in time for v11.  Ripping
out CreateLocalJoinPath() won't be any harder than ripping out
GetExistingLocalJoinPath().


Agreed.  Attached is an rebased version which moved the new fields in
JoinPathExtraData to the end of that struct.


FYI this doesn't compile anymore, because initial_cost_hashjoin() and
create_hashjoin_path() changed in master.


Thank you for letting me know about that!  Here is an updated version.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 1701,1722  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST, r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
!->  Merge Join
   Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!  Merge Cond: (t1.c1 = t2.c1)
!  ->  Sort
 Output: t1.c1, t1.c3, t1.*
!Sort Key: t1.c1
!->  Foreign Scan on public.ft1 t1
!  Output: t1.c1, t1.c3, t1.*
!  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!  ->  Sort
 Output: t2.c1, t2.*
!Sort Key: t2.c1
!->  Foreign Scan on public.ft2 t2
!  Output: t2.c1, t2.*
!  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
! (23 rows)
  
  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
   c1  | c1  
--- 1701,1716 
 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST, r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
!->  Nested Loop
   Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!  Join Filter: (t1.c1 = t2.c1)
!  ->  Foreign Scan on public.ft1 t1
 Output: t1.c1, t1.c3, t1.*
!Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!  ->  Foreign Scan on public.ft2 t2
 Output: t2.c1, t2.*
!Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
! (17 rows)
  
  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
   c1  | c1  
***
*** 1745,1766  SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST, r1."C 1" ASC NULLS LAST FOR UPDATE OF r1 FOR UPDATE OF r2
!->  Merge Join
   Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!   

Re: [HACKERS] Restrict concurrent update/delete with UPDATE of partition key

[amul sul]

On Thu, Jan 11, 2018 at 8:06 PM, Stephen Frost  wrote:
> Amul,
>
> * amul sul (sula...@gmail.com) wrote:
>> Agree, updated in the attached patch.  Patch 0001 also includes your
>> previous review comment[1] and typo correction suggested by Alvaro[2].
>
> Looks like this needs to be rebased (though the failures aren't too bad,
> from what I'm seeing), so going to mark this back to Waiting For Author.
> Hopefully this also helps to wake this thread up a bit and get another
> review of it.
>

Thanks for looking at this thread, attached herewith an updated patch rebase on
'UPDATE of partition key v35' patch[1].


Regards,
Amul


1] 
https://postgr.es/m/CAJ3gD9dixkkMzNnnP1CaZ1H17-U17ok_sVbjZZo+wnB=rjh...@mail.gmail.com


0001-Invalidate-ip_blkid-v4.patch
Description: Binary data


0002-isolation-tests-v3.patch
Description: Binary data

Re: PATCH: psql tab completion for SELECT

[Edmund Horner]

Hi, here's a new patch.

This one makes some changes to the criteria for which functions to
include; namely filtering out trigger functions and those that take or
return values of type "internal"; and including aggregate and window
functions.  Some of the other checks could be removed as they were
covered by the "internal" check.

It also uses the server version to determine which query to run.  I
have not written a custom query for every version from 7.1!  I've
split up the server history into:

pre-7.3 - does not even have pg_function_is_visible.  Not supported.
pre-9.0 - does not have several support functions in types, languages,
etc.  We don't bother filtering using columns in other tables.
pre-9.6 - does not have various aggregate support functions.
9.6 or later - the full query

I was able to test against 9.2, 9.6, and 10 servers, but compiling and
testing the older releases was a bit much for a Friday evening.  I'm
not sure there's much value in support for old servers, as long as we
can make completion queries fail a bit more gracefully.

Edmund


psql-select-tab-completion-v2.patch
Description: Binary data

Re: CREATE ROUTINE MAPPING

[David Fetter]

On Thu, Jan 11, 2018 at 09:37:43PM -0500, Corey Huinker wrote:
> A few months ago, I was researching ways for formalizing calling functions
> on one postgres instance from another. RPC, basically. In doing so, I
> stumbled across an obscure part of the the SQL Standard called ROUTINE
> MAPPING, which is exactly what I'm looking for.
> 
> The syntax specified is, roughly:
> 
> CREATE ROUTINE MAPPING local_routine_name FOR remote_routine_spec
> SERVER my_server [ OPTIONS( ... ) ]

[neat use cases elided]

For what it's worth, the now-defunct DBI-Link I wrote had
remote_execute(), which did many of the things you describe here, only
with no help from the rest of PostgreSQL, as it was implemented
strictly in userland.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Fri, Jan 12, 2018 at 4:06 PM, Michael Paquier 
wrote:

> On Fri, Jan 12, 2018 at 03:55:04PM +1100, Haribabu Kommi wrote:
> > Before posting the patch, first I did the same, upon further study
> > I didn't find any scenario where the value is not present in
> > conn->connhost[conn->whichhost].host and present in conn->pghost.
> >
> > If user provides "host" as connection option, the value is present
> > in both the variables. Even if the connection is unix domain socket,
> > there is a value in conn->connhost[conn->whichhost].host.
> >
> > In case if user provides only hostaddr and host connection option,
> > then in that case, both the members are NULL. So even if we add
> > that case, it will be dead code.
>
> Hm. Wouldn't it matter for cases where caller has not yet established a
> connection to the server but still calls PQhost to get the host string?
>

Yes I agree that the above scenario leads to a wrong result with the
earlier patch,
Updated patch attached by including the conn->pghost. Thanks for the review.

Regards,
Hari Babu
Fujitsu Australia


PQhost-update-to-return-proper-host-details_v2.patch
Description: Binary data

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

On Fri, Jan 12, 2018 at 03:55:04PM +1100, Haribabu Kommi wrote:
> Before posting the patch, first I did the same, upon further study
> I didn't find any scenario where the value is not present in
> conn->connhost[conn->whichhost].host and present in conn->pghost.
> 
> If user provides "host" as connection option, the value is present
> in both the variables. Even if the connection is unix domain socket,
> there is a value in conn->connhost[conn->whichhost].host.
> 
> In case if user provides only hostaddr and host connection option,
> then in that case, both the members are NULL. So even if we add
> that case, it will be dead code.

Hm. Wouldn't it matter for cases where caller has not yet established a
connection to the server but still calls PQhost to get the host string?
--
Michael


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Fri, Jan 12, 2018 at 3:26 PM, Michael Paquier 
wrote:

> On Fri, Jan 12, 2018 at 11:37:22AM +0900, Michael Paquier wrote:
> > I have redone my set of previous tests and can confirm that PQhost is
> > behaving as I would expect it should, and those results are the same as
> > yours.
>
> if (conn->connhost != NULL &&
> -   conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
> +   conn->connhost[conn->whichhost].host != NULL &&
> +   conn->connhost[conn->whichhost].host[0] != '\0')
> return conn->connhost[conn->whichhost].host;
> -   else if (conn->pghost != NULL && conn->pghost[0] != '\0')
> -   return conn->pghost;
>
> Upon further review, the second bit of the patch is making me itching. I
> think that you should not remove the second check which returns
> conn->pghost if a value is found in it.
>

Thanks for the review.

Before posting the patch, first I did the same, upon further study
I didn't find any scenario where the value is not present in
conn->connhost[conn->whichhost].host and present in conn->pghost.

If user provides "host" as connection option, the value is present
in both the variables. Even if the connection is unix domain socket,
there is a value in conn->connhost[conn->whichhost].host.

In case if user provides only hostaddr and host connection option,
then in that case, both the members are NULL. So even if we add
that case, it will be dead code.

I agree with your opinion of creating a new thread of this discussion.

Regards,
Hari Babu
Fujitsu Australia

Re: Parameters in user-defined aggregate final functions

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 11, 2018 at 6:11 PM, Tom Lane  wrote:
>> So define it as an ordered-set aggregate, and just ignore the question
>> of whether you need to sort the input (which is something that we leave
>> to the aggregate function to do anyway).  The syntax would be a little
>> weird/non-orthogonal, but you can blame the SQL committee for that.

> Or alternatively, don't define a final function at all, or define one
> that just serializes the transition state to JSON or whatever.

A third possibility, which preserves notational simplicity at some
cost, is just to define the aggregate as taking the additional
parameter(s) as regular aggregate inputs.  The transition function
would simply remember the last (or first) values of those parameters
as part of the transition state, and the final function would use
them from there.

The costs of this are:

1. you'd evaluate the additional params again at each row.  I think
this is probably not a big deal if they're just constants, but YMMV.

2. if the aggregate executes over zero input rows, you don't get an
opportunity to collect the extra params at all.  This might be fatal,
but it could also be a nonissue, either because you know your
application never aggregates over no rows, or because the correct
answer would be NULL or some such regardless of the extra params.

So there's more than one way to do it ...

regards, tom lane

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

On Fri, Jan 12, 2018 at 11:37:22AM +0900, Michael Paquier wrote:
> I have redone my set of previous tests and can confirm that PQhost is
> behaving as I would expect it should, and those results are the same as
> yours.

if (conn->connhost != NULL &&
-   conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
+   conn->connhost[conn->whichhost].host != NULL &&
+   conn->connhost[conn->whichhost].host[0] != '\0')
return conn->connhost[conn->whichhost].host;
-   else if (conn->pghost != NULL && conn->pghost[0] != '\0')
-   return conn->pghost;

Upon further review, the second bit of the patch is making me itching. I
think that you should not remove the second check which returns
conn->pghost if a value is found in it.
--
Michael


signature.asc
Description: PGP signature

Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [HACKERS] path toward faster partition pruning

[Amit Langote]

David,

On 2018/01/12 12:30, David Rowley wrote:
> Can you also perform a self-review of the patch? Some of the things
> I'm picking up are leftovers from a previous version of the patch. We
> might never get through this review if you keep leaving those around!

Sorry, I will look more closely before posting the next version.  I guess
I may have rushed a bit too much when posting the v18/v19 patches, partly
because it's been 3 weeks since v17 and I felt I needed to catch up
quickly given the activity on the run-time pruning thread which depends on
the patches here.

Thanks,
Amit

Re: [HACKERS] GUC for cleanup indexes threshold.

[Masahiko Sawada]

On Wed, Jan 10, 2018 at 11:28 AM, Masahiko Sawada  wrote:
> On Sun, Jan 7, 2018 at 8:40 AM, Peter Geoghegan  wrote:
>> On Sat, Jan 6, 2018 at 2:20 PM, Stephen Frost  wrote:
 > IIRC the patches that makes the cleanup scan skip has a problem
 > pointed by Peter[1], that is that we stash an XID when a btree page is
 > deleted, which is used to determine when it's finally safe to recycle
 > the page. Yura's patch doesn't have that problem?
 >
 > [1] 
 > https://www.postgresql.org/message-id/CAH2-Wz%3D1%3Dt5fcGGfarQGcAWBqaCh%2BdLMjpYCYHpEyzK8Qg6OrQ%40mail.gmail.com
>>
>>> Masahiko Sawada, if this patch isn't viable or requires serious rework
>>> to be acceptable, then perhaps we should change its status to 'returned
>>> with feedback' and you can post a new patch for the next commitfest..?
>>
>> I believe that the problem that I pointed out with freezing/wraparound
>> is a solvable problem. If we think about it carefully, we will come up
>> with a good solution. I have tried to get the ball rolling with my
>> pd_prune_xid suggestion. I think it's important to not lose sight of
>> the fact that the page deletion/recycling XID thing is just one detail
>> that we need to think about some more.
>>
>> I cannot fault Sawada-san for waiting to hear other people's views
>> before proceeding. It really needs to be properly discussed.
>>
>
> Thank you for commenting.
>
> IIUC we have two approaches: one idea is based on Peter's suggestion.
> We can use pd_prune_xid to store epoch of xid of which the page is
> deleted. That way, we can correctly mark deleted pages as recyclable
> without breaking on-disk format.
>
> Another idea is suggested by  Sokolov Yura. His original patch makes
> btree have a flag in btpo_flags that implies the btree has deleted but
> not recyclable page or not. I'd rather want to store it as bool in
> BTMetaPageData. Currently btm_version is defined as uint32 but I think
> we won't use all of them. If we store it in part of btm_version we
> don't break on-disk format. However, we're now assuming that the
> vacuum on btree index always scans whole btree rather than a part, and
> this approach will nurture it more. It might be possible that it will
> become a restriction in the future.
>

On third thought, it's similar to what we are doing for heaps but we
can store the oldest btpo.xact of a btree index to somewhere(metapage
or pg_class.relfrozenxid?) after vacuumed. We can skip cleanup
vacuuming as long as the xid is not more than a certain threshold old
(say vacuum_index_cleanup_age). Combining with new parameter I
proposed before, the condition of doing cleanup index vacuum will
become as follows.

if (there is garbage on heap)
   Vacuum index, and update oldest btpo.xact
else /* there is no garbage on heap */
{
if (# of scanned pages > (nblocks *
vacuum_cleanup_index_scale_factor) OR oldest btpo.xact is more than
vacuum_index_cleanup_age old?))
Cleanup vacuum index, and update oldest btpo.xact
}

In current index vacuuming, it scans whole index pages if there is
even one garbage on heap(I'd like to improve it though someday), which
it also lead to update the oldest btpo.xact at the time. If
vacuum_cleanup_index_slace_factor is enough high, we can skip the
scanning whole index as long as either the table isn't modified for 2
billion transactions or the oldest btpo.xact isn't more than
vacuum_index_cleanup_age transactions old. But if there is a opened
transaction for a very long time, we might not be able to mark deleted
page as recyclable. Some tricks might be required. Feedback is
welcome.

Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?

[Thomas Munro]

On Fri, Jan 12, 2018 at 4:19 PM, Robert Haas  wrote:
> On Thu, Jan 11, 2018 at 6:01 PM, Thomas Munro
>  wrote:
>> [ the data isn't session lifetime ]
>>
>> So I agree with Tom's suggestion:
>>
>> On Wed, Oct 4, 2017 at 2:29 PM, Tom Lane  wrote:
>>> Perhaps serialize the contents into an array in DSM, then rebuild a hash
>>> table from that in the worker.  Robert might have a better idea though.
>>
>> I'd happily volunteer to write or review a patch to do that.  Is there
>> a rebase of the stuff that got reverted, to build on?
>
> Those seem like reasons not to use Session, but not necessarily
> reasons not to have the leader directly build the dshash that the
> workers access rather than building a separate hash table in every
> worker.

Are you saying we should do the work now to create a per-transaction
DSM segment + DSA area + thing that every backend attaches to?  I
guess that would be much like the per-session one, except that we'd
reset it and end of xact (a DSA facility we don't have yet but which
would amount to zapping all superblocks, freeing all but one segment
or something).  Like the per-session one, I suppose it would be
created on demand when you run your first parallel query, and then
survive as long as the leader backend.  I assumed that we'd do that
work when we really need it for writable parallel query, but that it'd
be overkill for this.

Note that the shared record thing still has the backend local cache in
front of the shared registry, to avoid acquiring locks, so doesn't
actually avoid creating a per-backend hash table, though its entries
point directly to TupleDesc objects in shm.

> Maybe having every worker build a separate hash table is a good idea
> for some reason, but it's not clear to me that you've stated such a
> reason.

I didn't think creating backend local hash tables would be a problem
because it's a vanishingly rare occurrence for the hash table to be
created at all (ie when you've altered an enum), and if created, to
have more than a couple of entries in it.  But here's another idea, at
the small end of the how-much-work spectrum:

1.  Put the OIDs into a sorted array in the DSM segment.
2.  Arrange for EnumBlacklisted() (assuming we're talking about the
infrastructure from commit 1635e80d that was later reverted) to get
its hands on a pointer to that + size and binary search it, instead of
looking in the hash table (enum_blacklist), when running in a worker.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Stephen Frost]

Magnus,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Mon, Oct 2, 2017 at 3:29 PM, Michael Paquier
>  wrote:
> > On Mon, Oct 2, 2017 at 8:13 AM, Daniel Gustafsson  wrote:
> >> I’ve moved this to the next CF, but since this no longer applies cleanly 
> >> I’ve
> >> reset it to Waiting for author.
> >
> > Thanks Daniel for the reminder. Attached are rebased patches. This
> > thing rots easily...
> 
> This set of patches still applies, and is marked as ready for
> committer. Are any of the committers cited on this thread, aka Magnus,
> Heikki, Tom interested in this patch set? Or not? We are close to the
> end of CF 2017-11, so I am bumping it to the next one.

Magnus, this was your thread to begin with, though I know others have
been involved, any chance you'll be able to review this for commit
during this CF?  I agree that this is certainly a good thing to have
too, though I've not looked at the patch itself in depth.  Is there
anything we can do to help move it along?

Appears to compile and pass make check-world still (not sure why Thomas'
bot currently thinks the build fails, since it worked here..).

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [Sender Address Forgery]Re: [Sender Address Forgery]Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 12 January 2018 at 15:27, Amit Langote  wrote:
> On 2018/01/11 19:23, David Rowley wrote:

>> ERROR:  operator 531 is not a member of opfamily 1976
>
> You'll be able to see that the error no longer appears with the attached
> updated set of patches, but I'm now seeing that the resulting plan with
> patched for this particular query differs from what master (constraint
> exclusion) produces.  Master produces a plan with no partitions (as one
> would think is the correct plan), whereas patched produces a plan
> including the xy1 partition.  I will think about that a bit and post
> something later.

Thanks for looking at that.

I've got a few more things for you. I'm only partway through another
pass, but it makes sense to post what I have now if you're working on
a new version.

1. partitioing -> partitioning

 * Strategy of a partition clause operator per the partitioing operator class

2. get_partitions_from_clauses() modifies partclauses without
mentioning it in the header. I think you need to either:

a) warn about this in the header comment; or
b) do a list_copy() before list_concat()
c) do list_truncate back to the original length after you're done with the list.

3. get_partitions_from_clauses_recurse(), with:

  result = bms_add_range(result, 0, partdesc->nparts - 1);

You could change that to bms_add_range(NULL, ...) and ditch the
assignment of result to NULL at the start of the function.

4. classify_partition_bounding_keys() now returns bool, but the return
statement is still:

return keys->n_eqkeys + keys->n_minkeys + keys->n_maxkeys + n_keynullness;

my compiler didn't warn about that, but I'd imagine some might.

Instead, can you make it:

if (keys->n_eqkeys > 0 || keys->n_minkeys > 0 ||
keys->n_maxkeys > 0 || n_keynullness > 0)
return true;

return false;

probably equal keys are the most likely case, so it'll be good to
short circuit instead of performing addition on a bunch of stuff we
don't care about anymore.

5. In classify_partition_bounding_keys, why do we "continue" here?

clause = rinfo->clause;
if (rinfo->pseudoconstant &&
!DatumGetBool(((Const *) clause)->constvalue))
{
*constfalse = true;
continue;
}

Is there any point in searching further?

Also, if you were consistent with the return value for
classify_partition_bounding_keys when you've set *constfalse = true;
you wouldn't need to handle the case twice like you are in
get_partitions_from_clauses_recurse().

6. I think it would be nicer if get_partitions_from_ne_clauses returns
a set of partitions that could be excluded.

So instead of:

 * get_partitions_from_ne_clauses
 *
 * Return partitions of relation that satisfy all <> operator clauses in
 * ne_clauses.  Only ever called if relation is a list partitioned table.

Have:

 * get_partitions_from_ne_clauses
 *
 * Returns a Bitmapset of partitions that can be safely excluded due to
 * not-equal clauses existing for all possible partition values. It is only
 * valid to call this for LIST partitioned tables.

and instead of:

result = bms_add_range(NULL, 0, partdesc->nparts - 1);
result = bms_del_members(result, excluded_parts);
bms_free(excluded_parts);

return result;

Just do:

return excluded_parts;

and in get_partitions_from_clauses_recurse(), do bms_del_members
instead of bms_int_members.

there's less bit shuffling and it seems cleaner. Perhaps the function
name would need to be changed if we're inverting the meaning too.

(I've attached a patch which makes this change along with an idea in #8 below)

7. The following comment claims the function sets *datum, but there's
no param by that name:

/*
 * partkey_datum_from_expr
 * Extract constant value from expr and set *datum to that value
 */
static bool
partkey_datum_from_expr(PartitionKey key, int partkeyidx,
Expr *expr, Datum *value)

8. The code in get_partitions_from_ne_clauses() does perform quite a
few nested loops. I think a more simple way to would be to track the
offsets you've seen in a Bitmapset. This would save you having to
check for duplicates, as an offset can only contain a single datum.
You'd just need to build a couple of arrays after that, one to sum up
the offsets found per partition, and one for the total datums allowed
in the partition. If the numbers match then you can remove the
partition.

I've written this and attached it to this email. It saves about 50
lines of code and should perform much better for complex cases, for
example, a large NOT IN list. This also implements #6.

9. "the same" -> "it"

/*
* In case of NOT IN (..), we get a '<>', which while not
* listed as part of any operator family, we are able to
* handle the same if its negator is indeed a part of the
* partitioning operator family.
*/

10. in classify_partition_bounding_keys: "0" -> "false"

/* Return if no work to do below. */
if (!will_compute_keys || *constfalse)
return 0;

Likewise for:

if (*constfalse)
return 0;

11. I don't see partition_bound_bsearch used anywhere below the

Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?

[Robert Haas]

On Thu, Jan 11, 2018 at 6:01 PM, Thomas Munro
 wrote:
> [ the data isn't session lifetime ]
>
> So I agree with Tom's suggestion:
>
> On Wed, Oct 4, 2017 at 2:29 PM, Tom Lane  wrote:
>> Perhaps serialize the contents into an array in DSM, then rebuild a hash
>> table from that in the worker.  Robert might have a better idea though.
>
> I'd happily volunteer to write or review a patch to do that.  Is there
> a rebase of the stuff that got reverted, to build on?

Those seem like reasons not to use Session, but not necessarily
reasons not to have the leader directly build the dshash that the
workers access rather than building a separate hash table in every
worker.

Maybe having every worker build a separate hash table is a good idea
for some reason, but it's not clear to me that you've stated such a
reason.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Parameters in user-defined aggregate final functions

[Robert Haas]

On Thu, Jan 11, 2018 at 6:11 PM, Tom Lane  wrote:
> Esteban Zimanyi  writes:
>> How to tell PostgreSQL that my final function also needs a parameter? I am
>> working on PostgreSQL 10.1. I know that according to the documentation
>> direct parameters are only allowed for ordered-set aggregates, but I would
>> also need a direct parameter for "normal" aggregates.
>
> So define it as an ordered-set aggregate, and just ignore the question
> of whether you need to sort the input (which is something that we leave
> to the aggregate function to do anyway).  The syntax would be a little
> weird/non-orthogonal, but you can blame the SQL committee for that.

Or alternatively, don't define a final function at all, or define one
that just serializes the transition state to JSON or whatever.  Then
define some completely separate function that takes the transition
state (or the serialized representation thereof) and the additional
parameters and write something like:

SELECT 
completely_separate_nonaggregate_function(not_quite_the_aggregate_i_really_want(stuff),
42, 'omaha') FROM my_table;

Like Tom's proposal that's syntactically different but it should be
close enough.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: IndexTupleDSize macro seems redundant

[Robert Haas]

On Thu, Jan 11, 2018 at 9:17 PM, Stephen Frost  wrote:
> Great, thanks, I'll mark it as Ready For Committer then.
>
> Robert, since you were on this thread and the patch is mostly yours
> anyway, did you want to commit it?  I'm happy to do so also, either way.

Feel free.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Fix duplicated "the" occurrences in codebase

[Bruce Momjian]

On Mon, Oct 30, 2017 at 05:43:02PM +0100, Christoph Dreis wrote:
> Hey,
> 
> please find a patch attached that fixes duplicated "the" occurrences in the
> codebase.
> 
> As this is my first patch, please let me know in case I did something wrong.

Patch applied in git head.  Thanks.

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

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

On Wed, Jan 10, 2018 at 04:10:35PM +1100, Haribabu Kommi wrote:
> On Tue, Jan 9, 2018 at 12:15 PM, Michael Paquier 
> wrote:
>> Hm. Any users of psql's PROMPT would be equally confused, and this can
>> actually lead to more confusion from the user prospective I think than
>> just pg_stat_wal_receiver. If you take the case of Haribabu from
>> upthread with say this bit in psqlrc:
>> \set PROMPT1 '[host=%M;port=%>]=%# '
>>
>> Then you get on HEAD the following set of results using different
>> connection strings:
>> 1) host=localhost,localhost hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
>> [host=localhost,localhost;port=5432]=#
>>
>> 2) host=localhost,localhost port=5432,5433
>> [host=localhost;port=5432]=#
>>
>> 3) hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
>> [host=[local];port=5432]=#
>>
>> 4) host=/tmp,tmp hostaddr=127.0.0.1,127.0.0.1
>> [host=[local:/tmp,tmp];port=5432]=#
>>
>> So for cases 2) and 4), mixing both hostaddr and host is hurting the
>> experience. The documentation in [1] also specifies that if both host
>> and hostaddrs are specified then host is ignored. The same rule applies
>> for multiple values so for 2) and 4) the correct values ought to be
>> "local" for both of them. This would be more consistent with the pre-9.6
>> behavior as well.
>>
> 
> I think you mean to say for the cases 1) and 4)? because those are the
> cases where it differs with pre-9.6 behavior.

Sure, sorry for the mistake. That's indeed what I meant.

> With the attached patch of changing PQhost() to return the host if
> exists, irrespective of the connection type will bring back the
> pre-9.6 behavior. 
>
> 1) host=localhost,localhost hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
> [host=localhost;port=5432]=#
> 
> 4) host=/tmp,tmp hostaddr=127.0.0.1,127.0.0.1
> [host=[local];port=5432]=#
> 
> Even for default unix domain socket connection,
> conn->connhost[conn->whichhost].host
> is filled with the details, but not the global member. So no need of
> checking global member and returning the same in PQhost() function.

Thanks for the patch and the investigation, this visibly points out to
the fact that 11003eb5 did not get it completely right either. I am
adding Robert in CC for some input on the matter. To me, this looks like
a bug that should be applied down to v10. I think that it would be better
to spawn a new thread as well to raise awareness on the matter. This is
quite different than the patch you are presenting here. What do you
think?

I have redone my set of previous tests and can confirm that PQhost is
behaving as I would expect it should, and those results are the same as
yours.

With your patch, please note also that the SSL test suite does not
complain, which is an excellent thing!
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] using arc4random for strong randomness matters.

[Stephen Frost]

David, all,

* David CARLIER (devne...@gmail.com) wrote:
> > IIUC, what this code actually does is reseed itself from /dev/urandom
> > every so often and work from a PRNG in between.  That's not a layer that
> > we need, because the code on top is already designed to cope with the
> > foibles of /dev/urandom --- or, to the extent it isn't, that's something
> > we have to fix anyway.  So it seems like having this optionally in place
> > just reduces what we can assume about the randomness properties of
> > pg_strong_random output, which doesn't seem like a good idea.
>
> That I admit these are valid points.

Based on the discussion, it looks like this patch should be marked as
'Rejected', so I've gone ahead and done that.

We can reopen it if that's incorrect for some reason.

Thanks!

Stephen


signature.asc
Description: PGP signature

CREATE ROUTINE MAPPING

[Corey Huinker]

A few months ago, I was researching ways for formalizing calling functions
on one postgres instance from another. RPC, basically. In doing so, I
stumbled across an obscure part of the the SQL Standard called ROUTINE
MAPPING, which is exactly what I'm looking for.

The syntax specified is, roughly:

CREATE ROUTINE MAPPING local_routine_name FOR remote_routine_spec
SERVER my_server [ OPTIONS( ... ) ]


Which isn't too different from CREATE USER MAPPING.

The idea here is that if I had a local query:

SELECT t.x, remote_func1(),  remote_func2(t.y)

FROM remote_table t

WHERE t.active = true;


that would become this query on the remote side:

SELECT t.x, local_func1(), local_func2(t.y)

FROM local_table t

WHERE t.active = true;



That was probably the main intention of this feature, but I see a different
possibility there. Consider the cases:

SELECT remote_func(1,'a');


and

SELECT * FROM remote_srf(10, true);


Now we could have written remote_func() and remote_srf() in plpythonu, and
it could access whatever remote data that we wanted to see, but that
exposes our local server to the untrusted pl/python module as well as
python process overhead.

We could create a specialized foreign data wrapper that requires a WHERE
clause to include all the require parameters as predicates, essentially
making every function a table, but that's awkward and unclear to an end
user.

Having the ability to import functions from other servers allows us to
write foreign servers that expose functions to the local database, and
those foreign servers handle the bloat and risks associated with accessing
that remote data.

Moreover, it would allow hosted environments (AWS, etc) that restrict the
extensions that can be added to the database to still connect to those
foreign data sources.

I'm hoping to submit a patch for this someday, but it touches on several
areas of the codebase where I have no familiarity, so I've put forth to
spark interest in the feature, to see if any similar work is underway, or
if anyone can offer guidance.

Thanks in advance.

Re: Transform for pl/perl

[Thomas Munro]

On Thu, Dec 7, 2017 at 10:56 PM, Anthony Bykov  wrote:
>> Please, find a new version of the patch in attachments to this
>> message.

Hi again Anthony,

I wonder why make check passes for me on my Mac, but when Travis CI
(Ubuntu Trusty on amd64) runs it, it fails like this:

test jsonb_plperl ... FAILED
test jsonb_plperl_relocatability ... ok
test jsonb_plperlu... FAILED
test jsonb_plperlu_relocatability ... ok

= Contents of ./contrib/jsonb_plperl/regression.diffs
*** 
/home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/expected/jsonb_plperl.out
2018-01-11 21:46:35.867584467 +
--- 
/home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperl.out
2018-01-11 21:55:08.564204175 +
***
*** 89,96 
  (1 row)

  SELECT testSVToJsonb2('1E+131071');
! ERROR:  could not transform to type "jsonb"
! DETAIL:  The type you are trying to transform can't be transformed to jsonb
  CONTEXT:  PL/Perl function "testsvtojsonb2"
  SELECT testSVToJsonb2('-1');
   testsvtojsonb2
--- 89,95 
  (1 row)

  SELECT testSVToJsonb2('1E+131071');
! ERROR:  invalid input syntax for type numeric: "inf"
  CONTEXT:  PL/Perl function "testsvtojsonb2"
  SELECT testSVToJsonb2('-1');
   testsvtojsonb2
==
*** 
/home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/expected/jsonb_plperlu.out
2018-01-11 21:46:35.867584467 +
--- 
/home/travis/build/postgresql-cfbot/postgresql/contrib/jsonb_plperl/results/jsonb_plperlu.out
2018-01-11 21:55:08.704204228 +
***
*** 89,96 
  (1 row)

  SELECT testSVToJsonb2('1E+131071');
! ERROR:  could not transform to type "jsonb"
! DETAIL:  The type you are trying to transform can't be transformed to jsonb
  CONTEXT:  PL/Perl function "testsvtojsonb2"
  SELECT testSVToJsonb2('-1');
   testsvtojsonb2
--- 89,95 
  (1 row)

  SELECT testSVToJsonb2('1E+131071');
! ERROR:  invalid input syntax for type numeric: "inf"
  CONTEXT:  PL/Perl function "testsvtojsonb2"
  SELECT testSVToJsonb2('-1');
   testsvtojsonb2
==

-- 
Thomas Munro
http://www.enterprisedb.com

Re: IndexTupleDSize macro seems redundant

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Updated (combined) patch attached for review.  I went through and looked
> > again to make sure there weren't any cases of making an unaligned
> > pointer to a struct and didn't see any, and I added some comments to
> > _bt_restore_page().
> 
> Looks OK from here.

Great, thanks, I'll mark it as Ready For Committer then.

Robert, since you were on this thread and the patch is mostly yours
anyway, did you want to commit it?  I'm happy to do so also, either way.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Add default role 'pg_access_server_files'

[Stephen Frost]

Thomas,

* Thomas Munro (thomas.mu...@enterprisedb.com) wrote:
> On Mon, Jan 1, 2018 at 8:19 AM, Stephen Frost  wrote:
> > This patch adds a new default role called 'pg_access_server_files' which
> > allows an administrator to GRANT to a non-superuser role the ability to
> > access server-side files through PostgreSQL (as the user the database is
> > running as).  By itself, having this role allows a non-superuser to use
> > server-side COPY and to use file_fdw (if installed by a superuser and
> > GRANT'd USAGE on it).
> 
> Not sure if you are aware of this failure?
> 
> test file_fdw ... FAILED

Thanks, I did do a make check-world, but I tend to do them in parallel
and evidently missed this.  The patch needs to be reworked based on
discussion with Magnus anyhow, which I hope to do this weekend.
Currently trying to push other patches along. :)

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Peter Geoghegan]

On Thu, Jan 11, 2018 at 2:26 PM, Robert Haas  wrote:
> While the patch contains, as I said before, an excellent set of how-to
> directions explaining how to use the new parallel sort facilities in
> tuplesort.c, there seems to be no such thing for logtape.c, and as a
> result I find it a bit unclear how the interface is supposed to work.
> I think it would be good to add a similar summary here.

Okay. I came up with something for that.

> It seems like the words "leader" and "worker" here refer to the leader
> of a parallel operation and the associated workers, but do we really
> need to make that assumption?  Couldn't we generally describe this as
> merging a bunch of 1-tape LogicalTapeSets created from a SharedFileSet
> into a single LogicalTapeSet that can thereafter be read by the
> process that does the merging?

It's not so much an assumption as it is the most direct way of
referring to these various objects. logtape.c is very clearly a
submodule of tuplesort.c, so this felt okay to me. There are already
several references to what tuplesort.c expects. I'm not going to argue
about it if you insist on this, though I do think that trying to
describe things in more general terms would be a net loss. It would
kind of come off as feigning ignorance IMV. There is nothing that
logtape.c could know less about other than names/roles, and I find it
hard to imagine those changing, even when we add support for
partitioning/distribution sort (where logtape.c handles
"redistribution", something discussed early in this project's
lifetime).

> +/* Pass worker BufFile pieces, and a placeholder leader piece */
> +for (i = 0; i < lts->nTapes; i++)
> +{
> +lt = >tapes[i];
> +
> +/*
> + * Build concatenated view of all BufFiles, remembering the block
> + * number where each source file begins.
> + */
> +if (i < lts->nTapes - 1)
>
> Unless I'm missing something, the "if" condition just causes the last
> pass through this loop to do nothing.  If so, why not just change the
> loop condition to i < lts->nTapes - 1 and drop the "if" statement
> altogether?

The last "lt" in the loop is in fact used separately, just outside the
loop. But that use turns out to have been subtly wrong, apparently due
to a problem with converting logtape.c to use the shared buffile
stuff. This buglet would only have caused writing to the leader tape
to break (never trace_sort instrumentation), something that isn't
supported anyway due to the restrictions that shared BufFiles have.
But, we should, on general principle, be able to write to the leader
tape if and when shared buffiles learn to support writing (after
exporting original BufFile in worker).

Buglet fixed in my local working copy. I did so in a way that changes
loop test along the lines you suggest. This should make the whole
design of tape concatenation a bit clearer.

> +charfilename[MAXPGPATH] = {0};
>
> I don't think you need = {0}, because pg_itoa is about to clobber it anyway.

Okay.

> +/* Alter worker's tape state (generic values okay for leader) */
>
> What do you mean by generic values?

I mean that the leader's tape doesn't need to have
lt->firstBlockNumber set, because it's empty -- it can remain -1. Same
applies to lt->offsetBlockNumber, too.

I'll remove the text within parenthesis, since it seems redundant
given the structure of the loop.

> + * Each tape is initialized in write state.  Serial callers pass ntapes, but
> + * NULL arguments for everything else.  Parallel worker callers pass a
> + * shared handle and worker number, but tapeset should be NULL.  Leader
> + * passes worker -1, a shared handle, and shared tape metadata. These are
> + * used to claim ownership of worker tapes.
>
> This comment doesn't match the actual function definition terribly
> well.  Serial callers don't pass NULL for "everything else", because
> "int worker" is not going to be NULL.  For parallel workers, it's not
> entirely obvious whether "a shared handle" means TapeShare *tapes or
> SharedFileSet *fileset.  "tapeset" sounds like an argument name, but
> there is no such argument.

Okay. I've tweaked things here.

> lt->max_size looks like it might be an optimization separate from the
> overall patch, but maybe I'm wrong about that.

I think that it's pretty much essential. Currently, the MaxAllocSize
restriction is needed in logtape.c for the same reason that it's
needed anywhere else. Not much to talk about there. The new max_size
thing is about more than that, though -- it's really about not
stupidly allocating up to a full MaxAllocSize when you already know
that you're going to use next to no memory.

You don't have this issue with serial sorts because serial sorts that
only sort a tiny number of tuples never end up as external sorts --
when you end up doing a serial external sort, clearly you're never
going to allocate an excessive amount of memory up front in logtape.c,

Re: Add default role 'pg_access_server_files'

[Thomas Munro]

On Mon, Jan 1, 2018 at 8:19 AM, Stephen Frost  wrote:
> Greetings,
>
> This patch adds a new default role called 'pg_access_server_files' which
> allows an administrator to GRANT to a non-superuser role the ability to
> access server-side files through PostgreSQL (as the user the database is
> running as).  By itself, having this role allows a non-superuser to use
> server-side COPY and to use file_fdw (if installed by a superuser and
> GRANT'd USAGE on it).

Hi Stephen,

Not sure if you are aware of this failure?

test file_fdw ... FAILED

Because:

! ERROR: only superuser can change options of a file_fdw foreign table
...
! ERROR: only superuser or a member of the pg_access_server_files role
can change options of a file_fdw foreign table

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Stephen Frost]

Greetings Jing,

* Jing Wang (jingwang...@gmail.com) wrote:
> I have rebased the patch on the latest version.

Thanks!  Looks like there's still more work to be done here, and
unfortunately this ended up on a new thread somehow from the prior one.
I've added this newer thread to the CF app too.

> Because the CURRENT_DATABASE can not only being used on COMMENT ON
> statement but also on other statements as following list so the patch name
> is renamed to "support_CURRENT_DATABASE_keyword_vxx.patch".

Makes sense.

Unfortunately, this still is throwing a warning when building:

/src/backend/parser/gram.y: In function ‘base_yyparse’:
/src/backend/parser/gram.y:1160:19: warning: assignment from incompatible 
pointer type [-Wincompatible-pointer-types]
| IN_P DATABASE db_spec_name { $$ = $3; }
   ^
Also, the documentation changes aren't quite right, you're doing:

ALTER DATABASE {name | 
CURRENT_DATABASE} OWNER TO { new_owner 
| CURRENT_USER | SESSION_USER }

When it should be:

ALTER DATABASE { name | 
CURRENT_DATABASE } OWNER TO { new_owner | 
CURRENT_USER | SESSION_USER }

Note that the "replaceable class" tag goes directly around 'name', and
doesn't include CURRENT_DATABASE.  Also, keep a space before 'name' and
after 'CURRENT_DATABASE', just like how the "OWNER TO ..." piece works.

Please don't include whitespace-only hunks, like this one:

*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
*** const ObjectAddress InvalidObjectAddress
*** 724,730 
InvalidOid,
0
  };
- 
  static ObjectAddress get_object_address_unqualified(ObjectType objtype,
   Value *strval, bool missing_ok);
  static ObjectAddress get_relation_by_qualified_name(ObjectType objtype,

The TAP regression tests for pg_dump are failing.  It looks like you've
changed pg_dump to use CURRENT_DATABASE, which is fine, but please
adjust the regexp's used in the pg_dump TAP tests to handle that.  The
regexp you're looking to change is in src/bin/pg_dump/t/002_pg_dump.pl,
around line 1515 in current head (look for the stanza:

'COMMENT ON DATABASE postgres' => {

and the regexp:

regexp=> qr/^COMMENT ON DATABASE postgres IS .*;/m,

That looks to be the only thing that needs to be changed to make this
test pass.

In gram.y, I would have thought to make a db_spec_name a 'dbspec' type,
similar to what was done with 'rolespec' (though, I note, the efforts
around RoleSpec seem to have stalled since COMMENT ON ROLE CURRENT_ROLE
doesn't work and get_object_address seems to still think that the parser
works with roles as strings, when only half of it actually does..) and
then make makeDbSpec() return a DbSpec and then try to minimize the
forced-casting happening.  On a similar vein, I would think that the
various 'dbspec' pointers in AlterRoleSetStmt and others should be of
the DbSpec type instead of just being Node*'s.

Ideally, try to make sure that the changes you're making are pgindent'd
properly.

There's probably more to do on this, but hopefully this gets the patch
into a bit of a cleaner state for further review.  Setting to Waiting on
Author.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Bug in to_timestamp().

[Thomas Munro]

On Thu, Nov 23, 2017 at 4:23 AM, Arthur Zakirov
 wrote:
> I've attached new version of the patch. It is a little bit simpler now than 
> the previous one.
> The patch doesn't handle backslashes now, since there was a commit which 
> fixes quoted-substring handling recently.
> Anyway I'm not sure that this handling was necessary.
>
> I've checked queries from this thread. It seems that they give right 
> timestamps and work like in Oracle (except different messages).
>
> The patch contains documentation and regression test fixes.

I'm guessing that commit 11b623dd0a2c385719ebbbdd42dd4ec395dcdc9d had
something to do with the following failure, when your patch is
applied:

 horology ... FAILED

= Contents of ./src/test/regress/regression.diffs
*** 
/home/travis/build/postgresql-cfbot/postgresql/src/test/regress/expected/horology.out
2018-01-11 17:11:49.744128698 +
--- 
/home/travis/build/postgresql-cfbot/postgresql/src/test/regress/results/horology.out
2018-01-11 17:18:53.988815652 +
***
*** 2972,2978 
  SELECT to_timestamp('2011-12-18 11:38 -05','-MM-DD HH12:MI TZH');
   to_timestamp
  --
!  Sun Dec 18 08:38:00 2011 PST
  (1 row)

  SELECT to_timestamp('2011-12-18 11:38 +05:20', '-MM-DD HH12:MI TZH:TZM');
--- 2972,2978 
  SELECT to_timestamp('2011-12-18 11:38 -05','-MM-DD HH12:MI TZH');
   to_timestamp
  --
!  Sat Dec 17 22:38:00 2011 PST
  (1 row)

  SELECT to_timestamp('2011-12-18 11:38 +05:20', '-MM-DD HH12:MI TZH:TZM');
***
*** 2984,2990 
  SELECT to_timestamp('2011-12-18 11:38 -05:20', '-MM-DD HH12:MI TZH:TZM');
   to_timestamp
  --
!  Sun Dec 18 08:58:00 2011 PST
  (1 row)

  SELECT to_timestamp('2011-12-18 11:38 20', '-MM-DD HH12:MI TZM');
--- 2984,2990 
  SELECT to_timestamp('2011-12-18 11:38 -05:20', '-MM-DD HH12:MI TZH:TZM');
   to_timestamp
  --
!  Sat Dec 17 22:18:00 2011 PST
  (1 row)

  SELECT to_timestamp('2011-12-18 11:38 20', '-MM-DD HH12:MI TZM');
==

-- 
Thomas Munro
http://www.enterprisedb.com

Re: add queryEnv to ExplainOneQuery_hook

[Tatsuro Yamada]

On 2018/01/12 2:02, Tom Lane wrote:

Thomas Munro  writes:

On Fri, Jan 12, 2018 at 12:16 AM, Tatsuro Yamada
 wrote:

I found a variable (queryEnv) which should be added in
ExplainOneQuery_hook because if it is missing, hook function
can't call ExplainOnePlan.



Yeah, I think you're right.  That's an oversight in 18ce3a4a.


Clearly :-(.  Too bad we didn't find this sooner --- I suppose changing it
in v10 now is a nonstarter.  Will push to HEAD though.

regards, tom lane


Thanks Tom!
I read that commit log on HEAD.

Regards,
Tatsuro Yamada

Re: add queryEnv to ExplainOneQuery_hook

[Tatsuro Yamada]

On 2018/01/11 21:46, Michael Paquier wrote:

I'm surprised we haven't heard any complaints sooner if there are
advisors using that hook[1] and expecting to be able to forward to
ExplainOnePlan(), but I suppose it would nearly always works to call
ExplainOnePlan() with NULL as queryEnv.  It'd currently only be
non-NULL for trigger functions running SQL to access transition
tables, which is a bit obscure: you'd need to run EXPLAIN inside a
suitable trigger function (though in future there might be more ways
to create named tuplestore relations).


It seems to me that QueryEnv should be pushed to the hook, but only on
HEAD. You surely don't want to break people's extensions after a minor
upgrade.


Thanks guys! :)

I also surprised that there is no complaint from extension creators.
I suppose that if possible, it would be better to create a unit test
for the hook function to avoid the same bug because there is no contrib
module using ExplainOneQuery_hook in contrib directory.
(It might unnecessary thing, maybe.)

Regards,
Tatsuro Yamada

Re: [HACKERS] postgres_fdw bug in 9.6

[Thomas Munro]

On Mon, Dec 11, 2017 at 8:25 PM, Etsuro Fujita
 wrote:
>> Now, if you're still super-concerned about this breaking something, we
>> could commit it only to master, where it will have 9 months to bake
>> before it gets released.  I think that's overly conservative, but I
>> think it's still better than waiting for the rewrite you'd like to see
>> happen.  We don't know when or if anyone is going to undertake that,
>> and if we wait, we may easing release a v11 that's got the same defect
>> as v9.6 and now v10.  And I don't see that we lose much by committing
>> this now even if that rewrite does happen in time for v11.  Ripping
>> out CreateLocalJoinPath() won't be any harder than ripping out
>> GetExistingLocalJoinPath().
>
> Agreed.  Attached is an rebased version which moved the new fields in
> JoinPathExtraData to the end of that struct.

FYI this doesn't compile anymore, because initial_cost_hashjoin() and
create_hashjoin_path() changed in master.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] Re: proposal - psql: possibility to specify sort for describe commands, when size is printed

[Thomas Munro]

On Thu, Nov 2, 2017 at 12:44 AM, Pavel Stehule  wrote:
> I am sending updated patch with some basic doc

Hi Pavel,

I am not sure what the status of this patch is, but FYI:

startup.c: In function ‘main’:
startup.c:284:3: error: too few arguments to function ‘listAllDbs’
   success = listAllDbs(NULL, false);
   ^
In file included from startup.c:21:0:
describe.h:68:13: note: declared here
 extern bool listAllDbs(const char *pattern, bool verbose, sortby_type
sortby, bool sort_desc);
 ^

-- 
Thomas Munro
http://www.enterprisedb.com

Possible performance regression with pg_dump of a large number of relations

[Luke Cowell]

I've been troubleshooting an issue with slow pg_dump times on postgres 9.6.6. I 
believe something changed between 9.5.10 and 9.6.6 that has made dumps 
significantly slower for databases with a large number of relations. I posted 
this in irc and someone suggested that I should post this here. I'm sorry if 
this isn't the right place.

To simulate the issue I generated 150,000 relations spread across 1000 schemas 
(this roughly reflects my production setup).

```ruby
File.write "many_relations.sql", (15 / 150).times.flat_map {|n|
  [
   "create schema s_#{n};",
   150.times.map do |t|
 "create table s_#{n}.test_#{t} (id int);"
   end
   ]
}.join("\n")
```

I have 2 identical pieces of hardware. I've installed 9.5 on one and 9.6 on the 
other. I've run the same generated piece of sql in a fresh database on both 
systems.

On my 9.5.10 system:
> time pg_dump -n s_10 testing > /dev/null
real0m5.492s
user0m1.424s
sys 0m0.184s

On my 9.6.6 system:
> time pg_dump -n s_10 testing > /dev/null
real0m27.342s
user0m1.748s
sys 0m0.248s

If I call that same pg_dump command with the verbose option, the delay is at 
`pg_dump: reading user-defined tables` step.

I don't have identical hardware, so I can't say for sure, but I believe this 
issue is still present in 10.1.

Is this a legitimate issue? Is there more information I can provide to help 
better assess the situation?

Thanks in advance everyone!

Luke

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Michael Paquier]

On Thu, Jan 11, 2018 at 10:42:38AM -0500, Tom Lane wrote:
> Michael Paquier  writes:
>> guc.c already holds a find_option()
>> which can be used to retrieve the flags of a parameter. What about using
>> that and filtering by GUC_LIST_INPUT? This requires exposing the
>> function, and I am not sure what people think about that.
> 
> Don't like exposing find_option directly, but I think it would make
> sense to provide an API along the lines of
> int GetConfigOptionFlags(const char *name, bool missing_ok).

OK, I can live with that. What do you think about the attached? I'll be
happy to produce patches for back-branches as necessary. When an option
is not found, I have made the function return 0 as value for the flags,
which is consistent with flatten_set_variable_args(). To make things
behave more consistently with GUC_LIST_QUOTE GUCs, it seems to me that
those should not be quoted as well (ALTER SYSTEM shares the same
compatibility). And attached is a patch.

While reviewing more the code, I have noticed the same code pattern in
pg_dump.c and pg_dumpall.c. In the patch attached, I have corrected
things so as all parameters marked as GUC_LIST_QUOTE are correctly not
quoted because we have no generic solution to know from frontends what's
a GUC type (it would make sense to me to expose a text[] with this
information in pg_settings). However, let's be honest, it does not make
sense to update all those parameters because who is really going to use
them for functions! Two things that make sense to me are just
wal_consistency_checking and synchronous_standby_names for developers
which could use it to tune WAL generated within a set of SQL or plpgsql
functions. As it is easier to delete than add code here, I have added
all of them to ease the production of a committable version. For
correctness, still having them may make sense.
--
Michael
diff --git a/src/backend/utils/adt/ruleutils.c 
b/src/backend/utils/adt/ruleutils.c
index 9cdbb06add..7914f4dd88 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -61,6 +61,7 @@
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
+#include "utils/guc.h"
 #include "utils/hsearch.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
@@ -2561,6 +2562,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
{
char   *configitem = TextDatumGetCString(d);
char   *pos;
+   int flags;
 
pos = strchr(configitem, '=');
if (pos == NULL)
@@ -2571,11 +2573,11 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
 
quote_identifier(configitem));
 
/*
-* Some GUC variable names are 'LIST' type and 
hence must not
-* be quoted.
+* Some GUC variable names are of type 
GUC_LIST_INPUT
+* and hence must not be quoted.
 */
-   if (pg_strcasecmp(configitem, "DateStyle") == 0
-   || pg_strcasecmp(configitem, 
"search_path") == 0)
+   flags = GetConfigOptionFlags(configitem, false);
+   if ((flags & GUC_LIST_INPUT) != 0)
appendStringInfoString(, pos);
else
simple_quote_literal(, pos);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 72f6be329e..5a39c96242 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8099,6 +8099,30 @@ GetConfigOptionByName(const char *name, const char 
**varname, bool missing_ok)
return _ShowOption(record, true);
 }
 
+/*
+ * Return "flags" for a given GUC variable. If missing_ok is true, ignore
+ * any errors and return 0 to the caller instead.
+ */
+int
+GetConfigOptionFlags(const char *name, bool missing_ok)
+{
+   struct config_generic *record;
+
+   record = find_option(name, false, ERROR);
+
+   if (record == NULL)
+   {
+   if (missing_ok)
+   return 0;
+
+   ereport(ERROR,
+   (errcode(ERRCODE_UNDEFINED_OBJECT),
+errmsg("unrecognized configuration parameter 
\"%s\"", name)));
+   }
+
+   return record->flags;
+}
+
 /*
  * Return GUC variable value by variable number; optionally return canonical
  * form of name.  Return value is palloc'd.
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 27628a397c..f9b9de8894 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -11777,11 

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Thomas Munro]

On Mon, Dec 4, 2017 at 1:34 PM, Jing Wang  wrote:
> I have rebased the patch on the latest version.

Hi Jing,

According to my testing robot this fails make check-world (or
presumably cd src/bin/pg_dump ; make check), here:

t/001_basic.pl . ok
#   Failed test 'binary_upgrade: dumps COMMENT ON DATABASE postgres'
#   at t/002_pg_dump.pl line 6753.
... masses of dump output omitted ...
# Looks like you failed 19 tests of 4727.
t/002_pg_dump.pl ...
Dubious, test returned 19 (wstat 4864, 0x1300)
Failed 19/4727 subtests
t/010_dump_connstr.pl .. ok

Test Summary Report
---
t/002_pg_dump.pl (Wstat: 4864 Tests: 4727 Failed: 19)
  Failed tests:  63, 224, 412, 702, 992, 1161, 1330, 1503
1672, 1840, 2008, 2176, 2343, 2495, 2663
3150, 3901, 4282, 4610
  Non-zero exit status: 19

There is also a warning from my compiler here:

gram.y:1160:19: error: incompatible pointer types assigning to 'char
*' from 'Node *' (aka 'struct Node *')
[-Werror,-Wincompatible-pointer-types]
{ (yyval.str) = (yyvsp[0].node); }
  ^ ~~~

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] Cache lookup errors with functions manipulation object addresses

[Thomas Munro]

On Mon, Nov 27, 2017 at 1:01 AM, Michael Paquier
 wrote:
> On Fri, Nov 24, 2017 at 9:13 PM, Michael Paquier
>  wrote:
>> Attached is a rebased patch set. Álvaro, as you have introduced most
>> of the problems with 4464303 & friends dated of 2015 when you
>> introduced get_object_address(), could you look at this patch set?

Hi Michael,

FYI:

func.sgml:17550: parser error : Opening and ending tag mismatch:
literal line 17550 and unparseable
   NULL values for undefined objects.
   ^
func.sgml:17567: parser error : Opening and ending tag mismatch:
literal line 17567 and unparseable
   with NULL values.
^
func.sgml:17582: parser error : Opening and ending tag mismatch:
literal line 17582 and unparseable
   Undefined objects are identified with NULL values.
 ^
func.sgml:20721: parser error : chunk is not well balanced
postgres.sgml:105: parser error : Failure to process entity func
  
^
postgres.sgml:105: parser error : Entity 'func' not defined
  
^

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Jsonb transform for pl/python

[Thomas Munro]

On Thu, Dec 7, 2017 at 12:40 AM, Anthony Bykov  wrote:
> Hello,
> fixed the issues:
> 1. Rising errors when invalid object being transformed.
> 2. We don't rise the exception when transforming range(3) only in
> python 2. In third one it is an error.
>
> Please, find the 4-th version of the patch in attachments to this
> message. --

Hi Anthony,

FYI make docs fails:

json.sgml:584: parser error : Opening and ending tag mismatch: xref
line 581 and para
  
 ^
json.sgml:585: parser error : Opening and ending tag mismatch: para
line 575 and sect2
 
 ^
json.sgml:588: parser error : Opening and ending tag mismatch: sect2
line 572 and sect1

^
json.sgml:589: parser error : Premature end of data in tag sect1 line 3
json.sgml:589: parser error : chunk is not well balanced
datatype.sgml:4354: parser error : Failure to process entity json
  
^
datatype.sgml:4354: parser error : Entity 'json' not defined
  
^
datatype.sgml:4955: parser error : chunk is not well balanced
postgres.sgml:104: parser error : Failure to process entity datatype
  
^
postgres.sgml:104: parser error : Entity 'datatype' not defined
  
^

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] taking stdbool.h into use

[Tom Lane]

Peter Eisentraut  writes:
> That leaves the uses in rowtypes.c.  Those were introduced as a
> portability fix by commit 4cbb646334b.  I'm curious why these are
> necessary.  The Datums they operate come from heap_deform_tuple(), which
> gets them from fetchatt(), which does run all pass-by-value values
> through the very same GET_X_BYTES() macros (until now).  I don't see
> where those dirty upper bits would be coming from.

I don't see it either.  I think the actually useful parts of that patch
were to declare record_image_cmp's result correctly as int rather than
bool, and to cope with varlena fields of unequal size.  Unfortunately
there seems to be no contemporaneous mailing list discussion, so
it's not clear what Kevin based this change on.

Want to try reverting the GET_X_BYTES() parts of it and see if the
buildfarm complains?

Note if that if we simplify the GetDatum macros, it's possible that
record_image_cmp would change behavior, since it might now see signed not
unsigned values depending on whether the casts do sign extension or not.
Not sure if that'd be a problem.

regards, tom lane

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Greg Stark]

On 11 January 2018 at 19:41, Peter Eisentraut
 wrote:

> Two, what to do when the memory limit is reached.  With the old
> accounting, this was easy, because we'd decide for each subtransaction
> independently whether to spill it to disk, when it has reached its 4096
> limit.  Now, we are looking at a global limit, so we have to find a
> transaction to spill in some other way.  The proposed patch searches
> through the entire list of transactions to find the largest one.  But as
> the patch says:
>
> "XXX With many subtransactions this might be quite slow, because we'll
> have to walk through all of them. There are some options how we could
> improve that: (a) maintain some secondary structure with transactions
> sorted by amount of changes, (b) not looking for the entirely largest
> transaction, but e.g. for transaction using at least some fraction of
> the memory limit, and (c) evicting multiple transactions at once, e.g.
> to free a given portion of the memory limit (e.g. 50%)."

AIUI spilling to disk doesn't affect absorbing future updates, we
would just keep accumulating them in memory right? We won't need to
unspill until it comes time to commit.

Is there any actual advantage to picking the largest transaction? it
means fewer spills and fewer unspills at commit time but that just a
bigger spike of i/o and more of a chance of spilling more than
necessary to get by. In the end it'll be more or less the same amount
of data read back, just all in one big spike when spilling and one big
spike when committing. If you spilled smaller transactions you would
have a small amount of i/o more frequently and have to read back small
amounts for many commits. But it would add up to the same amount of
i/o (or less if you avoid spilling more than necessary).

The real aim should be to try to pick the transaction that will be
committed furthest in the future. That gives you the most memory to
use for live transactions for the longest time and could let you
process the maximum amount of transactions without spilling them. So
either the oldest transaction (in the expectation that it's been open
a while and appears to be a long-lived batch job that will stay open
for a long time) or the youngest transaction (in the expectation that
all transactions are more or less equally long-lived) might make
sense.



-- 
greg

Re: IndexTupleDSize macro seems redundant

[Tom Lane]

Stephen Frost  writes:
> Updated (combined) patch attached for review.  I went through and looked
> again to make sure there weren't any cases of making an unaligned
> pointer to a struct and didn't see any, and I added some comments to
> _bt_restore_page().

Looks OK from here.

regards, tom lane

Re: [HACKERS] Planning counters in pg_stat_statements

[Haribabu Kommi]

On Thu, Jan 11, 2018 at 10:00 PM, Thomas Munro <
thomas.mu...@enterprisedb.com> wrote:

> On Thu, Jan 11, 2018 at 7:36 PM, Haribabu Kommi
>  wrote:
>
> > +OUT plans int8,
> >
> > Addition of this column is good to find out how many time the plan is
> > generated
> > for the same query. But I am thinking "plans" column name may confuse?
>
> What else would you call that?  It's the number of plans that have
> been created.  It sits nicely beside "calls" don't you think?  I could
> change it to "plan_count" but that's longer and not like "calls".
>

OK.

I checked the latest patch and it is working fine and I don't have any
further comments. Marked the patch as "ready for committer".

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Vaishnavi Prabakaran]

On Fri, Jan 5, 2018 at 4:55 PM, Vaishnavi Prabakaran <
vaishnaviprabaka...@gmail.com> wrote:

>
>
> On Tue, Nov 28, 2017 at 12:57 PM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
>
>> On Thu, Oct 5, 2017 at 9:58 AM, Vaishnavi Prabakaran
>>  wrote:
>> > Thanks for the suggestion and, OK I will create a new patch in upcoming
>> > commitfest with attached patch addressing above review comments.
>>
>> The patch still applies and there has been no updates for the last
>> month, as well as no reviews. I am bumping it to next CF.
>
>
> Thank you, I see the patch generates a compilation error due to usage of
> "FALSE" with latest postgres code, Hence attaching the patch with
> correction.
>

Corrected compilation error in documentation portion of patch with latest
postgres code. Attached the corrected patch.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.


0001-Pipelining-batch-support-for-libpq-code-v16.patch
Description: Binary data

Re: Parameters in user-defined aggregate final functions

[Tom Lane]

Esteban Zimanyi  writes:
> How to tell PostgreSQL that my final function also needs a parameter? I am
> working on PostgreSQL 10.1. I know that according to the documentation
> direct parameters are only allowed for ordered-set aggregates, but I would
> also need a direct parameter for "normal" aggregates.

So define it as an ordered-set aggregate, and just ignore the question
of whether you need to sort the input (which is something that we leave
to the aggregate function to do anyway).  The syntax would be a little
weird/non-orthogonal, but you can blame the SQL committee for that.

regression=# create function trans(int, int) returns int language sql
regression-# as 'select $1+$2' strict;
CREATE FUNCTION
regression=# create function final(int, float8) returns float8 language sql
regression-# as 'select $1*$2' strict;
CREATE FUNCTION
regression=# create aggregate myosa(float8 order by int) (
regression(# sfunc = trans, stype = int, finalfunc = final);
CREATE AGGREGATE
regression=# select sum(ten), myosa(0.5) within group (order by ten) from tenk1;
  sum  | myosa 
---+---
 45000 | 22500
(1 row)


regards, tom lane

Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?

[Thomas Munro]

On Fri, Oct 6, 2017 at 2:45 AM, Robert Haas  wrote:
> On Tue, Oct 3, 2017 at 9:38 PM, Andres Freund  wrote:
>>> Do you have any suggestion as to how we should transmit the blacklist to
>>> parallel workers?
>>
>> How about storing them in the a dshash table instead of dynahash?
>> Similar to how we're now dealing with the shared typmod registry stuff?
>> It should be fairly simple to now simply add a new struct Session member
>> shared_enum_whatevs_table.
>
> Yeah, that approach seems worth exploring.

Andrew Dunstan asked me off-list which README covers that stuff.  Erm,
there isn't one, so I was going to write some explanation here to see
if that could help... but after looking at this I'm not sure I agree
it's the right approach anyway.

The reason commit cc5f8136 introduced Session and
SharedRecordTypmodRegistry was that we have some state that is
session-scoped and writable by any worker.  In contrast:

1.  The enum OID blacklist has transaction scope.  If you wanted to
put it into the Session you'd have to explicitly zap it at end of
transaction.  Incidentally dshash has no fast reset, though that could
be added, but I'd probably want fast per-transaction cleanup to skip
retail destruction entirely and simply give back all the memory, just
like MemoryContext does.  There are other transaction-scoped things
we'll eventually want to share, like ComboCIDs, so I think we'll need
something like that, but no one has been brave enough to propose the
machinery yet.

2.  The enum OID blacklist is read-only for workers.  They don't
create new blacklisted enums and don't see that they ever will.

So I agree with Tom's suggestion:

On Wed, Oct 4, 2017 at 2:29 PM, Tom Lane  wrote:
> Perhaps serialize the contents into an array in DSM, then rebuild a hash
> table from that in the worker.  Robert might have a better idea though.

I'd happily volunteer to write or review a patch to do that.  Is there
a rebase of the stuff that got reverted, to build on?

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Parameters in user-defined aggregate final functions

[David Fetter]

On Thu, Jan 11, 2018 at 08:51:27PM +0100, Esteban Zimanyi wrote:
> I am creating a user-defined aggregate function that needs an additional
> parameter. More precisely it is a cumulative (aka window) minimum that
> takes as second parameter a time interval defining the window. Since the
> aggregate function operates on my user-defined data types I have conveyed a
> dummy example that concatenates the n last values of a text column. I am
> aware that I can solve this dummy problem in PostgreSQL but the purpose of
> the example is only to highlight my problem.
> 
> CREATE FUNCTION lastNconcat_transfn(state text[], next text, n integer)
> RETURNS text[] AS $$
> BEGIN
> RETURN array_append(state, next);
> END;
> $$ LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;
> 
> CREATE FUNCTION lastNconcat_combinefn(state1 text[], state2 text[], n
> integer)
> RETURNS text[] AS $$
> BEGIN
> RETURN array_concat(state1, state2);
> END;
> $$ LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;
> 
> CREATE FUNCTION lastNconcat_finalfn(state text[], n integer)
> RETURNS text AS $$
> DECLARE
> card integer;
> result text;
> BEGIN
> result := '';
> card := array_length(state, 1);
> FOR i IN greatest(1,card-n+1)..card
> LOOP
> result := result || state[i];
> END LOOP;
> RETURN result;
> END;
> $$ LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;
> 
> CREATE AGGREGATE lastNconcat(text, integer) (
> SFUNC = lastNconcat_transfn,
> STYPE = text[],
> INITCOND = '{}',
> COMBINEFUNC = lastNconcat_combinefn,
> FINALFUNC = lastNconcat_finalfn,
> PARALLEL = SAFE
> );
> 
> I receive the following error
> 
> ERROR: function lastnconcat_finalfn(text[]) does not exist
> SQL state: 42883
> 
> How to tell PostgreSQL that my final function also needs a parameter? I am
> working on PostgreSQL 10.1. I know that according to the documentation
> direct parameters are only allowed for ordered-set aggregates, but I would
> also need a direct parameter for "normal" aggregates.
> 
> Notice that the solution proposed here
> https://stackoverflow.com/questions/48189751/direct-arguments-in-postgresql-user-defined-aggregate-functions/48190288?noredirect=1#comment83364017_48190288
> is neither ideal nor efficient.
> 
> IMHO since combine functions accept parameters I don't see why final
> functions should not also accept parameters.

This is an interesting problem.  In CREATE AGGREGATE, I count 10
parameters that could easily have a function attached. One could
imagine an aggregate which took different parameters at each stage,
but is there really any sane way to do this other than making a call
to the aggregate with those parameters all included, passing each
along as one goes?

SELECT my_custom_agg(expression) WITH ([finalfunc_args = ...][, 
finalfunc_extra_args = ...]...)

is what I could come up with.  It seems ugly as grammar and
ill-advised in that it makes promises about the implementation details
of aggregates into a distant future.

What am I missing?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Robert Haas]

On Wed, Jan 10, 2018 at 1:45 PM, Robert Haas  wrote:
> There's a lot here I haven't grokked yet, but I'm running out of
> mental energy so I think I'll send this for now and work on this some
> more when time permits, hopefully tomorrow.

Looking at the logtape changes:

While the patch contains, as I said before, an excellent set of how-to
directions explaining how to use the new parallel sort facilities in
tuplesort.c, there seems to be no such thing for logtape.c, and as a
result I find it a bit unclear how the interface is supposed to work.
I think it would be good to add a similar summary here.

It seems like the words "leader" and "worker" here refer to the leader
of a parallel operation and the associated workers, but do we really
need to make that assumption?  Couldn't we generally describe this as
merging a bunch of 1-tape LogicalTapeSets created from a SharedFileSet
into a single LogicalTapeSet that can thereafter be read by the
process that does the merging?

+/* Pass worker BufFile pieces, and a placeholder leader piece */
+for (i = 0; i < lts->nTapes; i++)
+{
+lt = >tapes[i];
+
+/*
+ * Build concatenated view of all BufFiles, remembering the block
+ * number where each source file begins.
+ */
+if (i < lts->nTapes - 1)

Unless I'm missing something, the "if" condition just causes the last
pass through this loop to do nothing.  If so, why not just change the
loop condition to i < lts->nTapes - 1 and drop the "if" statement
altogether?

+charfilename[MAXPGPATH] = {0};

I don't think you need = {0}, because pg_itoa is about to clobber it anyway.

+/* Alter worker's tape state (generic values okay for leader) */

What do you mean by generic values?

+ * Each tape is initialized in write state.  Serial callers pass ntapes, but
+ * NULL arguments for everything else.  Parallel worker callers pass a
+ * shared handle and worker number, but tapeset should be NULL.  Leader
+ * passes worker -1, a shared handle, and shared tape metadata. These are
+ * used to claim ownership of worker tapes.

This comment doesn't match the actual function definition terribly
well.  Serial callers don't pass NULL for "everything else", because
"int worker" is not going to be NULL.  For parallel workers, it's not
entirely obvious whether "a shared handle" means TapeShare *tapes or
SharedFileSet *fileset.  "tapeset" sounds like an argument name, but
there is no such argument.

lt->max_size looks like it might be an optimization separate from the
overall patch, but maybe I'm wrong about that.

+/* palloc() larger than MaxAllocSize would fail */
 lt->buffer = NULL;
 lt->buffer_size = 0;
+lt->max_size = MaxAllocSize;

The comment about palloc() should move down to where you assign max_size.

Generally we avoid returning a struct type, so maybe
LogicalTapeFreeze() should instead grow an out parameter of type
TapeShare * which it populates only if not NULL.

Won't LogicalTapeFreeze() fail an assertion in BufFileExportShared()
if the file doesn't belong to a shared fileset?  If you adopt the
previous suggestion, we can probably just make whether to call this
contingent on whether the TapeShare * out parameter is provided.

I'm not confident I completely understand what's going on with the
logtape stuff yet, so I might have more comments (or better ones)
after I study this further.  To your question about whether to go
ahead and post a new version, I'm OK to keep reviewing this version
for a little longer or to switch to a new one, as you prefer.  I have
not made any local changes, just written a blizzard of email text.
:-p

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Peter Geoghegan]

On Thu, Jan 11, 2018 at 1:44 PM, Robert Haas  wrote:
>> A third option here is to specifically recognize that
>> compute_parallel_worker() returned a value based on the table storage
>> param max_workers, and for that reason alone no "insufficient memory
>> per participant" decrementing/vetoing should take place. That is, when
>> the max_workers param is set, perhaps it should be completely
>> impossible for CREATE INDEX to ignore it for any reason other than an
>> inability to launch parallel workers (though that could be due to the
>> max_parallel_workers GUC's setting).
>>
>> You could argue that we should do this anyway, I suppose.
>
> Yes, I think this sounds like a good idea.

Cool. I've already implemented this in my local working copy of the
patch. That settles that.

If I'm not mistaken, the only outstanding question at this point is
whether or not we're going to give in and completely remove parallel
leader participation entirely. I suspect that we won't end up doing
that, because while it's not very useful, it's also not hard to
support. Besides, to some extent that's the expectation that has been
established already.

I am not far from posting a revision that incorporates all of your
feedback. Expect that tomorrow afternoon your time at the latest. Of
course, you may have more feedback for me in the meantime. Let me know
if I should hold off on posting a new version.

-- 
Peter Geoghegan

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

On 1/9/18 00:17, Michael Paquier wrote:
>>  * When a type narrower than Datum is stored in a Datum, we place it in the
>>  * low-order bits and are careful that the DatumGetXXX macro for it discards
>>  * the unused high-order bits (as opposed to, say, assuming they are zero).
>>  * This is needed to support old-style user-defined functions, since 
>> depending
>>  * on architecture and compiler, the return value of a function returning 
>> char
>>  * or short may contain garbage when called as if it returned Datum.
>>
>> Since we flushed support for V0 functions, the stated rationale doesn't
>> apply anymore.  I wonder whether there is anything to be gained by
>> changing DatumGetXXX and XXXGetDatum to be simple casts (as, if memory
>> serves, they once were until we noticed the stated hazard).  Or, if
>> there's still a reason to keep the masking steps in place, we'd better
>> update this comment.

Yeah, we should remove those bit-masking calls if they are no longer
necessary.

Looking around where else they are used, the uses in numeric.c sure seem
like noops:

#if SIZEOF_DATUM == 8
#define NumericAbbrevGetDatum(X) ((Datum) SET_8_BYTES(X))
#define DatumGetNumericAbbrev(X) ((int64) GET_8_BYTES(X))
#define NUMERIC_ABBREV_NAN  NumericAbbrevGetDatum(PG_INT64_MIN)
#else
#define NumericAbbrevGetDatum(X) ((Datum) SET_4_BYTES(X))
#define DatumGetNumericAbbrev(X) ((int32) GET_4_BYTES(X))
#define NUMERIC_ABBREV_NAN  NumericAbbrevGetDatum(PG_INT32_MIN)
#endif

We can just replace these by straight casts, too.

That leaves the uses in rowtypes.c.  Those were introduced as a
portability fix by commit 4cbb646334b.  I'm curious why these are
necessary.  The Datums they operate come from heap_deform_tuple(), which
gets them from fetchatt(), which does run all pass-by-value values
through the very same GET_X_BYTES() macros (until now).  I don't see
where those dirty upper bits would be coming from.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Robert Haas]

On Thu, Jan 11, 2018 at 3:25 PM, Peter Geoghegan  wrote:
> On Thu, Jan 11, 2018 at 12:06 PM, Peter Geoghegan  wrote:
>> It might make sense to have the "minimum memory per participant" value
>> come from a GUC, rather than be hard coded (it's currently hard-coded
>> to 32MB).
>
>> What do you think of that idea?
>
> A third option here is to specifically recognize that
> compute_parallel_worker() returned a value based on the table storage
> param max_workers, and for that reason alone no "insufficient memory
> per participant" decrementing/vetoing should take place. That is, when
> the max_workers param is set, perhaps it should be completely
> impossible for CREATE INDEX to ignore it for any reason other than an
> inability to launch parallel workers (though that could be due to the
> max_parallel_workers GUC's setting).
>
> You could argue that we should do this anyway, I suppose.

Yes, I think this sounds like a good idea.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: IndexTupleDSize macro seems redundant

[Stephen Frost]

Greetings Tom, Robert, Ildar, all,

* Stephen Frost (sfr...@snowman.net) wrote:
> That said, since it's not aligned, regardless of the what craziness the
> compiler might try to pull, we probably shouldn't go casting it
> to something that later hackers might think will be aligned, but we
> should add a comment to clarify that it's not aligned and that we can't
> act like it is.

Updated (combined) patch attached for review.  I went through and looked
again to make sure there weren't any cases of making an unaligned
pointer to a struct and didn't see any, and I added some comments to
_bt_restore_page().

Thanks!

Stephen
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
new file mode 100644
index b38208e..ab5aaff
*** a/src/backend/access/hash/hash_xlog.c
--- b/src/backend/access/hash/hash_xlog.c
*** hash_xlog_move_page_contents(XLogReaderS
*** 558,564 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleDSize(*itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
--- 558,564 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleSize(itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
*** hash_xlog_squeeze_page(XLogReaderState *
*** 686,692 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleDSize(*itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
--- 686,692 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleSize(itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
new file mode 100644
index f668dcf..f121286
*** a/src/backend/access/hash/hashinsert.c
--- b/src/backend/access/hash/hashinsert.c
*** _hash_doinsert(Relation rel, IndexTuple
*** 55,61 
  	hashkey = _hash_get_indextuple_hashkey(itup);
  
  	/* compute item size too */
! 	itemsz = IndexTupleDSize(*itup);
  	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
   * need to be consistent */
  
--- 55,61 
  	hashkey = _hash_get_indextuple_hashkey(itup);
  
  	/* compute item size too */
! 	itemsz = IndexTupleSize(itup);
  	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
   * need to be consistent */
  
*** restart_insert:
*** 222,228 
  		XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
  
  		XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
! 		XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
  
  		recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
  
--- 222,228 
  		XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
  
  		XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
! 		XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup));
  
  		recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
  
*** _hash_pgaddmultitup(Relation rel, Buffer
*** 309,315 
  	{
  		Size		itemsize;
  
! 		itemsize = IndexTupleDSize(*itups[i]);
  		itemsize = MAXALIGN(itemsize);
  
  		/* Find where to insert the tuple (preserving page's hashkey ordering) */
--- 309,315 
  	{
  		Size		itemsize;
  
! 		itemsize = IndexTupleSize(itups[i]);
  		itemsize = MAXALIGN(itemsize);
  
  		/* Find where to insert the tuple (preserving page's hashkey ordering) */
diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/hashovfl.c
new file mode 100644
index c9de128..d92ecd7
*** a/src/backend/access/hash/hashovfl.c
--- b/src/backend/access/hash/hashovfl.c
*** readpage:
*** 890,896 
  
  			itup = (IndexTuple) PageGetItem(rpage,
  			PageGetItemId(rpage, roffnum));
! 			itemsz = IndexTupleDSize(*itup);
  			itemsz = MAXALIGN(itemsz);
  
  			/*
--- 890,896 
  
  			itup = (IndexTuple) PageGetItem(rpage,
  			PageGetItemId(rpage, roffnum));
! 			itemsz = IndexTupleSize(itup);
  			itemsz = MAXALIGN(itemsz);
  
  			/*
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
new file mode 100644
index e3c8721..3859e3b
*** a/src/backend/access/hash/hashpage.c
--- b/src/backend/access/hash/hashpage.c
*** _hash_splitbucket(Relation rel,
*** 1173,1179 
   * the current page in the new bucket, we must allocate a new
   * overflow page and place the tuple on that page instead.
   */
! itemsz = IndexTupleDSize(*new_itup);
  itemsz = MAXALIGN(itemsz);
  
  if (PageGetFreeSpaceForMultipleTuples(npage, nitups + 1) < (all_tups_size + itemsz))
--- 1173,1179 
   * the current page in the new bucket, we must allocate a new
   * overflow page and place the tuple on that page instead.
   */
! itemsz = IndexTupleSize(new_itup);
  itemsz = MAXALIGN(itemsz);
  
  if (PageGetFreeSpaceForMultipleTuples(npage, nitups + 1) < (all_tups_size + itemsz))
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
new file mode 100644
index 

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Peter Geoghegan]

On Thu, Jan 11, 2018 at 12:06 PM, Peter Geoghegan  wrote:
> It might make sense to have the "minimum memory per participant" value
> come from a GUC, rather than be hard coded (it's currently hard-coded
> to 32MB).

> What do you think of that idea?

A third option here is to specifically recognize that
compute_parallel_worker() returned a value based on the table storage
param max_workers, and for that reason alone no "insufficient memory
per participant" decrementing/vetoing should take place. That is, when
the max_workers param is set, perhaps it should be completely
impossible for CREATE INDEX to ignore it for any reason other than an
inability to launch parallel workers (though that could be due to the
max_parallel_workers GUC's setting).

You could argue that we should do this anyway, I suppose.

-- 
Peter Geoghegan

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Peter Geoghegan]

On Thu, Jan 11, 2018 at 11:51 AM, Robert Haas  wrote:
> I think the force_parallel_mode thing is too ugly to live.  I'm not
> sure that forcing low memory in workers is a thing we need to have,
> but if we do, then we'll have to invent some other way to have it.

It might make sense to have the "minimum memory per participant" value
come from a GUC, rather than be hard coded (it's currently hard-coded
to 32MB). I don't think that it's that compelling as a user-visible
option, but it might make sense as a testing option, that we might
very well decide to kill before v11 is released (we might kill it when
we come up with an acceptable interface for "just use this many
workers" in a later commit, which I think we'll definitely end up
doing anyway). By setting the minimum participant memory to 0, you can
then rely on the parallel_workers table storage param forcing the
number of worker processes that we'll request. You can accomplish the
same thing with "min_parallel_table_scan_size = 0", of course.

What do you think of that idea?

To be clear, I'm not actually arguing that we need any of this. My
point about being able to test low memory conditions from the first
commit is that insisting on it is reasonable. I don't actually feel
strongly either way, though, and am not doing any insisting myself.

-- 
Peter Geoghegan

Re: CUBE seems a bit confused about ORDER BY

[Teodor Sigaev]

This was discussed upthread and the solution found was "objects need to
be rebuilt, indexes need to be reindexed".  The point of Alexander's
query was to find affected objects that need such treatment.  Teodor
explicitly disregarded any change in pg_upgrade because the database
you're upgrading *from* is supposed to have gotten indexes reindexed,
etc.


I don't think that is really going to be acceptable.  People do not like
minor version updates that break their databases.  If we start doing
that we're going to find people refusing to apply minor updates, which
is not a place we want to be.
That's true, but we have choice of bad solutions. Current index could 
not support operator before patch. So we can:

1) Change operator to support existing index. That is what Alexander
   did. And yes, it changes returning order for both sequential and
   index scans, but makes them synced. Actually, user should not
   reindex existing indexes but should be ready for order changing
2) Change index structure which isn't obvious how. At least, it's
   possible to  add new operator class (so, upgrade script is needed)
   Mandatory reindex and order changes for index scans
3) Remove index support for this operator at all. And introduce new
   operator in HEAD with index support. This will need an upgrade script
   in minor versions

--
Teodor Sigaev  E-mail: teo...@sigaev.ru
  WWW: http://www.sigaev.ru/

Parameters in user-defined aggregate final functions

[Esteban Zimanyi]

I am creating a user-defined aggregate function that needs an additional
parameter. More precisely it is a cumulative (aka window) minimum that
takes as second parameter a time interval defining the window. Since the
aggregate function operates on my user-defined data types I have conveyed a
dummy example that concatenates the n last values of a text column. I am
aware that I can solve this dummy problem in PostgreSQL but the purpose of
the example is only to highlight my problem.

CREATE FUNCTION lastNconcat_transfn(state text[], next text, n integer)
RETURNS text[] AS $$
BEGIN
RETURN array_append(state, next);
END;
$$ LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;

CREATE FUNCTION lastNconcat_combinefn(state1 text[], state2 text[], n
integer)
RETURNS text[] AS $$
BEGIN
RETURN array_concat(state1, state2);
END;
$$ LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;

CREATE FUNCTION lastNconcat_finalfn(state text[], n integer)
RETURNS text AS $$
DECLARE
card integer;
result text;
BEGIN
result := '';
card := array_length(state, 1);
FOR i IN greatest(1,card-n+1)..card
LOOP
result := result || state[i];
END LOOP;
RETURN result;
END;
$$ LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;

CREATE AGGREGATE lastNconcat(text, integer) (
SFUNC = lastNconcat_transfn,
STYPE = text[],
INITCOND = '{}',
COMBINEFUNC = lastNconcat_combinefn,
FINALFUNC = lastNconcat_finalfn,
PARALLEL = SAFE
);

I receive the following error

ERROR: function lastnconcat_finalfn(text[]) does not exist
SQL state: 42883

How to tell PostgreSQL that my final function also needs a parameter? I am
working on PostgreSQL 10.1. I know that according to the documentation
direct parameters are only allowed for ordered-set aggregates, but I would
also need a direct parameter for "normal" aggregates.

Notice that the solution proposed here
https://stackoverflow.com/questions/48189751/direct-arguments-in-postgresql-user-defined-aggregate-functions/48190288?noredirect=1#comment83364017_48190288
is neither ideal nor efficient.

IMHO since combine functions accept parameters I don't see why final
functions should not also accept parameters.

-- 

Prof. Esteban Zimanyi
Department of Computer & Decision Engineering  (CoDE) CP 165/15
Universite Libre de Bruxelles
Avenue F. D. Roosevelt 50
B-1050 Brussels, Belgium
fax: + 32.2.650.47.13
tel: + 32.2.650.31.85
e-mail: ezima...@ulb.ac.be
Internet: http://code.ulb.ac.be/

Re: CUBE seems a bit confused about ORDER BY

[Alexander Korotkov]

On Thu, Jan 11, 2018 at 10:29 PM, Alvaro Herrera 
wrote:

> Tom Lane wrote:
> > What we've done in the past for comparable situations is to make the
> > change in a new major version and teach pg_upgrade to detect and report
> > the need for changes --- in this case, it might do something like create
> > a script of REINDEX commands for the affected indexes.  See e.g.
> > old_9_6_invalidate_hash_indexes().
>
> I think the whole point is that any knn-gist indexes using this operator
> are completely broken (i.e. they return the wrong answer), so leaving
> them as-is in existing branches is not cool.
>
> The idea of an extension update being the trigger for a fix sounds
> reasonable.  Maybe we can have the current function under ~> that throws
> a WARNING each time it is invoked, inviting people to upgrade the
> extension; and the new extension would contain a new ~> with the right
> semantics.  Then all the magic to rebuild objects has to occur in the
> upgrade .sql script.


Yes, the point is that ~> operator was especially designed to get knn-gist
support.
However, its design was broken.  And operator with that behavior can't be
accelerated using knn-gist.  We could leave this operator "as is", but drop
its
knn-gist support.  But that would be discouraging since then ~> operator
would
be left almost useless.  Assuming that ~> operator seems to not being
heavily
used (bug report was received after more than year after release), I
proposed
to change operator behavior so that it can be accelerated by knn-gist.

I like Alvaro's proposal about extension upgrade.  Assuming that there are
not
many users of ~>, and even smaller amount of them has built depending
objects
over ~> operator, I'd like to propose to not invent magic in upgrade .sql
script.
In .sql script can just do DROP OPERATOR, and CREATE OPERATOR with
new function.  If depending objects exist then this script will trigger an
error.
Given this error, user can manually drop depending objects before entension
upgrade.  Anyway, assuming that behavior of ~> operator was changed,
depending
objects need to be adjusted not just rebuilt.  So, magic would unlikely
work in
this case.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Robert Haas]

On Wed, Jan 10, 2018 at 5:42 PM, Peter Geoghegan  wrote:
> On Wed, Jan 10, 2018 at 2:21 PM, Robert Haas  wrote:
>>> I share your general feelings on all of this, but I really don't know
>>> what to do about it. Which of these alternatives is the least worst,
>>> all things considered?
>>
>> Let's get the patch committed without any explicit way of forcing the
>> number of workers and then think about adding that later.
>
> It could be argued that you need some way of forcing low memory in
> workers with any committed version. So while this sounds reasonable,
> it might not be compatible with throwing out what I've done with
> force_parallel_mode up-front, before you commit anything. What do you
> think?

I think the force_parallel_mode thing is too ugly to live.  I'm not
sure that forcing low memory in workers is a thing we need to have,
but if we do, then we'll have to invent some other way to have it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] [PATCH] Generic type subscripting

[Robert Haas]

On Thu, Jan 11, 2018 at 2:20 PM, Tom Lane  wrote:
> But I think we've probably beaten this topic to death ...

Yep.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] UPDATE of partition key

[Robert Haas]

On Thu, Jan 11, 2018 at 6:07 AM, Amit Khandekar  wrote:
> In the first paragraph of my explanation, I was explaining why two
> Transition capture states does not look like a good idea to me :

Oh, sorry.  I didn't read what you wrote carefully enough, I guess.

I see your points.  I think that there is probably a general need for
some refactoring here.  AfterTriggerSaveEvent() got significantly more
complicated and harder to understand with the arrival of transition
tables, and this patch is adding more complexity still.  It's also
adding complexity in other places to make ExecInsert() and
ExecDelete() usable for the semi-internal DELETE/INSERT operations
being produced when we split a partition key update into a DELETE and
INSERT pair.  It would be awfully nice to have some better way to
separate out each of the different things we might or might not want
to do depending on the situation: capture old tuple, capture new
tuple, fire before triggers, fire after triggers, count processed
rows, set command tag, perform actual heap operation, update indexes,
etc.  However, I don't have a specific idea how to do it better, so
maybe we should just get this committed for now and perhaps, with more
eyes on the code, someone will have a good idea.

> Slight correction; it was suggested by Amit Langote; not by David.

Oh, OK, sorry.

> So there are two independent optimizations we are talking about :
>
> 1. Create the map only when needed.
> 2. In case of UPDATE, for partitions that take part in update scans,
> there should be a single map; there should not be two separate maps,
> one for accessing per-subplan and the other for accessing per-leaf.

These optimizations aren't completely independent.   Optimization #2
can be implemented in several different ways.  The way you've chosen
to do it is to index the same array in two different ways depending on
whether per-leaf indexing is not needed, which I think is
unacceptable.  Another approach, which I proposed upthread, is to
always built the per-leaf mapping, but you pointed out that this could
involve doing a lot of unnecessary work in the case where most leaves
were pruned.  However, if you also implement #1, then that problem
goes away.  In other words, depending on the design you choose for #2,
you may or may not need to also implement optimization #1 to get good
performance.

To put that another way, I think Amit's idea of keeping a
subplan-offsets array is a pretty good one.  From your comments, you
do too.  But if we want to keep that, then we need a way to avoid the
expense of populating it for leaves that got pruned, except when we
are doing update row movement.  Otherwise, I don't see much choice but
to jettison the subplan-offsets array and just maintain two separate
arrays of mappings.

> Regarding the array names ...
>
> Noting all this, I feel we can go with names according to the
> structure of maps. Something like : mt_perleaf_tupconv_maps, and
> mt_persubplan_tupconv_maps. Other suggestions welcome.

I'd probably do mt_per_leaf_tupconv_maps, since inserting an
underscore between some but not all words seems strange.  But OK
otherwise.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Peter Eisentraut]

On 12/22/17 23:57, Tomas Vondra wrote:
> PART 1: adding logical_work_mem memory limit (0001)
> ---
> 
> Currently, limiting the amount of memory consumed by logical decoding is
> tricky (or you might say impossible) for several reasons:

I would like to see some more discussion on this, but I think not a lot
of people understand the details, so I'll try to write up an explanation
here.  This code is also somewhat new to me, so please correct me if
there are inaccuracies, while keeping in mind that I'm trying to simplify.

The data in the WAL is written as it happens, so the changes belonging
to different transactions are all mixed together.  One of the jobs of
logical decoding is to reassemble the changes belonging to each
transaction.  The top-level data structure for that is the infamous
ReorderBuffer.  So as it reads the WAL and sees something about a
transaction, it keeps a copy of that change in memory, indexed by
transaction ID (ReorderBufferChange).  When the transaction commits, the
accumulated changes are passed to the output plugin and then freed.  If
the transaction aborts, then changes are just thrown away.

So when logical decoding is active, a copy of the changes for each
active transaction is kept in memory (once per walsender).

More precisely, the above happens for each subtransaction.  When the
top-level transaction commits, it finds all its subtransactions in the
ReorderBuffer, reassembles everything in the right order, then invokes
the output plugin.

All this could end up using an unbounded amount of memory, so there is a
mechanism to spill changes to disk.  The way this currently works is
hardcoded, and this patch proposes to change that.

Currently, when a transaction or subtransaction has accumulated 4096
changes, it is spilled to disk.  When the top-level transaction commits,
things are read back from disk to do the final processing mentioned above.

This all works mostly fine, but you can construct some more extreme
cases where this can blow up.

Here is a mundane example.  Let's say a change entry takes 100 bytes (it
might contain a new row, or an update key and some new column values,
for example).  If you have 100 concurrent active sessions and no
subtransactions, then logical decoding memory is bounded by 4096 * 100 *
100 = 40 MB (per walsender) before things spill to disk.

Now let's say you are using a lot of subtransactions, because you are
using PL functions, exception handling, triggers, doing batch updates.
If you have 200 subtransactions on average per concurrent session, the
memory usage bound in that case would be 4096 * 100 * 100 * 200 = 8 GB
(per walsender).  And so on.  If you have more concurrent sessions or
larger changes or more subtransactions, you'll use much more than those
8 GB.  And if you don't have those 8 GB, then you're stuck at this point.

That is the consideration when we record changes, but we also need
memory when we do the final processing at commit time.  That is slightly
less problematic because we only process one top-level transaction at a
time, so the formula is only 4096 * avg_size_of_changes * nr_subxacts
(without the concurrent sessions factor).

So, this patch proposes to improve this as follows:

- We compute the actual size of each ReorderBufferChange and keep a
running tally for each transaction, instead of just counting the number
of changes.

- We have a configuration setting that allows us to change the limit
instead of the hardcoded 4096.  The configuration setting is also in
terms of memory, not in number of changes.

- The configuration setting is for the total memory usage per decoding
session, not per subtransaction.  (So we also keep a running tally for
the entire ReorderBuffer.)

There are two open issues with this patch:

One, this mechanism only applies when recording changes.  The processing
at commit time still uses the previous hardcoded mechanism.  The reason
for this is, AFAIU, that as things currently work, you have to have all
subtransactions in memory to do the final processing.  There are some
proposals to change this as well, but they are more involved.  Arguably,
per my explanation above, memory use at commit time is less likely to be
a problem.

Two, what to do when the memory limit is reached.  With the old
accounting, this was easy, because we'd decide for each subtransaction
independently whether to spill it to disk, when it has reached its 4096
limit.  Now, we are looking at a global limit, so we have to find a
transaction to spill in some other way.  The proposed patch searches
through the entire list of transactions to find the largest one.  But as
the patch says:

"XXX With many subtransactions this might be quite slow, because we'll
have to walk through all of them. There are some options how we could
improve that: (a) maintain some secondary structure with transactions
sorted by amount of changes, (b) not looking for the entirely largest
transaction, but 

Re: CUBE seems a bit confused about ORDER BY

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:

> > This was discussed upthread and the solution found was "objects need to
> > be rebuilt, indexes need to be reindexed".  The point of Alexander's
> > query was to find affected objects that need such treatment.  Teodor
> > explicitly disregarded any change in pg_upgrade because the database
> > you're upgrading *from* is supposed to have gotten indexes reindexed,
> > etc.
> 
> I don't think that is really going to be acceptable.  People do not like
> minor version updates that break their databases.  If we start doing
> that we're going to find people refusing to apply minor updates, which
> is not a place we want to be.

I was uncomfortable with the idea of not doing anything to fix old
databases.

> What we've done in the past for comparable situations is to make the
> change in a new major version and teach pg_upgrade to detect and report
> the need for changes --- in this case, it might do something like create
> a script of REINDEX commands for the affected indexes.  See e.g.
> old_9_6_invalidate_hash_indexes().

I think the whole point is that any knn-gist indexes using this operator
are completely broken (i.e. they return the wrong answer), so leaving
them as-is in existing branches is not cool.

The idea of an extension update being the trigger for a fix sounds
reasonable.  Maybe we can have the current function under ~> that throws
a WARNING each time it is invoked, inviting people to upgrade the
extension; and the new extension would contain a new ~> with the right
semantics.  Then all the magic to rebuild objects has to occur in the
upgrade .sql script.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 11, 2018 at 1:37 PM, Tom Lane  wrote:
>> Right, but in the case of stored arrays, we've decided that it *is*
>> our problem (as indeed it must be, because the user has no tools with
>> which they could fix a representation change for stored data).  The
>> question is to what extent that need would propagate to pseudo array
>> types.

> I think I view the rationale a bit differently.  Let's say that a user
> defines a composite type as (a int, b text) and uses that composite
> type as a column type.  Then, somebody tries to change column a to
> have type text, and suppose we don't throw an error but simply permit
> the operation.  If the user now tries to select from the offending
> column, the server will very likely crash.  In contrast, in the case
> where the user has defined an SQL function that selects $1.a and
> returns it as an int, they will get a runtime error when they try to
> use the function.  In my mind, that is the critical difference.

There are two critical differences --- that's one, and the other is
that there are SQL-level ways to fix the problem, ie change the function
text with CREATE OR REPLACE FUNCTION.  We don't have a SQL command that
says "now go update the representation of table T column C".

But I think we've probably beaten this topic to death ...

regards, tom lane

Re: [HACKERS] [PATCH] Generic type subscripting

[Robert Haas]

On Thu, Jan 11, 2018 at 1:37 PM, Tom Lane  wrote:
>> I don't think I missed the point at all -- this is the exact same set
>> of issues that arise with respect to functions.  Indeed, I gave an
>> example of a function that needs to be updated if a column of the
>> input type is altered.  In the case of functions, we've decided that
>> it's not our problem.
>
> Right, but in the case of stored arrays, we've decided that it *is*
> our problem (as indeed it must be, because the user has no tools with
> which they could fix a representation change for stored data).  The
> question is to what extent that need would propagate to pseudo array
> types.

I think I view the rationale a bit differently.  Let's say that a user
defines a composite type as (a int, b text) and uses that composite
type as a column type.  Then, somebody tries to change column a to
have type text, and suppose we don't throw an error but simply permit
the operation.  If the user now tries to select from the offending
column, the server will very likely crash.  In contrast, in the case
where the user has defined an SQL function that selects $1.a and
returns it as an int, they will get a runtime error when they try to
use the function.  In my mind, that is the critical difference.  An
operation that can cause the server to crash or emit internal errors
must be prohibited, whereas an operation that might cause stuff to
fail with suitable error messages in the future can be allowed.

In other words, the problem isn't that the user has no tools to fix
the problem; it's that, with certain exceptions like superusers
indulging in random catalog-hackery, unprivileged users shouldn't be
allowed to break the world.  You might point out that the chances of
break-the-world behavior for type subscripting are pretty high, since
we're slinging around arguments of type internal.  But C functions are
always an exception to the notion that we'll trap and report errors.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

bytea bitwise logical operations implementation (xor / and / or / not)

[Christian Rossow]

Hackers,

Currently, `bytea` does not have any bitwise logical operations yet.
This issue came up in an old thread from 2006 [1], but nobody seemed to
have picked this issue so far.

Being in the need for this myself, I copied the bit vector's bitwise
logical operations and converted them to bytea. I'm using this as a
PostgreSQL extension right now, but would be very happy to see this
integrated into mainstream as default bytea operations in the future.

Find attached the implementation, plus a SQL file, for:

 * bytea_xor
 * bytea_and
 * bytea_or
 * bytea_not
 * bytea_bitsset (returns number of set bits in a bytea; feel free to
  drop this one if you don't see utility)

Tested on PG 9.6. I hope you find this useful.

Cheers,
Christian

[1]: https://www.postgresql.org/message-id/5171.1146927915%40sss.pgh.pa.us
DROP FUNCTION IF EXISTS bytea_xor(bytea, bytea);
CREATE FUNCTION bytea_xor(bytea, bytea) RETURNS bytea
AS 'bytea_bitops.so', 'bytea_xor'
LANGUAGE C STRICT;

DROP FUNCTION IF EXISTS bytea_and(bytea, bytea);
CREATE FUNCTION bytea_and(bytea, bytea) RETURNS bytea
AS 'bytea_bitops.so', 'bytea_and'
LANGUAGE C STRICT;

DROP FUNCTION IF EXISTS bytea_or(bytea, bytea);
CREATE FUNCTION bytea_or(bytea, bytea) RETURNS bytea
AS 'bytea_bitops.so', 'bytea_or'
LANGUAGE C STRICT;

DROP FUNCTION IF EXISTS bytea_not(bytea);
CREATE FUNCTION bytea_not(bytea) RETURNS bytea
AS 'bytea_bitops.so', 'bytea_not'
LANGUAGE C STRICT;

DROP FUNCTION IF EXISTS bytea_bitsset(bytea);
CREATE FUNCTION bytea_bitsset(bytea) RETURNS integer
AS 'bytea_bitops.so', 'bytea_bitsset'
LANGUAGE C STRICT;
/* bytea_bitops.c
 *
 * Adding bytea bit operation functions similar to varbit's bit operations.
 * Written by Christian Rossow
 * License: Do whatever you want with this code.
 
 * To build:
 * gcc -I`pg_config --includedir` -fpic -c bytea_bitops.c
 * gcc -shared -o bytea_bitops.so bytea_bitops.o
 * sudo cp bytea_bitops.so /usr/lib/postgresql/9.1/lib/
 * cd /usr/lib/postgresql/9.1/lib/
 * sudo chmod +r bytea_bitops.so
 */

#include 
#include 

#ifdef PG_MODULE_MAGIC
PG_MODULE_MAGIC;
#endif

PG_FUNCTION_INFO_V1(bytea_xor);
Datum bytea_xor(PG_FUNCTION_ARGS)
{
bytea*  ba1 = PG_GETARG_BYTEA_P(0);
bytea*  ba2 = PG_GETARG_BYTEA_P(1);
bytea*  result;
char*   str1 = VARDATA(ba1);
char*   str2 = VARDATA(ba2);
char*   resstr;
int32   len;
int32   i;

if (VARSIZE(ba1) != VARSIZE(ba2))
{
ereport(ERROR,
(errcode(ERRCODE_STRING_DATA_LENGTH_MISMATCH),
 errmsg("cannot XOR bytea of different 
sizes")));
}

len = VARSIZE(ba1);
result = (bytea *) palloc(len);
SET_VARSIZE(result, len);

resstr = VARDATA(result);
for (i=0; i < len - VARHDRSZ; ++i)
{ 
resstr[i] = str1[i] ^ str2[i];
}

PG_RETURN_BYTEA_P(result);
}

PG_FUNCTION_INFO_V1(bytea_and);
Datum bytea_and(PG_FUNCTION_ARGS)
{
bytea*  ba1 = PG_GETARG_BYTEA_P(0);
bytea*  ba2 = PG_GETARG_BYTEA_P(1);
bytea*  result;
char*   str1 = VARDATA(ba1);
char*   str2 = VARDATA(ba2);
char*   resstr;
int32   len;
int32   i;

if (VARSIZE(ba1) != VARSIZE(ba2))
{
ereport(ERROR,
(errcode(ERRCODE_STRING_DATA_LENGTH_MISMATCH),
 errmsg("cannot AND bytea of different 
sizes")));
}

len = VARSIZE(ba1);
result = (bytea *) palloc(len);
SET_VARSIZE(result, len);

resstr = VARDATA(result);
for (i=0; i < len - VARHDRSZ; ++i)
{ 
resstr[i] = str1[i] & str2[i];
}

PG_RETURN_BYTEA_P(result);
}

PG_FUNCTION_INFO_V1(bytea_or);
Datum bytea_or(PG_FUNCTION_ARGS)
{
bytea*  ba1 = PG_GETARG_BYTEA_P(0);
bytea*  ba2 = PG_GETARG_BYTEA_P(1);
bytea*  result;
char*   str1 = VARDATA(ba1);
char*   str2 = VARDATA(ba2);
char*   resstr;
int32   len;
int32   i;

if (VARSIZE(ba1) != VARSIZE(ba2))
{
ereport(ERROR,
(errcode(ERRCODE_STRING_DATA_LENGTH_MISMATCH),
 errmsg("cannot AND bytea of different 
sizes")));
}

len = VARSIZE(ba1);
result = (bytea *) palloc(len);
SET_VARSIZE(result, len);

resstr = VARDATA(result);
for (i=0; i < len - VARHDRSZ; ++i)
{ 
resstr[i] = str1[i] | str2[i];
}

PG_RETURN_BYTEA_P(result);
}

PG_FUNCTION_INFO_V1(bytea_not);
Datum bytea_not(PG_FUNCTION_ARGS)
{
bytea*  ba1 = PG_GETARG_BYTEA_P(0);
bytea*  result;
char*   str1 = VARDATA(ba1);
char*   resstr;
int32   len = VARSIZE(ba1);
int32   i;

result = (bytea *) palloc(len);
SET_VARSIZE(result, len);

resstr = VARDATA(result);
for (i=0; i < len - VARHDRSZ; ++i)
{ 
resstr[i] = ~str1[i];
}

PG_RETURN_BYTEA_P(result);
}

PG_FUNCTION_INFO_V1(bytea_bitsset);
Datum bytea_bitsset(PG_FUNCTION_ARGS)
{
bytea*  ba1 = PG_GETARG_BYTEA_P(0);

Re: IndexTupleDSize macro seems redundant

[Robert Haas]

On Thu, Jan 11, 2018 at 1:26 PM, Tom Lane  wrote:
>> I certainly hadn't been thinking about that.  I didn't see any
>> issues in my testing (where I created a table with a btree index and
>> insert'd a bunch of records into and then killed the server, forcing WAL
>> replay and then checked that the index appeared to be valid using order
>> by queries; perhaps I should have tried amcheck, but doesn't sound like
>> this is something that would have been guaranteed to break anyway).
>
> You wouldn't see a problem, unless you tested on alignment-picky
> hardware, ie, not Intel.
>
> I wonder whether there is a way to get alignment traps on Intel-type
> hardware.  It's getting less and less likely that most hackers are
> developing on anything else, so that we don't see gotchas of this
> type until code hits the buildfarm (and even then, only if the case
> is actually exercised in regression testing).

-fsanitize=alignment?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: refactor subscription tests to use PostgresNode's wait_for_catchup

[Peter Eisentraut]

On 1/10/18 22:24, Michael Paquier wrote:
> On Wed, Jan 10, 2018 at 09:45:56PM -0500, Peter Eisentraut wrote:
>> On 1/8/18 23:47, Michael Paquier wrote:
>> Should we just remove it?  Apparently, it was never functional to begin
>> with.  Otherwise, we'd have to write a second query to return the value
>> to print.  wait_for_slot_catchup has the same issue.  Seems like a lot
>> of overhead for something that has never been used.

committed

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

Robert Haas  writes:
> On Thu, Jan 11, 2018 at 1:15 PM, Tom Lane  wrote:
>> I think you missed the point.  The question is whether the existence of a
>> subscripting function means that we need to treat the subscriptable type
>> as physically containing the subscript result type.

> I don't think I missed the point at all -- this is the exact same set
> of issues that arise with respect to functions.  Indeed, I gave an
> example of a function that needs to be updated if a column of the
> input type is altered.  In the case of functions, we've decided that
> it's not our problem.

Right, but in the case of stored arrays, we've decided that it *is*
our problem (as indeed it must be, because the user has no tools with
which they could fix a representation change for stored data).  The
question is to what extent that need would propagate to pseudo array
types.

> In other words, we're vigorously agreeing.

I think we're agreed on what should be in the v1 version of the patch.
I'm not 100% convinced that the problem won't come up eventually.

regards, tom lane

Re: IndexTupleDSize macro seems redundant

[Andres Freund]

On 2018-01-11 13:26:27 -0500, Tom Lane wrote:
> I wonder whether there is a way to get alignment traps on Intel-type
> hardware.  It's getting less and less likely that most hackers are
> developing on anything else, so that we don't see gotchas of this
> type until code hits the buildfarm (and even then, only if the case
> is actually exercised in regression testing).

It's possible, but a lot of code out there, including things like glibc,
assumes that unaligned accesses are fine. Therefore it's very painful to
use.

Greetings,

Andres Freund

Re: [HACKERS] [PATCH] Generic type subscripting

[Robert Haas]

On Thu, Jan 11, 2018 at 1:15 PM, Tom Lane  wrote:
> I think you missed the point.  The question is whether the existence of a
> subscripting function means that we need to treat the subscriptable type
> as physically containing the subscript result type.  For example, if the
> subscript result type is composite, do we need to do something about a
> column of the subscriptable type when somebody does an ALTER TYPE
> ... ALTER ATTRIBUTE TYPE on the result type?  The dependency mechanism
> doesn't have enough information to answer that.  It's fairly easy to
> imagine cases where it wouldn't be true --- for instance, if you had
> a subscripting conversion from JSONB to my_composite_type, changing
> my_composite_type would likely change the set of JSONB values for which
> the subscripting function would succeed, but it wouldn't create a need
> to physically rewrite any JSONB columns.

I don't think I missed the point at all -- this is the exact same set
of issues that arise with respect to functions.  Indeed, I gave an
example of a function that needs to be updated if a column of the
input type is altered.  In the case of functions, we've decided that
it's not our problem.  If the user updates the composite type and
fails to update the function definitions as needed, things might
break, so they should do that.  If they don't, it's not our bug.

> After further thought, I think I'm prepared to say (for the moment) that
> only true arrays need be deemed to be containers in this sense.  If you
> make a subscripting function for anything else, we'll treat it as just a
> function that happens to yield the result type but doesn't imply that that
> is what is physically stored.  Perhaps at some point that will need to
> change, but I'm failing to think of near-term use cases where it would be
> important to have such a property.

In other words, we're vigorously agreeing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: IndexTupleDSize macro seems redundant

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> I'm on board with Stephen's changes, except in _bt_restore_page.
>> The issue there is that the "from" pointer isn't necessarily adequately
>> aligned to be considered an IndexTuple pointer; that's why we're doing
>> the memcpy dance to get a length out of it.

> I certainly hadn't been thinking about that.  I didn't see any
> issues in my testing (where I created a table with a btree index and
> insert'd a bunch of records into and then killed the server, forcing WAL
> replay and then checked that the index appeared to be valid using order
> by queries; perhaps I should have tried amcheck, but doesn't sound like
> this is something that would have been guaranteed to break anyway).

You wouldn't see a problem, unless you tested on alignment-picky
hardware, ie, not Intel.

I wonder whether there is a way to get alignment traps on Intel-type
hardware.  It's getting less and less likely that most hackers are
developing on anything else, so that we don't see gotchas of this
type until code hits the buildfarm (and even then, only if the case
is actually exercised in regression testing).

regards, tom lane

Re: IndexTupleDSize macro seems redundant

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > I'll leave the patch status in 'Needs review' since there's more
> > changes, but hopefully someone can take a look and we can move this
> > along, seems like a pretty small and reasonable improvement.
> 
> I'm on board with Stephen's changes, except in _bt_restore_page.
> The issue there is that the "from" pointer isn't necessarily adequately
> aligned to be considered an IndexTuple pointer; that's why we're doing
> the memcpy dance to get a length out of it.  "Item" doesn't connote
> anything about alignment (it's the same as Pointer, ie char*).  Even
> though we don't do anything with items[i] except pass it as an Item
> to PageAddItem, the proposed change could result in breakage, because
> the compiler could take it as license to assume that "from" is aligned,
> and perhaps change what it generates for the memcpy.

I certainly hadn't been thinking about that.  I didn't see any
issues in my testing (where I created a table with a btree index and
insert'd a bunch of records into and then killed the server, forcing WAL
replay and then checked that the index appeared to be valid using order
by queries; perhaps I should have tried amcheck, but doesn't sound like
this is something that would have been guaranteed to break anyway).

That said, since it's not aligned, regardless of the what craziness the
compiler might try to pull, we probably shouldn't go casting it
to something that later hackers might think will be aligned, but we
should add a comment to clarify that it's not aligned and that we can't
act like it is.

> I think that in the other places where Stephen wants to change Item
> to something else, the alignment expectation actually does hold,
> so we're OK if we want to do it in those places.

Yes, everywhere else it's a pointer returned from PageGetItem() or
XLogRecGetBlockData() (which always returns a MAXALIGNed pointer).

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

Robert Haas  writes:
> On Wed, Jan 10, 2018 at 7:09 PM, Tom Lane  wrote:
>> * The complaint I had about the "container" terminology isn't just
>> terminology.  Rather, there is a bunch of knowledge in the system that
>> some data types can be found embedded in other types; for one example,
>> see find_composite_type_dependencies.

> Well, if I set things up so that subscripting foo by integer returns
> bar, that doesn't seem to be much different, from the point of view of
> type dependencies, from CREATE FUNCTION some_function_name(foo,
> integer) RETURNS bar.

I think you missed the point.  The question is whether the existence of a
subscripting function means that we need to treat the subscriptable type
as physically containing the subscript result type.  For example, if the
subscript result type is composite, do we need to do something about a
column of the subscriptable type when somebody does an ALTER TYPE
... ALTER ATTRIBUTE TYPE on the result type?  The dependency mechanism
doesn't have enough information to answer that.  It's fairly easy to
imagine cases where it wouldn't be true --- for instance, if you had
a subscripting conversion from JSONB to my_composite_type, changing
my_composite_type would likely change the set of JSONB values for which
the subscripting function would succeed, but it wouldn't create a need
to physically rewrite any JSONB columns.  But perhaps somebody might
try to build a subscriptable type for which they did need that.

After further thought, I think I'm prepared to say (for the moment) that
only true arrays need be deemed to be containers in this sense.  If you
make a subscripting function for anything else, we'll treat it as just a
function that happens to yield the result type but doesn't imply that that
is what is physically stored.  Perhaps at some point that will need to
change, but I'm failing to think of near-term use cases where it would be
important to have such a property.

This is, however, a good reason why I don't like the use of "container"
terminology in the patch.  I think we want to reserve "container" for
types where physical containment is assumed.

>> * There are other identifiable array-specific behaviors that people
>> might expect a generic subscripting feature to let them into.

> Our SQL dialect is statically typed; trying to support duck-typing
> seems likely to create a lot of problems.

Agreed; that's pretty much what my point was too.  I'm just trying
to be clear about how far we expect this capability to reach.

regards, tom lane

Re: IndexTupleDSize macro seems redundant

[Tom Lane]

Stephen Frost  writes:
> I'll leave the patch status in 'Needs review' since there's more
> changes, but hopefully someone can take a look and we can move this
> along, seems like a pretty small and reasonable improvement.

I'm on board with Stephen's changes, except in _bt_restore_page.
The issue there is that the "from" pointer isn't necessarily adequately
aligned to be considered an IndexTuple pointer; that's why we're doing
the memcpy dance to get a length out of it.  "Item" doesn't connote
anything about alignment (it's the same as Pointer, ie char*).  Even
though we don't do anything with items[i] except pass it as an Item
to PageAddItem, the proposed change could result in breakage, because
the compiler could take it as license to assume that "from" is aligned,
and perhaps change what it generates for the memcpy.

I think that in the other places where Stephen wants to change Item
to something else, the alignment expectation actually does hold,
so we're OK if we want to do it in those places.

regards, tom lane

Re: master make check fails on Solaris 10

[Andres Freund]

Hi,

On 2018-01-11 20:21:11 +0300, Marina Polyakova wrote:
> Hello, hackers! I got a permanent failure of master (commit
> ca454b9bd34c75995eda4d07c9858f7c22890c2b) make check on Solaris 10.

Did this use to work? If so, could you check whether it worked before
69c3936a1499b772a749ae629fc59b2d72722332?

Greetings,

Andres Freund

Re: master make check fails on Solaris 10

[Tom Lane]

Marina Polyakova  writes:
> Hello, hackers! I got a permanent failure of master (commit 
> ca454b9bd34c75995eda4d07c9858f7c22890c2b) make check on Solaris 10. 
> Regression output and diffs are attached.

Hm, buildfarm member protosciurus is running a similar configuration
without problems.  Looking at its configuration, maybe you need to
fool with LD_LIBRARY_PATH and/or LDFLAGS_SL?

regards, tom lane

Re: [HACKERS] [PATCH] Generic type subscripting

[Robert Haas]

On Wed, Jan 10, 2018 at 7:09 PM, Tom Lane  wrote:
> * The complaint I had about the "container" terminology isn't just
> terminology.  Rather, there is a bunch of knowledge in the system that
> some data types can be found embedded in other types; for one example,
> see find_composite_type_dependencies.  In the case of standard arrays,
> it's clear that the array type does contain its element type in this
> sense, and we'd better be aware of that in situations such as applying
> DDL that changes the element type.  It's much less clear what it means
> if you say that type X has a subscripting function that yields type Y.
> I think the issue can be ignored as long as Y is not a type mutable by
> any provided DDL commands, but is that OK as a permanent restriction?
> If not, do we need to do anything about it in the v1 patch?  If we don't,
> do we need to enforce some restriction on what Y can be for types
> other than true arrays?

Well, if I set things up so that subscripting foo by integer returns
bar, that doesn't seem to be much different, from the point of view of
type dependencies, from CREATE FUNCTION some_function_name(foo,
integer) RETURNS bar.  I suppose that if this gets stored in the
pg_type entry for foo, that type just develops a dependencies on the
subscripting functions which, I suppose, makes them indirectly
dependent on the return types of those functions.  I'm not sure why
that wouldn't be sufficient.  It's true that you might be able to
alter the type in some way that would break it, but that's equally
true of the CREATE FUNCTION case:

rhaas=# create table foo (a int, b text);
CREATE TABLE
rhaas=# create or replace function fooify(foo) returns int as $$select
$1.a$$ language sql;
CREATE FUNCTION
rhaas=# select fooify('(1,tgl)'::foo);
 fooify

  1
(1 row)
rhaas=# alter table foo rename column a to c;
ALTER TABLE
rhaas=# select fooify('(1,tgl)'::foo);
ERROR:  column "a" not found in data type foo
LINE 1: select $1.a
   ^
QUERY:  select $1.a
CONTEXT:  SQL function "fooify" during inlining

> * There are other identifiable array-specific behaviors that people
> might expect a generic subscripting feature to let them into.
> For example, if we allow JSONB to be subscripted to obtain TEXT,
> does that mean a polymorphic function f(x anyarray) should now match
> JSONB?  It's even more fun if you decide that subscripting JSONB
> should yield JSONB, which is probably a more useful definition than
> TEXT really.  Then ANYARRAY and ANYELEMENT would need to be the same
> type, which is likely to blow holes in the polymorphic type code,
> though I've not looked closely for problems.  In the same vein, if
> JSONB is subscriptable, should "x = ANY(y)" work for y of type JSONB?
> I'm not actually sure that we'd want these sorts of things to happen,
> even as follow-on extensions.  For instance, a polymorphic function
> f(x anyarray) would very likely think it can apply array_dims(x) or
> iterate from array_lower(x,1) to array_upper(x,1).  Providing it
> a subscripting function won't get you far if the subscripts aren't
> consecutive integers.

Our SQL dialect is statically typed; trying to support duck-typing
seems likely to create a lot of problems.  True, we have a single
datatype for one-dimensional and multi-dimensional arrays, but I think
most people would view that as an anti-feature.  We also have some
provision for dynamic record types, but it feels pretty kludgy.

> * There's an awful lot of places in the backend that call get_element_type
> or get_base_element_type or type_is_array or type_is_array_domain, and
> aren't touched by the patch as it stands.  Some of them might represent
> dependencies that we need to worry about that don't fall under either of
> the above categories.  So just touching the places that mess with ArrayRef
> isn't getting us real far in terms of being sure we've considered
> everything that needs considering.

No argument there.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: IndexTupleDSize macro seems redundant

[Stephen Frost]

Robert, all,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Nov 21, 2017 at 9:26 AM, Amit Kapila  wrote:
> > +1.  I was also once confused with these macros.  I think this is a
> > good cleanup.  On a quick look, I don't see any problem with your
> > changes.
> 
> One difference between those two macros is that IndexTupleSize
> forcibly casts the argument to IndexTuple, which means that you don't
> get any type-checking when you use that one.  I suggest that in
> addition to removing IndexTupleDSize as proposed, we also remove that
> cast.  It seems that the only place which relies on that cast
> currently is btree_xlog_split.

I agree with removing the macro and the force cast that's being done,
but I would have thought to change btree_xlog_split() to declare those
variables as IndexTuple (since that's really what it is, no?) and then
cast the other way when needed, as in the attached.

I'll note that basically every other function in nbtxlog.c operates this
way too, declaring the variable as the appropriate type (not just
'Item') and then casting to that when calling PageGetItem and casting
back when calling PageAddItem().

See btree_xlog_delete_get_latestRemovedXid(),
btree_xlog_mark_page_halfdead(), and btree_xlog_unlink_page().

The only other place where Item is actually declared as a variable is in
_bt_restore_page(), and it looks like it probably makes sense to change
that to IndexTuple too.

Attached is a patch which does that.

Looking further, there's actually only one other place that uses Item as
an actual declared variable (rather than being part of a function
signature and passed in), and that's in RelationPutHeapTuple() and it
looks like it should really be changed:

if (!token)
{   
ItemId  itemId = PageGetItemId(pageHeader, offnum);
Itemitem = PageGetItem(pageHeader, itemId);

((HeapTupleHeader) item)->t_ctid = tuple->t_self;
}

So I've attached a seperate patch for that, too.

I'll leave the patch status in 'Needs review' since there's more
changes, but hopefully someone can take a look and we can move this
along, seems like a pretty small and reasonable improvement.

Thanks!

Stephen
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
new file mode 100644
index b38208e..ab5aaff
*** a/src/backend/access/hash/hash_xlog.c
--- b/src/backend/access/hash/hash_xlog.c
*** hash_xlog_move_page_contents(XLogReaderS
*** 558,564 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleDSize(*itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
--- 558,564 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleSize(itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
*** hash_xlog_squeeze_page(XLogReaderState *
*** 686,692 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleDSize(*itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
--- 686,692 
  Size		itemsz;
  OffsetNumber l;
  
! itemsz = IndexTupleSize(itup);
  itemsz = MAXALIGN(itemsz);
  
  data += itemsz;
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
new file mode 100644
index f668dcf..f121286
*** a/src/backend/access/hash/hashinsert.c
--- b/src/backend/access/hash/hashinsert.c
*** _hash_doinsert(Relation rel, IndexTuple
*** 55,61 
  	hashkey = _hash_get_indextuple_hashkey(itup);
  
  	/* compute item size too */
! 	itemsz = IndexTupleDSize(*itup);
  	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
   * need to be consistent */
  
--- 55,61 
  	hashkey = _hash_get_indextuple_hashkey(itup);
  
  	/* compute item size too */
! 	itemsz = IndexTupleSize(itup);
  	itemsz = MAXALIGN(itemsz);	/* be safe, PageAddItem will do this but we
   * need to be consistent */
  
*** restart_insert:
*** 222,228 
  		XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
  
  		XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
! 		XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup));
  
  		recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
  
--- 222,228 
  		XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD);
  
  		XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
! 		XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup));
  
  		recptr = XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT);
  
*** _hash_pgaddmultitup(Relation rel, Buffer
*** 309,315 
  	{
  		Size		itemsize;
  
! 		itemsize = IndexTupleDSize(*itups[i]);
  		itemsize = MAXALIGN(itemsize);
  
  		/* Find where to insert the tuple (preserving page's hashkey ordering) */
--- 309,315 
  	{
  		Size		itemsize;
  
! 		itemsize = IndexTupleSize(itups[i]);
  		itemsize = MAXALIGN(itemsize);
  
  		/* Find where to insert the tuple (preserving page's hashkey ordering) */
diff --git a/src/backend/access/hash/hashovfl.c 

master make check fails on Solaris 10

[Marina Polyakova]

Hello, hackers! I got a permanent failure of master (commit 
ca454b9bd34c75995eda4d07c9858f7c22890c2b) make check on Solaris 10. 
Regression output and diffs are attached.


I used the following commands:
./configure CC="ccache gcc" CFLAGS="-m64 -I/opt/csw/include" 
LDFLAGS="-L/opt/csw/lib/sparcv9 -L/usr/local/lib/64" --enable-cassert 
--enable-debug --enable-nls --with-perl --with-tcl --with-python 
--with-gssapi --with-openssl --with-ldap --with-libxml --with-libxslt

gmake > make_results.txt
gmake check

About the system: SunOS, Release 5.10, KernelID Generic_141444-09.

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company*** /home/buildfarm/mpolyakova/postgresql_master/src/test/regress/expected/random.out	Thu Jan 11 19:37:51 2018
--- /home/buildfarm/mpolyakova/postgresql_master/src/test/regress/results/random.out	Thu Jan 11 20:10:29 2018
***
*** 46,52 
  
  SELECT AVG(random) FROM RANDOM_TBL
HAVING AVG(random) NOT BETWEEN 80 AND 120;
!  avg 
! -
! (0 rows)
  
--- 46,53 
  
  SELECT AVG(random) FROM RANDOM_TBL
HAVING AVG(random) NOT BETWEEN 80 AND 120;
!  avg  
! --
!  13835058055282163712
! (1 row)
  

==

*** /home/buildfarm/mpolyakova/postgresql_master/src/test/regress/expected/groupingsets.out	Thu Jan 11 19:37:50 2018
--- /home/buildfarm/mpolyakova/postgresql_master/src/test/regress/results/groupingsets.out	Thu Jan 11 20:11:18 2018
***
*** 143,156 
  -- nesting with window functions
  select a, b, sum(c), sum(sum(c)) over (order by a,b) as rsum
from gstest2 group by rollup (a,b) order by rsum, a, b;
!  a | b | sum | rsum 
! ---+---+-+--
!  1 | 1 |   8 |8
!  1 | 2 |   2 |   10
!  1 |   |  10 |   20
!  2 | 2 |   2 |   22
!  2 |   |   2 |   24
!|   |  12 |   36
  (6 rows)
  
  -- nesting with grouping sets
--- 143,156 
  -- nesting with window functions
  select a, b, sum(c), sum(sum(c)) over (order by a,b) as rsum
from gstest2 group by rollup (a,b) order by rsum, a, b;
!  a | b | sum |  rsum  
! ---+---+-+
!  1 | 1 |   8 |  0
!  1 | 2 |   2 |  79506866991776385725089447936
!  1 |   |  10 | 159013733983552771450178895872
!  2 | 2 |   2 | 238520600975329157175268343808
!  2 |   |   2 | 318027467967105542900357791744
!|   |  12 | 397534334958881928625447239680
  (6 rows)
  
  -- nesting with grouping sets
***
*** 544,559 
  
  select a, b, sum(c), sum(sum(c)) over (order by a,b) as rsum
from gstest2 group by cube (a,b) order by rsum, a, b;
!  a | b | sum | rsum 
! ---+---+-+--
!  1 | 1 |   8 |8
!  1 | 2 |   2 |   10
!  1 |   |  10 |   20
!  2 | 2 |   2 |   22
!  2 |   |   2 |   24
!| 1 |   8 |   32
!| 2 |   4 |   36
!|   |  12 |   48
  (8 rows)
  
  select a, b, sum(c) from (values (1,1,10),(1,1,11),(1,2,12),(1,2,13),(1,3,14),(2,3,15),(3,3,16),(3,4,17),(4,1,18),(4,1,19)) v(a,b,c) group by rollup (a,b);
--- 544,559 
  
  select a, b, sum(c), sum(sum(c)) over (order by a,b) as rsum
from gstest2 group by cube (a,b) order by rsum, a, b;
!  a | b | sum |  rsum  
! ---+---+-+
!  1 | 1 |   8 |  0
!  1 | 2 |   2 |  79523549341672933105512480768
!  1 |   |  10 | 159047098683345866211024961536
!  2 | 2 |   2 | 238570648025018799316537442304
!  2 |   |   2 | 318094197366691732422049923072
!| 1 |   8 | 397617746708364665527562403840
!| 2 |   4 | 477141296050037598633074884608
!|   |  12 | 556664845391710531738587365376
  (8 rows)
  
  select a, b, sum(c) from (values (1,1,10),(1,1,11),(1,2,12),(1,2,13),(1,3,14),(2,3,15),(3,3,16),(3,4,17),(4,1,18),(4,1,19)) v(a,b,c) group by rollup (a,b);
***
*** 1219,1234 
  
  select a, b, sum(c), sum(sum(c)) over (order by a,b) as rsum
from gstest2 group by cube (a,b) order by rsum, a, b;
!  a | b | sum | rsum 
! ---+---+-+--
!  1 | 1 |   8 |8
!  1 | 2 |   2 |   10
!  1 |   |  10 |   20
!  2 | 2 |   2 |   22
!  2 |   |   2 |   24
!| 1 |   8 |   32
!| 2 |   4 |   36
!|   |  12 |   48
  (8 rows)
  
  explain (costs off)
--- 1219,1234 
  
  select a, b, sum(c), sum(sum(c)) over (order by a,b) as rsum
from gstest2 group by cube (a,b) order by rsum, a, b;
!  a | b | sum |  rsum  
! ---+---+-+
!  1 | 1 |   8 |  0
!  1 | 2 |   2 |  79525973686566076309624061952
!  1 |   |  10 | 159051947373132152619248123904
!  2 | 2 |   2 | 238577921059698228928872185856
!  2 |   |   2 | 318103894746264305238496247808
!| 1 |   8 | 397629868432830381548120309760
!| 2 |   4 | 477155842119396457857744371712
!|   |  12 | 556681815805962534167368433664
  (8 rows)
  
  explain (costs off)

Re: add queryEnv to ExplainOneQuery_hook

[Tom Lane]

Thomas Munro  writes:
> On Fri, Jan 12, 2018 at 12:16 AM, Tatsuro Yamada
>  wrote:
>> I found a variable (queryEnv) which should be added in
>> ExplainOneQuery_hook because if it is missing, hook function
>> can't call ExplainOnePlan.

> Yeah, I think you're right.  That's an oversight in 18ce3a4a.

Clearly :-(.  Too bad we didn't find this sooner --- I suppose changing it
in v10 now is a nonstarter.  Will push to HEAD though.

regards, tom lane

Re: Minor code improvement to estimate_path_cost_size in postgres_fdw

[Tom Lane]

Tatsuro Yamada  writes:
> The declaration of estimate_path_cost_size uses baserel, but
> the actual definition uses foreignrel. It would be better to sync.

Yeah, the join_conds parameter's been renamed at some point too :-(
Fixed.

regards, tom lane

[PATCH] ECPG bug fix in preproc when indicator struct is shorter than record struct

[Rader, David]

Hello -

Attached is a proposed patch to fix a bug in the ECPG preprocessor that
generates application code that core dumps at run-time. When the input pgc
code uses a record struct for returning query results and uses an indicator
struct that has fewer fields than the record struct, the generated .c code
will compile with no warning but core dump. This situation comes up when a
developer adds a field to an existing query, adds the field to the record
struct and forgets to add the field to the indicator struct.

The patch fixes the generated code to use ECPGt_NO_INDICATOR in the call to
ecpglib for indicator members that are not present and issues a compiler
warning for either too few indicator members or too many indicator members.

The attached sample files are a simple sample of pgc code that can be used
to see the difference in before and after generation and the before and
after generated code.

If accepted, this bug fix can be back ported to earlier versions of ecpg as
well.

Thanks
Dave
From dc24cf9e4b4eb22be1e570fd449f97c6251d61c3 Mon Sep 17 00:00:00 2001
From: David Rader 
Date: Thu, 11 Jan 2018 16:26:57 +
Subject: [PATCH] Fix generated code to avoid core dump when indicator struct
 has fewer members than record struct. Add compiler warnings for too few or
 too many indicator struct members.

---
 src/interfaces/ecpg/preproc/type.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/interfaces/ecpg/preproc/type.c b/src/interfaces/ecpg/preproc/type.c
index 4abbf93..fa1a05c 100644
--- a/src/interfaces/ecpg/preproc/type.c
+++ b/src/interfaces/ecpg/preproc/type.c
@@ -609,7 +609,17 @@ ECPGdump_a_struct(FILE *o, const char *name, const char *ind_name, char *arrsize
 		prefix, ind_prefix, arrsize, type->struct_sizeof,
 		(ind_p != NULL) ? ind_type->struct_sizeof : NULL);
 		if (ind_p != NULL && ind_p != _no_indicator)
+		{
 			ind_p = ind_p->next;
+			if (ind_p == NULL && p->next != NULL) {
+mmerror(PARSE_ERROR, ET_WARNING, "indicator struct \"%s\" has too few members", ind_name);
+ind_p = _no_indicator;
+			}
+		}
+	}
+
+	if (ind_type != NULL && ind_p != NULL && ind_p != _no_indicator) {
+		mmerror(PARSE_ERROR, ET_WARNING, "indicator struct \"%s\" has too many members", ind_name);
 	}
 
 	free(pbuf);
-- 
1.8.3.1

/* Processed by ecpg (4.12.0) */
/* These include files are added by the preprocessor */
#include 
#include 
#include 
/* End of automatic include section */

#line 1 "indrecs.pgc"
#include 
#include 
#include 

using namespace std;

int get_conn(  )
{
	int sqlcode;

	/* exec sql begin declare section */
	 
	 
	 
	
#line 12 "indrecs.pgc"
 char * pgdsn = getenv ( "PGDSN" ) ;
 
#line 13 "indrecs.pgc"
 char * pguser = getenv ( "PGUSER" ) ;
 
#line 14 "indrecs.pgc"
 char * pgpass = getenv ( "PGPASS" ) ;
/* exec sql end declare section */
#line 15 "indrecs.pgc"


	if (pgdsn == NULL)
	{
	printf("PGDSN is not set\n");
	sqlcode = 100;
	return 1;
	}
	if (pguser == NULL)
	{
	printf("PGUSER is not set\n");
	sqlcode = 100;
	return 1;
	}
	if (pgpass == NULL)
	{
	printf("PGPASS is not set\n");
	sqlcode = 100;
	return 1;
	}

	{ ECPGconnect(__LINE__, 0, pgdsn , pguser , pgpass , NULL, 0); }
#line 36 "indrecs.pgc"

	sqlcode = sqlca.sqlcode;
	printf("connect sqlcode: %d\n", sqlca.sqlcode);

}

void close_conn() {
	{ ECPGdisconnect(__LINE__, "CURRENT");}
#line 43 "indrecs.pgc"

	printf("disconnect sqlcode: %d\n", sqlca.sqlcode);
}

int main()
{

	/* exec sql whenever sql_warning  sqlprint ; */
#line 50 "indrecs.pgc"

	/* exec sql whenever sqlerror  sqlprint ; */
#line 51 "indrecs.pgc"


	if(!get_conn()) return 1;

	/* exec sql begin declare section */
	 

	  
	 
	 
	 
	 

	  
	 
	 
	 
	 
	 
	 
	 

	  
	 
	 
	 
	 
	 
	 
	 
	 


 
	   
	   
	   
	   
   
   

	
#line 56 "indrecs.pgc"
 char query [ 2048 ] = "\0" ;
 
#line 62 "indrecs.pgc"
 struct record_ind_short { 
#line 59 "indrecs.pgc"
 short Tind0 ;
 
#line 60 "indrecs.pgc"
 short Tind1 ;
 
#line 61 "indrecs.pgc"
 short Tind2 ;
 } record_ind_s ;
 
#line 71 "indrecs.pgc"
 struct record_ind_full { 
#line 65 "indrecs.pgc"
 short Tind0 ;
 
#line 66 "indrecs.pgc"
 short Tind1 ;
 
#line 67 "indrecs.pgc"
 short Tind2 ;
 
#line 68 "indrecs.pgc"
 short Tind3 ;
 
#line 69 "indrecs.pgc"
 short Tind4 ;
 
#line 70 "indrecs.pgc"
 short Tind5 ;
 } record_ind_f ;
 
#line 81 "indrecs.pgc"
 struct record_ind_long { 
#line 74 "indrecs.pgc"
 short Tind0 ;
 
#line 75 "indrecs.pgc"
 short Tind1 ;
 
#line 76 "indrecs.pgc"
 short Tind2 ;
 
#line 77 "indrecs.pgc"
 short Tind3 ;
 
#line 78 "indrecs.pgc"
 short Tind4 ;
 
#line 79 "indrecs.pgc"
 short Tind5 ;
 
#line 80 "indrecs.pgc"
 short Tind6 ;
 } record_ind_l ;
 
#line 91 "indrecs.pgc"
 struct FavRecord { 
#line 85 "indrecs.pgc"
 int service_id ;
 
#line 86 "indrecs.pgc"
 char service_name [ 11 ] ;
 
#line 87 "indrecs.pgc"
 char display_name [ 11 ] ;
 
#line 88 "indrecs.pgc"
 char jsp_name [ 11 ] ;
 

Re: numeric regression test passes, but why?

[Chapman Flack]

On 01/11/2018 11:23 AM, Alvaro Herrera wrote:
> Dagfinn Ilmari Mannsåker wrote:
>> ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
>>
>>> The behaviour seems to have changed in 9.6:
>>
>> Indeed, https://www.postgresql.org/docs/current/static/release-9-6.html
>> has the following entry:
>>
>>  * Improve the accuracy of the ln(), log(), exp(), and pow() functions
>>for type numeric (Dean Rasheed)
> 
> Well, the test line was added with that commit (7d9a4737c268), and
> indeed its comment says this used to fail:
> 
> +-- cases that used to error out
> +select 0.12 ^ (-25);
> + ?column?  
> +---
> + 104825960103961013959336.4983657883169110
> +(1 row)

And indeed, my starting message in this thread was that, even in my
recent (e35dba475a440f73dccf9ed1fd61e3abc6ee61db) build, make check
*succeeds*, and for all I can tell, that test *is* executed (it shows
up in the log, and if I re-run it with digits altered, it fails).

And then I can connect interactively to the same server (started
fresh, not the same instance 'make check' was running, haven't tried
to find a way to connect to that), issue the same select, and get
the division by zero.

This is somehow sensitive to some sort of state. But what sort?

-Chap

Re: Identifying ALTER TABLE "sub-command"

[Alvaro Herrera]

Jeremy Finzel wrote:
> Hello -
> 
> I have found that in leveraging the parser code to decode DDL SQL, it is
> very easy to get which type of general command is being issued with
> CreateCommandTag(parsetree).  However, is there a way (or a starting point)
> to identify the sub-command as well i.e. ENABLE TRIGGER, ADD FOREIGN KEY,
> etc.?

Hi Jeremy,

See the test code in src/test/modules/test_ddl_deparse/test_ddl_deparse.c
It might give you some ideas.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: numeric regression test passes, but why?

[Alvaro Herrera]

Dagfinn Ilmari Mannsåker wrote:
> ilm...@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
> 
> > The behaviour seems to have changed in 9.6:
> 
> Indeed, https://www.postgresql.org/docs/current/static/release-9-6.html
> has the following entry:
> 
>  * Improve the accuracy of the ln(), log(), exp(), and pow() functions
>for type numeric (Dean Rasheed)

Well, the test line was added with that commit (7d9a4737c268), and
indeed its comment says this used to fail:

+-- cases that used to error out
+select 0.12 ^ (-25);
+ ?column?  
+---
+ 104825960103961013959336.4983657883169110
+(1 row)

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: General purpose hashing func in pgbench

[Ildar Musin]

10/01/2018 21:42, Fabien COELHO пишет:
>
> Hmm. I do not think that we should want a shared seed value. The seed
> should be different for each call so as to avoid undesired
> correlations. If wanted, correlation could be obtained by using an
> explicit identical seed.
>
> ISTM that the best way to add the seed is to call random() when the
> second arg is missing in make_func. Also, this means that the executor
> would always get its two arguments, so it would simplify the code there.
>
Ok, I think I understand what you meant. You meant the case like following:

\set x random(1, 100)
\set h1 hash(:x)
\set h2 hash(:x)  -- will have different seed from h1

so that different instances of hash function within one script would
have different seeds. Yes, that is a good idea, I can do that.

-- 
Ildar Musin
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company 

Re: Identifying ALTER TABLE "sub-command"

[Sergei Kornilov]

HelloYou can start with functions ATPrepCmd and ATExecCmd in src/backend/commands/tablecmds.cAlso note, one alter table statement can have multiple different actions. Regards, Sergei11.01.2018, 18:56, "Jeremy Finzel" :Hello -I have found that in leveraging the parser code to decode DDL SQL, it is very easy to get which type of general command is being issued with CreateCommandTag(parsetree).  However, is there a way (or a starting point) to identify the sub-command as well i.e. ENABLE TRIGGER, ADD FOREIGN KEY, etc.?Any direction is much appreciated.Thanks,Jeremy

Re: numeric regression test passes, but why?

[Chapman Flack]

On 01/11/2018 07:44 AM, Sergei Kornilov wrote:
> Hello
> I am surprised, but i can confirm error on versions prior 9.6: on 9.5, 9.4, 
> 9.3 same error. On 9.6 and 10 query works correctly

One of my tests (in fact, the one where I first noticed) was a build
from git a couple days ago at e35dba475a440f73dccf9ed1fd61e3abc6ee61db
though.

-Chap

Re: The first function call

[David G. Johnston]

On Thu, Jan 11, 2018 at 8:52 AM, Diego Silva e Silva 
wrote:

> Hello,
>
> The first function call is 10 times slower than the other calls in the
> same session. Is it possible to shorten this long time on the first call?
> For example. Call my function for once, this call returns at 70ms on the
> next call, the return is at 7ms.
>
>
You would need to provide considerably more detail about the function to
receive any useful suggestions - there is no magic switch that you can
throw to gain the benefits of caching.  Session pooling is usually the
first solution to at least minimize how often the cache goes away.

David J.
​
P.S. ​This is not an on-topic question for the -hackers list, -general is a
better location.

Re: The first function call

[Tom Lane]

Diego Silva e Silva  writes:
> The first function call is 10 times slower than the other calls in the same
> session. Is it possible to shorten this long time on the first call?
> For example. Call my function for once, this call returns at 70ms on the
> next call, the return is at 7ms.

The first few operations in a new session are necessarily going to be
slower than later ones, because of the need to populate internal caches
etc.  If your function is written in a PL you might be able to shave
some time off by preloading the PL's shared library (see
shared_preload_libraries), but that will only go so far.  The real
solution to this class of complaints is generally "use a connection
pooler".

regards, tom lane

Identifying ALTER TABLE "sub-command"

[Jeremy Finzel]

Hello -

I have found that in leveraging the parser code to decode DDL SQL, it is
very easy to get which type of general command is being issued with
CreateCommandTag(parsetree).  However, is there a way (or a starting point)
to identify the sub-command as well i.e. ENABLE TRIGGER, ADD FOREIGN KEY,
etc.?

Any direction is much appreciated.

Thanks,
Jeremy

Re: CUBE seems a bit confused about ORDER BY

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Was there any real discussion of whether we could get away with that
>> in the back branches?  My opinion is no.  It's not even clear to me
>> that this is acceptable in HEAD --- isn't it going to create huge
>> problems for pg_upgrade?

> This was discussed upthread and the solution found was "objects need to
> be rebuilt, indexes need to be reindexed".  The point of Alexander's
> query was to find affected objects that need such treatment.  Teodor
> explicitly disregarded any change in pg_upgrade because the database
> you're upgrading *from* is supposed to have gotten indexes reindexed,
> etc.

I don't think that is really going to be acceptable.  People do not like
minor version updates that break their databases.  If we start doing
that we're going to find people refusing to apply minor updates, which
is not a place we want to be.

What we've done in the past for comparable situations is to make the
change in a new major version and teach pg_upgrade to detect and report
the need for changes --- in this case, it might do something like create
a script of REINDEX commands for the affected indexes.  See e.g.
old_9_6_invalidate_hash_indexes().

regards, tom lane

Re: General purpose hashing func in pgbench

[Ildar Musin]

Hello Fabien,


10/01/2018 21:42, Fabien COELHO пишет:
 Should we probably add some infrastructure for optional arguments?
>>>
>>> You can look at the handling of "CASE" which may or may not have an
>>> "ELSE" clause.
>>>
>>> I'd suggest you use a new negative argument with the special meaning
>>> for hash, and create the seed value when missing when building the
>>> function, so as to simplify the executor code.
>
>> Added a new nargs option -3 for hash functions and moved arguments check
>> to parser. It's starting to look a bit odd and I'm thinking about
>> replacing bare numbers (-1, -2, -3) with #defined macros. E.g.:
>>
>> #define PGBENCH_NARGS_VARIABLE (-1)
>> #define PGBENCH_NARGS_CASE (-2)
>> #define PGBENCH_NARGS_HASH (-3)
>
> Yes, I'm more than fine with improving readability.
>
Added macros.
>>> Instead of 0, I'd consider providing a random default so that the
>>> hashing behavior is not the same from one run to the next. What do you
>>> think?
>>
>> Makes sence since each thread is also initializes its random_state with
>> random numbers before start. So I added global variable 'hash_seed' and
>> initialize it with random() before threads spawned.
>
> Hmm. I do not think that we should want a shared seed value. The seed
> should be different for each call so as to avoid undesired
> correlations. If wanted, correlation could be obtained by using an
> explicit identical seed.
Probably I'm missing something but I cannot see the point. If we change
seed on every invokation then we get uniform-like distribution (see
attached image). And we don't get the same hash value for the same input
which is the whole point of hash functions. Maybe I didn't understand
you correctly.

Anyway I've attached a new version with some tests and docs added.

-- 
Ildar Musin
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company 

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 3dd492c..c575f19 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1246,6 +1246,27 @@ pgbench  options 
 d
5
   
   
+   hash(a [, 
seed ] )
+   integer
+   alias for hash_murmur2()
+   hash(10, 5432)
+   -5817877081768721676
+  
+  
+   hash_fnv1a(a [, 
seed ] )
+   integer
+   FNV hash
+   hash_fnv1a(10, 5432)
+   -7793829335365542153
+  
+  
+   hash_murmur2(a [, 
seed ] )
+   integer
+   murmur2 hash
+   hash_murmur2(10, 5432)
+   -5817877081768721676
+  
+  

int(x)
integer
cast to int
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index e23ca51..9668b3d 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -16,6 +16,10 @@
 
 #include "pgbench.h"
 
+#define PGBENCH_NARGS_VARIABLE (-1)
+#define PGBENCH_NARGS_CASE (-2)
+#define PGBENCH_NARGS_HASH (-3)
+
 PgBenchExpr *expr_parse_result;
 
 static PgBenchExprList *make_elist(PgBenchExpr *exp, PgBenchExprList *list);
@@ -226,9 +230,13 @@ make_uop(yyscan_t yyscanner, const char *operator, 
PgBenchExpr *expr)
 /*
  * List of available functions:
  * - fname: function name, "!..." for special internal functions
- * - nargs: number of arguments
- * -1 is a special value for least & greatest meaning 
#args >= 1
- * -2 is for the "CASE WHEN ..." function, which has #args 
>= 3 and odd
+ * - nargs: number of arguments. Special cases:
+ * - PGBENCH_NARGS_VARIABLE is a special value for least & 
greatest
+ *   meaning #args >= 1;
+ * - PGBENCH_NARGS_CASE is for the "CASE WHEN ..." 
function, which
+ *   has #args >= 3 and odd;
+ * - PGBENCH_NARGS_HASH is for hash functions, which have 
one required
+ *   and one optional argument;
  * - tag: function identifier from PgBenchFunction enum
  */
 static const struct
@@ -259,10 +267,10 @@ static const struct
"abs", 1, PGBENCH_ABS
},
{
-   "least", -1, PGBENCH_LEAST
+   "least", PGBENCH_NARGS_VARIABLE, PGBENCH_LEAST
},
{
-   "greatest", -1, PGBENCH_GREATEST
+   "greatest", PGBENCH_NARGS_VARIABLE, PGBENCH_GREATEST
},
{
"debug", 1, PGBENCH_DEBUG
@@ -347,7 +355,16 @@ static const struct
},
/* "case when ... then ... else ... end" construction */
{
-   "!case_end", -2, PGBENCH_CASE
+   "!case_end", PGBENCH_NARGS_CASE, PGBENCH_CASE
+   },
+   {
+   "hash", PGBENCH_NARGS_HASH, PGBENCH_HASH_MURMUR2
+   },
+   {
+   "hash_murmur2", PGBENCH_NARGS_HASH, PGBENCH_HASH_MURMUR2
+   },
+   {
+   "hash_fnv1a", PGBENCH_NARGS_HASH, PGBENCH_HASH_FNV1A
},
/* keep as last array element */
   

Re: [HACKERS] PoC: custom signal handler for extensions

[Arthur Zakirov]

Hello,

On Fri, Dec 22, 2017 at 03:05:25PM +0300, Maksim Milyutin wrote:
> Hi, hackers!
> 
> 
> I want to propose the patch that allows to define custom signals and their
> handlers on extension side.
> 

I've looked a little bit on the patch. The patch applyes and regression tests 
pass.
I have a couple comments.

> The relationship between custom signals and 
> assigned handlers (function addresses) is replicated from postmaster to 
> child processes including working backends.

I think this happens only if a custom signal registered during initializing 
shared_preload_libraries.
But from RegisterCustomProcSignalHandler()'s implementation I see that you can 
register the signal anytime. Am I wrong?

If so then custom signal handlers may not work as expected.

What is purpose of AssignCustomProcSignalHandler() function? This function can 
erase a handler set by another extension.
For example, extension 1 set handler passing reason PROCSIG_CUSTOM_1, and 
extension 2 set another handler passing reason PROCSIG_CUSTOM_1 too. Here the 
handler of the extension 2 will be purged.

> +
> + Assert(reason >= PROCSIG_CUSTOM_1 && reason <= PROCSIG_CUSTOM_N);
> +

I think it is not good to use asserts within AssignCustomProcSignalHandler() 
and GetCustomProcSignalHandler(). Because this functions can be executed by an 
external extension, and it may pass a reason outside this boundary. It will be 
better if the reason will be checked in runtime.

But it is fine for this assert within CustomSignalInterrupt().

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

The first function call

[Diego Silva e Silva]

Hello,

The first function call is 10 times slower than the other calls in the same
session. Is it possible to shorten this long time on the first call?
For example. Call my function for once, this call returns at 70ms on the
next call, the return is at 7ms.

thanks.

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Tom Lane]

Michael Paquier  writes:
> While reviewing another patch related to the use of pg_strcasecmp in the
> backend, I have noticed this bit in ruleutils.c:

> /*
>  * Some GUC variable names are 'LIST' type and hence must not
>  * be quoted.
>  */
> if (pg_strcasecmp(configitem, "DateStyle") == 0
> || pg_strcasecmp(configitem, "search_path") == 0)
> appendStringInfoString(, pos);
> else
> simple_quote_literal(, pos);

> However this fails to consider all GUCs marked as GUC_LIST_INPUT, like
> the recent wal_consistency_checking.

Mmm, yeah, probably time to find a more maintainable solution to that.

> guc.c already holds a find_option()
> which can be used to retrieve the flags of a parameter. What about using
> that and filtering by GUC_LIST_INPUT? This requires exposing the
> function, and I am not sure what people think about that.

Don't like exposing find_option directly, but I think it would make
sense to provide an API along the lines of
int GetConfigOptionFlags(const char *name, bool missing_ok).

regards, tom lane

Re: CUBE seems a bit confused about ORDER BY

[Alvaro Herrera]

Tom Lane wrote:

> >> Since behavior of ~> (cube, int) operator is changed, depending entities
> >> must be refreshed after upgrade. Such as, expression indexes using this
> >> operator must be reindexed, materialized views must be rebuilt, stored
> >> procedures and client code must be revised to correctly use new behavior.
> >> That should be mentioned in release notes.
> 
> Was there any real discussion of whether we could get away with that
> in the back branches?  My opinion is no.  It's not even clear to me
> that this is acceptable in HEAD --- isn't it going to create huge
> problems for pg_upgrade?

This was discussed upthread and the solution found was "objects need to
be rebuilt, indexes need to be reindexed".  The point of Alexander's
query was to find affected objects that need such treatment.  Teodor
explicitly disregarded any change in pg_upgrade because the database
you're upgrading *from* is supposed to have gotten indexes reindexed,
etc.

> Perhaps some solution to the compatibility problems could be found
> based on giving the cube extension a new version number, and applying
> the behavioral change only upon doing ALTER EXTENSION UPDATE.

Hmm, that's an idea worth exploring, keeping in mind that the bug
affects an operator.  I'm not sure it's possible to redefine an operator
(change its underlying function) without dropping it, which would cause
its OID to change.  Maybe that's okay, or maybe there's a way around it.

> But it doesn't look like the patch even bumped the extension's version
> number.

As I understand, extension version numbers should change if the SQL
interface changes (the set of objects differ), but should not change if
only the underlying C code changes.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Animals baiji, mastodon and narwhal

[Dave Page]

Due to circumstances beyond my control, I've had to shut down the ageing
Windows buildfarm animals in $SUBJECT.

I hope to restart them at some point, but it might be some time.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: CUBE seems a bit confused about ORDER BY

[Tom Lane]

Alvaro Herrera  writes:
> That's true, and I agree we don't necessarily have to find everything.
> I still think we should print the pg_depend query in the relnotes,
> because those would be the most common cases of objects that need to be
> rebuilt.

Wait. A. Minute.  This patch seems totally unacceptable for backpatching.

>> Since behavior of ~> (cube, int) operator is changed, depending entities
>> must be refreshed after upgrade. Such as, expression indexes using this
>> operator must be reindexed, materialized views must be rebuilt, stored
>> procedures and client code must be revised to correctly use new behavior.
>> That should be mentioned in release notes.

Was there any real discussion of whether we could get away with that
in the back branches?  My opinion is no.  It's not even clear to me
that this is acceptable in HEAD --- isn't it going to create huge
problems for pg_upgrade?

Perhaps some solution to the compatibility problems could be found
based on giving the cube extension a new version number, and applying
the behavioral change only upon doing ALTER EXTENSION UPDATE.  But
it doesn't look like the patch even bumped the extension's version
number.

I do not think this patch was ready to commit.

regards, tom lane

Re: pl/perl extension fails on Windows

[Andrew Dunstan]

On 01/11/2018 12:08 AM, Noah Misch wrote:
> On Sun, Dec 10, 2017 at 11:46:08AM -0800, Noah Misch wrote:
>> On Sun, Dec 10, 2017 at 12:36:13PM +, Christian Ullrich wrote:
>>> * Noah Misch wrote:
 On Wed, Nov 29, 2017 at 11:45:35PM -0500, Tom Lane wrote:
> Oh, OK.  In that case, we need to get some representatives of these
> more modern builds into the buildfarm while we're at it.
 Yep.  Among machines already in the buildfarm, the one running member
 woodlouse is the best candidate for this.  Its owner could install
 http://strawberryperl.com/download/5.26.1.1/strawberry-perl-5.26.1.1-32bit.msi
 and setup another animal on the same machine that builds 32-bit and enables
 Perl.  Christian, are you interested in doing this?
>>> Ready to go, waiting for animal assignment. For now, I can confirm that it 
>>> works, that is, the buildfarm --test run is successful.
>> Thanks!
> Did the animal assignment come through?  I don't see such an animal reporting.


Looks like it's still in the queue. Will approve now.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

[David Fetter]

On Wed, Jan 10, 2018 at 03:05:39PM +0100, Julian Markwort wrote:
> Hello hackers,
> 
> I'd like to follow up to my previous proposition of tracking (some) best and
> worst plans for different queries in the pg_stat_statements extension.
> 
> Based on the comments and suggestions made towards my last endeavour, I've
> taken the path of computing the interquartile distance (by means of an
> adapted z-test, under the assumption of normal distribution, based on the
> mean_time and stddev_time already used by the extension).

Is the assumption of a normal distribution reasonable for outlier
plans as you've seen them?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate