[HACKERS] Reporting planning time with EXPLAIN

2016-12-26 Thread Ashutosh Bapat
Hi,
We report planning and execution time when EXPLAIN ANALYZE is issued.
We do not have facility to report planning time as part EXPLAIN
output. In order to get the planning time, one has to issue EXPLAIN
ANALYZE which involves executing the plan, which is unnecessary.

We report planning and execution times when es->summary is true. It is
set to true when es->analyze is true.
 211 /* currently, summary option is not exposed to users; just set it */
 212 es->summary = es->analyze;

The comment was introduced by commit

commit 90063a7612e2730f7757c2a80ba384bbe7e35c4b
Author: Tom Lane 
Date:   Wed Oct 15 18:50:13 2014 -0400

Print planning time only in EXPLAIN ANALYZE, not plain EXPLAIN.

We've gotten enough push-back on that change to make it clear that it
wasn't an especially good idea to do it like that.  Revert plain EXPLAIN
to its previous behavior, but keep the extra output in EXPLAIN ANALYZE.
Per discussion.

Internally, I set this up as a separate flag ExplainState.summary that
controls printing of planning time and execution time.  For now it's
just copied from the ANALYZE option, but we could consider exposing it
to users.

The discussion referred to seems to be [1]. Here's patch to expose the
"summary" option as mentioned in the last paragraph of above commit
message. Right now I have named it as "summary", but I am fine if we
want to change it to something meaningful. "timing" already has got
some other usage, so can't use it here.

One can use this option as
postgres=# explain (summary on) select * from pg_class c, pg_type t
where c.reltype = t.oid;
QUERY PLAN
--
 Hash Join  (cost=17.12..35.70 rows=319 width=511)
   Hash Cond: (c.reltype = t.oid)
   ->  Seq Scan on pg_class c  (cost=0.00..14.19 rows=319 width=259)
   ->  Hash  (cost=12.61..12.61 rows=361 width=256)
 ->  Seq Scan on pg_type t  (cost=0.00..12.61 rows=361 width=256)
 Planning time: 48.823 ms
(6 rows)

When analyze is specified, summary is also set to ON. By default this
flag is OFF.

Suggestions welcome.

[1] 
https://www.postgresql.org/message-id/flat/1351f76f-69a4-4257-91c2-9382e2a6dc22%40email.android.com#1351f76f-69a4-4257-91c2-9382e2a6d...@email.android.com

[2] https://www.postgresql.org/message-id/19766.1413129321%40sss.pgh.pa.us
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


pg_explain_plan_time.patch
Description: binary/octet-stream

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


[HACKERS] merging some features from plpgsql2 project

2016-12-26 Thread Pavel Stehule
Hi

I reread ideas described on page https://github.com/trustly/plpgsql2

Some points are well and can be benefit for PlpgSQL.

First I describe my initial position. I am strongly against introduction
"new" language - plpgsql2 or new plpgsql, or any else. The trust of
developers to us is important and introduction of any not compatible or
different feature has to have really big reason. PostgreSQL is conservative
environment, and PLpgSQL should not be a exception. More - I have not any
information from my customers, colleagues about missing features in this
language.  If there is some gaps, then it is in outer environment - IDE,
deployment, testing,

I understand so we have to do some break compatibility changes, but the
changes should be smoothly with possibility to simply identify necessary
changes, but better don't do it - and use other possibility.

i lost hope so plpgsql_check can be integrated to core. It is living
outside well - only preparing dll for MSWindows is annoyance. But some
checks from plpgsql_check can be implemented in core as extra_checks, and
some checks from plpgsql2 can be implemented in plpgsql_check.

Points from plpgsql2:
* SELECT .. INTO vs. TOO_MANY_ROWS - can be implemented as extra check
* SELECT .. INTO and the number of result columns - good extra check too

* EXECUTE and FOUND - this is incompatible change, extra check can be used
(test on unset variable). I see solution in leaving FOUND variable and
introduction of some new without this issue - ROW_COUNT maybe (this is
another possible incompatible change, but with higher benefit - maybe we
can introduce some aliasing, PRAGMA clause, default PRAGMAs, ..).

* SELECT .. INTO and := - looks bizarre, but I see clean benefit and I can
accept it

* The OUT namespace and OUT parameter visibility - I don't like it - not in
this form - can we introduce some form of namespace aliasing? The arguments
are in function name named namespace already.

Now, we doesn't use plpgsql_extra_check much and it is pity.

I thing so real question is support some macros, that can help with code
maintenance for different PostgreSQL versions. PostGIS code is nice example
what we are missing.

Regards

Pavel


Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-26 Thread Etsuro Fujita

On 2016/12/22 1:04, Ashutosh Bapat wrote:

Some review comments


Thanks for the review!


2. We should try to look for other not-so-cheap paths if the cheapest one is
paramterized. You might want to use get_cheapest_path_for_pathkeys() to find a
suitable unparameterized path by passing NULL for required_outer and NIL for
pathkeys, that's a very strange usage, but I think it will serve the purpose.
On the thread we discussed that we should save the epq_path create for lower
join and use it here. That may be better than searching for a path.
+/* Give up if the cheapest-total-cost paths are parameterized. */
+if (!bms_is_empty(PATH_REQ_OUTER(outer_path)) ||
+!bms_is_empty(PATH_REQ_OUTER(inner_path)))
+return NULL;


I did that because I think that would work well for postgres_fdw, but I 
agree with you.  Will revise.



3. Talking about saving some CPU cycles - if a clauseless full join can be
implemented only using merge join, probably that's the only path available in
the list of paths for the given relation. Instead of building the same path
anew, should we use the existing path like GetExistingLocalJoinPath() for that
case?


Hm, that might be an idea, but my concern about that is: the existing 
path wouldn't always be guaranteed to be unprameterized.



In fact, I am wondering whether we should look for an existing nestloop
path for all joins except full, in which case we should look for merge or hash
join path. We go on building a new path, if only there isn't an existing one.
That will certainly save some CPU cycles spent in costing the path.


Maybe in many cases, nestloop paths for INNER/LEFT/SEMI/ANTI might be 
removed from the rel's pathlist by dominated merge or hash join paths, 
so searching the pathlist might cause a useless overhead.



4. Following comment mentions only hash join, but the code creates a merge join
or hash join path.
 * If the jointype is JOIN_FULL, try to create a hashjoin join path from


Will revise.


5. Per comment below a clauseless full join can be implemented using only merge
join. Why are we checking extra->mergejoin_allowed? Shouldn't it be true here?
/*
 * Special corner case: for "x FULL JOIN y ON true", there will be no
 * join clauses at all.  Note that mergejoin is our only join type
 * that supports FULL JOIN without any join clauses, and in that case
 * it doesn't require the input paths to be well ordered, so generate
 * a clauseless mergejoin path from the cheapest-total-cost paths.
 */
if (extra->mergejoin_allowed && !extra->mergeclause_list)


See the comments for select_mergejoin_clauses:

 * select_mergejoin_clauses
 *Select mergejoin clauses that are usable for a particular join.
 *Returns a list of RestrictInfo nodes for those clauses.
 *
 * *mergejoin_allowed is normally set to TRUE, but it is set to FALSE if
 * this is a right/full join and there are nonmergejoinable join clauses.
 * The executor's mergejoin machinery cannot handle such cases, so we have
 * to avoid generating a mergejoin plan.  (Note that this flag does NOT
 * consider whether there are actually any mergejoinable clauses.  This is
 * correct because in some cases we need to build a clauseless mergejoin.
 * Simply returning NIL is therefore not enough to distinguish safe from
 * unsafe cases.)


Rethinking about the problem, the error comes because the inner or outer plan
of a merge join plan doesn't have pathkeys required by the merge join. This
happens because the merge join path had foreign path with pathkeys as inner or
outer path and corresponding fdw_outerpath didn't have those pathkeys. I am
wondering whether the easy and possibly correct solution here is to not replace
a ForeignPath with fdw_outerpath in GetExistingLocalJoinPath()? If we don't do
that, there won't be error building merge join plan and
postgresRecheckForeignScan() would correctly route the EPQ checks to the local
plan available as outer plan. Attached patch with that code removed.


That might be fine for PG9.6, but I'd like to propose replacing 
GetExistingLocalJoinPath with CreateLocalJoinPath for PG10, because (1) 
GetExistingLocalJoinPath might choose an overkill, merge or hash join 
path for INNER/LEFT/SEMI/ANTI, not a nestloop join path, which might 
cause an overhead at EPQ rechecks, and (2) choosing a local join path 
randomly from the rel's pathlist wouldn't be easy to understand.


I'll add this to the next CF.

Best regards,
Etsuro Fujita




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


Re: [HACKERS] Potential data loss of 2PC files

2016-12-26 Thread Michael Paquier
On Fri, Dec 23, 2016 at 3:02 AM, Andres Freund  wrote:
> Not quite IIRC: that doesn't deal with file size increase.  All this would be 
> easier if hardlinks wouldn't exist IIUC. It's basically a question whether 
> dentry, inode or contents need to be synced.   Yes, it sucks.

I did more monitoring of the code... Looking at unlogged tables and
empty() routines of access methods, isn't there a risk as well for
unlogged tables? mdimmedsync() does not fsync() the parent directory
either! It seems to me that we want to fsync() the parent folder in
this code path especially and not just at checkpoint as this assumes
that it does not care about shared buffers. We could do that at
checkpoint as well, actually, by looping through all the tablespaces
and fsync the database directories.

Robert, as the former author of unlogged tables and Andres, as you
have done a lot of work on durability, could you share your opinion on
the matter? Of course opinions of others are welcome!
-- 
Michael


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


Re: [HACKERS] Speedup twophase transactions

2016-12-26 Thread Michael Paquier
On Tue, Dec 27, 2016 at 12:59 PM, Stas Kelvich  wrote:
> Standard config with increased shared_buffers. I think the most significant
> impact on the recovery speed here is on the client side, namely time between
> prepare and commit. Right now I’m using pgbench script that issues commit
> right after prepare. It’s also possible to put sleep between prepare and
> commit
> and increase number of connections to thousands. That will be probably the
> worst case — majority of prepared tx will be moved to files.

I think that it would be a good idea to actually test that in pure
recovery time, aka no client, and just use a base backup and make it
recover X prepared transactions that have created Y checkpoints after
dropping cache (or restarting server).
-- 
Michael


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


Re: [HACKERS] Speedup twophase transactions

2016-12-26 Thread Stas Kelvich

> On 22 Dec 2016, at 05:35, Michael Paquier  wrote:
> 
> True. The more spread the checkpoints and 2PC files, the more risk to
> require access to disk. Memory's cheap anyway. What was the system
> memory? How many checkpoints did you trigger for how many 2PC files
> created?

Standard config with increased shared_buffers. I think the most significant
impact on the recovery speed here is on the client side, namely time between
prepare and commit. Right now I’m using pgbench script that issues commit
right after prepare. It’s also possible to put sleep between prepare and commit
and increase number of connections to thousands. That will be probably the
worst case — majority of prepared tx will be moved to files.

> Perhaps it would be a good idea to look for the 2PC files
> from WAL records in a specific order. Did you try to use
> dlist_push_head instead of dlist_push_tail? This may make a difference
> on systems where WAL segments don't fit in system cache as the latest
> files generated would be looked at first for 2PC data.

Ouch! Good catch. I didn’t actually noticed that list populated in opposite 
order
with respect to traversal. I’ll fix that.

> On 27 Dec 2016, at 08:33, Michael Paquier  wrote:
> 
> Stas, have you tested as well tested the impact on recovery time when
> WAL segments are very likely evicted from the OS cache? This could be
> a plausible scenario if a standby instance is heavily used for
> read-only transactions (say pgbench -S), and that the data quantity is
> higher than the amount of RAM available. It would not be complicated
> to test that: just drop_caches before beginning recovery. The maximum
> amount of 2PC transactions that need to have access to the past WAL
> segments is linearly related to the volume of WAL between two
> checkpoints, so max_wal_size does not really matter. What matters is
> the time it takes to recover the same amount of WAL. Increasing
> max_wal_size would give more room to reduce the overall noise between
> two measurements though.

Okay, i’ll perform such testing.

-- 
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-26 Thread Etsuro Fujita

On 2016/12/23 1:04, Robert Haas wrote:

On Wed, Dec 21, 2016 at 11:04 AM, Ashutosh Bapat
 wrote:

Some review comments

1. postgres_fdw doesn't push down semi and anti joins so you may want to
discount these two too.
+   jointype == JOIN_SEMI ||
+   jointype == JOIN_ANTI);



But in the future, it might.


I plan to work on adding those cases to postgres_fdw.


We shouldn't randomly leave foot-guns
lying around if there's an easy alternative.


Some FDWs might have already supported pushing down semi/anti joins.  So 
I think it's better to handle those joins as well.



3. Adding new members to JoinPathExtraData may save some work for postgres_fdw
and other FDWs which would use CreateLocalJoinPath(), but it will add few bytes
to the structure even when there is no "FULL foreign join which requires EPQ"
involved in the query. That may not be so much of memory overhead since the
structure is used locally to add_paths_to_joinrel(), but it might be something
to think about. Instead, what if we call select_mergejoin_clauses() within
CreateLocalJoinPath() to get that information?



I think that's exactly backwards.  The few bytes of storage don't
matter, but extra CPU cycles might.


I agree with Robert.

Best regards,
Etsuro Fujita




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


Re: [HACKERS] postgres_fdw bug in 9.6

2016-12-26 Thread Etsuro Fujita

On 2016/12/21 21:44, Etsuro Fujita wrote:

On 2016/12/20 0:37, Tom Lane wrote:

Etsuro Fujita  writes:

On 2016/12/17 1:13, Tom Lane wrote:

So I think the rule could be



"When first asked to produce a path for a given foreign joinrel,
collect
the cheapest paths for its left and right inputs, and make a
nestloop path
(or hashjoin path, if full join) from those, using the join quals
needed
for the current input relation pair.



Seems reasonable.



Use this as the fdw_outerpath for
all foreign paths made for the joinrel."



I'm not sure that would work well for foreign joins with sort orders.
Consider a merge join, whose left input is a 2-way foreign join with a
sort order that implements a full join and whose right input is a sorted
local table scan.  If the EPQ subplan for the foreign join wouldn't
produce the right sort order, the merge join might break during EPQ
rechecks (note that in this case the EPQ subplan for the foreign join
might produce more than a single row during an EPQ recheck).



How so?  We only recheck one row at a time, therefore it can be
claimed to
have any sort order you care about.



I'll have second thoughts about that.


I noticed I was wrong and you are right.  Sorry for the noise.

Best regards,
Etsuro Fujita




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


[HACKERS] Commit fest 2017-01 will begin soon!

2016-12-26 Thread Michael Paquier
Hi all,

As mentioned a couple of weeks back, I am fine to work as commit fest
manager for 2017-01.

Here is the commit fest status at the moment I am writing this email:
Needs review: 68.
Waiting on Author: 17.
Ready for Committer: 18.
Committed: 27.
Rejected: 2.
Returned with Feedback: 6.
Total: 138.
So there are many of them, though there are not that many new entries
as many patches have been moved from the last CF to this one.

There are still a couple of days to register patches! So if you don't
want your fancy feature to be forgotten, please add it in time to the
CF app. Speaking of which, I am going to have a low bandwidth soon as
that's a period of National Holidays in Japan for the new year, and I
don't think I'll be able to mark the CF as in progress AoE time. So if
somebody could do it for me that would be great :)

Thanks,
-- 
Michael


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


Re: [HACKERS] IF (NOT) EXISTS in psql-completion

2016-12-26 Thread Kyotaro HORIGUCHI
Hello,

At Mon, 26 Dec 2016 14:24:33 +0100, Pavel Stehule  
wrote in 
pavel.stehule> 2016-12-26 9:40 GMT+01:00 Kyotaro HORIGUCHI 
 >:
> 
> > > Thanks for reviewing but I ran out of time for this CF..
> > >
> > > I'm going to move this to the next CF.
> >
> > I splitted the patch into small pieces. f3fd531 conflicted to
> > this so rebased onto the current master HEAD.
> >
> > 0001 is psql_completion refactoring.
> > 0002-0003 are patches prividing new infrastructures.
> > 0004 is README for the infrastructures.
> > 0005 is letter-case correction of SET/RESET/SHOW using 0002.
> > 0006-0008 are improvements of recursive syntaxes using 0001 and 0004.
> > 0009-0016 are simplifying (maybe) completion code per syntax.
> >
> > The last one (0017) is the IF(NOT)EXIST modifications. It
> > suggests if(not)exists for syntaxes already gets object
> > suggestion. So some kind of objects like operator, cast and so
> > don't get an if.. suggestion. Likewise, I intentionally didn't
> > modified siggestions for "TEXT SEARCH *".
> >
> >
> lot of patches. I hope I look on these patches this week.

Thank you for looking this and sorry for the many files. But I
hople that they would be far easier to read.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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


Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

2016-12-26 Thread Michael Paquier
On Thu, Dec 15, 2016 at 8:51 AM, Michael Paquier
 wrote:
> On Thu, Dec 15, 2016 at 1:20 AM, Vladimir Rusinov  wrote:
>>> Personally, I think this is not important, but if you want to do it, I'd
>>> follow the suggestion in the thread to rename all functions and leave
>>> the old names as aliases or wrappers of some kind.
>>
>> I think I agree with Michael Paquier: "Better to do breakages in a single
>> release rather than spreading them across releases.".
>> There's going to be a lot of broken scripts following pg_xlog rename, so I
>> think it makes sense to just drop functions as well.
>
> For consistency that makes sense in my view. But I won't be too noisy
> as well if people think that we should keep aliases for compatibility.

Actually, I am changing my mind on this bit, following Peter's
opinion. Maintaining aliases of the past functions is a no-brainer,
and it has no cost in terms of maintenance. That will save much pain
to many monitoring scripts as well.
-- 
Michael


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


Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

2016-12-26 Thread Kyotaro HORIGUCHI
Hello,

At Tue, 27 Dec 2016 10:28:53 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20161227.102853.204155513.horiguchi.kyot...@lab.ntt.co.jp>
> Putting the two above together, the following is my suggestion
> for the parser part.
> 
> - Make all parse nodes have the following two members at the
>   beginning. This unifies all parse node from the view of setting
>   their location. Commenting about this would be better.
> 
>   | NodeTag   type;
>   | int   location;
> 
> - Remove struct ParseNode and setQueryLocation. stmtmulti sets
>   location in the same manner to other kind of nodes, just doing
>   '= @n'. base_yyparse() doesn't calculate statement length.
> 
> - Make raw_parser caluclate statement length then store it into
>   each stmt scanning the yyextra.parsetree.

An additional comment on parser(planner?) part.

  This patch make planner() copy the location and length from
  parse to result, but copying such stuffs is a job of
  standard_planner.

Then the following is a comment on pg_stat_statements.c

- pg_stat_statements.c:977 - isParseNode(node)
  node should be parsenode.

- The name for the addional parameter query_loc is written as
  query_len in its prototype.

- In pgss_store, "else if (strlen(query)) != query_len)" doesn't
  work as expected because of one-off error. query_len here is
  the length of the query *excluding* the last semicolon.

- qtext_store doesn't rely on terminating nul character and the
  function already takes query length as a parameter. So, there's
  no need to copy the partial debug_query_string into palloc'ed
  memory.  Just replacing the query with query_loc will be
  sufficient.

Have a nice holidays.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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


Re: [HACKERS] gettimeofday is at the end of its usefulness?

2016-12-26 Thread Greg Stark
On Dec 26, 2016 10:35 PM, "Tom Lane"  wrote:


So it seems like the configure support we'd need is to detect
whether clock_gettime is available (note on Linux there's also
a library requirement, -lrt), and we would also need a way to
provide a platform-specific choice of clockid; we at least need
enough smarts to use CLOCK_MONOTONIC_RAW on macOS.


This seems like something that really should be checked at runtime. It's
very specific to the specific kernel you're running on, not the build
environment, and it can hopefully be measured in only a second or even a
fraction of a second. The only Pebblebrook would be if other things running
on the system made the test results unpredictable so that you had a small
chance of getting a very suboptimal choice and we ruling the dice each time
you restarted...


Re: [HACKERS] Speedup twophase transactions

2016-12-26 Thread Michael Paquier
On Thu, Dec 22, 2016 at 7:35 AM, Michael Paquier
 wrote:
> On Wed, Dec 21, 2016 at 10:37 PM, Stas Kelvich  
> wrote:
>> ISTM your reasoning about filesystem cache applies here as well, but just
>> without spending time on file creation.
>
> True. The more spread the checkpoints and 2PC files, the more risk to
> require access to disk. Memory's cheap anyway. What was the system
> memory? How many checkpoints did you trigger for how many 2PC files
> created? Perhaps it would be a good idea to look for the 2PC files
> from WAL records in a specific order. Did you try to use
> dlist_push_head instead of dlist_push_tail? This may make a difference
> on systems where WAL segments don't fit in system cache as the latest
> files generated would be looked at first for 2PC data.

Stas, have you tested as well tested the impact on recovery time when
WAL segments are very likely evicted from the OS cache? This could be
a plausible scenario if a standby instance is heavily used for
read-only transactions (say pgbench -S), and that the data quantity is
higher than the amount of RAM available. It would not be complicated
to test that: just drop_caches before beginning recovery. The maximum
amount of 2PC transactions that need to have access to the past WAL
segments is linearly related to the volume of WAL between two
checkpoints, so max_wal_size does not really matter. What matters is
the time it takes to recover the same amount of WAL. Increasing
max_wal_size would give more room to reduce the overall noise between
two measurements though.
-- 
Michael


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


Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

2016-12-26 Thread Kyotaro HORIGUCHI
Hello,

At Mon, 26 Dec 2016 16:00:57 +0100 (CET), Fabien COELHO  
wrote in 
> 
> Hello Craig,
> 
> > Please put me (ringerc) down as a reviewer.
> 
> Done.
> 
> Please do not loose time reviewing stupid version 1... skip to version
> 2 directly:-)
> 
> Also, note that the query-location part may be considered separately
> from the pg_stat_statements part which uses it.

In nonterminal stmtmulti, setQueryLocation is added and used to
calcualte and store the length of every stmt's. This needs an
extra storage in bse_yy_extra_type and introduces a bit
complicated stuff. But I think raw_parser can do that without
such extra storage and labor, then gram.y gets far simpler.

The struct member to store the location and length is named
'qlocation', and 'qlength' in the added ParseNode. But many nodes
already have 'location' of exactly the same purpopse. I don't see
a necessity to differentiate them.

Putting the two above together, the following is my suggestion
for the parser part.

- Make all parse nodes have the following two members at the
  beginning. This unifies all parse node from the view of setting
  their location. Commenting about this would be better.

  | NodeTag   type;
  | int   location;

- Remove struct ParseNode and setQueryLocation. stmtmulti sets
  location in the same manner to other kind of nodes, just doing
  '= @n'. base_yyparse() doesn't calculate statement length.

- Make raw_parser caluclate statement length then store it into
  each stmt scanning the yyextra.parsetree.

What do you think about this?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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


[HACKERS] Support for pg_receivexlog --format=plain|tar

2016-12-26 Thread Michael Paquier
Hi all,

Since 56c7d8d4, pg_basebackup supports tar format when streaming WAL
records. This has been done by introducing a new transparent routine
layer to control the method used to fetch WAL walmethods.c: plain or
tar.

pg_receivexlog does not make use of that yet, but I think that it
could to allow retention of more WAL history within the same amount of
disk space. OK, disk space is cheap but for some users things like
that matters to define a duration retention policy. Especially when
things are automated around Postgres. I really think that
pg_receivexlog should be able to support an option like
--format=plain|tar. "plain" is the default, and matches the current
behavior. This option is of course designed to match pg_basebackup's
one.

So, here is in details what would happen if --format=tar is done:
- When streaming begins, write changes to a tar stream, named
segnum.tar.partial as long as the segment is not completed.
- Once the segment completes, rename it to segnum.tar.
- each individual segment has its own tarball.
- if pg_receivexlog fails to receive changes in the middle of a
segment, it begins streaming back at the beginning of a segment,
considering that the current .partial segment is corrupted. So if
server comes back online, empty the current .partial file and begin
writing on it again. (I have found a bug on HEAD in this area
actually).

Magnus, you have mentioned me as well that you had a couple of ideas
on the matter, feel free to jump in and let's mix our thoughts!

There are a couple of things that I have been considering as well for
pg_receivexlog. Though they are not directly stick to this thread,
here they are as I don't forget about them:
- Removal of oldest WAL segments on a partition. When writing WAL
segments to a dedicated partition, we could have an option that
automatically removes the oldest WAL segment if the partition is full.
This triggers once a segment is completed.
- Compression of fully-written segments. When a segment is finished
being written, pg_receivexlog could compress them further with gz for
example. With --format=t this leads to segnum.tar.gz being generated.
The advantage of doing those two things in pg_receivexlog is
monitoring. One process to handle them all, and there is no need of
cron jobs to handle any cleanup or compression.

Thanks,
-- 
Michael


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


Re: [HACKERS] Questions regarding signal handler of postmaster

2016-12-26 Thread Tatsuo Ishii
> But we keep signals blocked almost all the time in the postmaster,
> so in reality no signal handler can interrupt anything except the
> select() wait call.  People complain about that coding technique
> all the time, but no one has presented any reason to believe that
> it's broken.

Ok, there seems no better solution than always blocking signals.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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


Re: [HACKERS] Questions regarding signal handler of postmaster

2016-12-26 Thread Tatsuo Ishii
> I encountered that problem with postmaster and fixed it in 9.4.0 (it's not 
> back-patched to earlier releases because it's relatively complex).
> 
> https://www.postgresql.org/message-id/20DAEA8949EC4E2289C6E8E58560DEC0@maumau
> 
> 
> [Excerpt from 9.4 release note]
> During crash recovery or immediate shutdown, send uncatchable termination 
> signals (SIGKILL) to child processes that do not shut down promptly (MauMau, 
> Álvaro Herrera)
> This reduces the likelihood of leaving orphaned child processes behind after 
> postmaster shutdown, as well as ensuring that crash recovery can proceed if 
> some child processes have become “stuck”.

Looks wild to me:-) I hope there exists better way to solve the problem...

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

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


Re: [HACKERS] Questions regarding signal handler of postmaster

2016-12-26 Thread Tsunakawa, Takayuki
From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> In postmaster.c signal handler pmdie() calls ereport() and
> errmsg_internal(), which could call palloc() then malloc() if necessary.
> Because it is possible that pmdie() gets called while
> malloc() gets called in postmaster, I think it is possible that a deadlock
> situation could occur through an internal locking inside malloc(). I have
> not observed the exact case in PostgreSQL but I see a suspected case in
> Pgpool-II. In the stack trace #14, malloc() is called by Pgpool-II. It is
> interrupted by a signal in #11, and the signal handler calls malloc() again,
> and it is stuck at #0.

I encountered that problem with postmaster and fixed it in 9.4.0 (it's not 
back-patched to earlier releases because it's relatively complex).

https://www.postgresql.org/message-id/20DAEA8949EC4E2289C6E8E58560DEC0@maumau


[Excerpt from 9.4 release note]
During crash recovery or immediate shutdown, send uncatchable termination 
signals (SIGKILL) to child processes that do not shut down promptly (MauMau, 
Álvaro Herrera)
This reduces the likelihood of leaving orphaned child processes behind after 
postmaster shutdown, as well as ensuring that crash recovery can proceed if 
some child processes have become “stuck”.

Regards
Takayuki Tsunakawa




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


Re: [HACKERS] Questions regarding signal handler of postmaster

2016-12-26 Thread Tom Lane
Tatsuo Ishii  writes:
> In postmaster.c signal handler pmdie() calls ereport() and
> errmsg_internal(), which could call palloc() then malloc() if
> necessary. Because it is possible that pmdie() gets called while
> malloc() gets called in postmaster, I think it is possible that a
> deadlock situation could occur through an internal locking inside
> malloc().

But we keep signals blocked almost all the time in the postmaster,
so in reality no signal handler can interrupt anything except the
select() wait call.  People complain about that coding technique
all the time, but no one has presented any reason to believe that
it's broken.

regards, tom lane


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


[HACKERS] Questions regarding signal handler of postmaster

2016-12-26 Thread Tatsuo Ishii
In postmaster.c signal handler pmdie() calls ereport() and
errmsg_internal(), which could call palloc() then malloc() if
necessary. Because it is possible that pmdie() gets called while
malloc() gets called in postmaster, I think it is possible that a
deadlock situation could occur through an internal locking inside
malloc(). I have not observed the exact case in PostgreSQL but I see a
suspected case in Pgpool-II. In the stack trace #14, malloc() is
called by Pgpool-II. It is interrupted by a signal in #11, and the
signal handler calls malloc() again, and it is stuck at #0.

So my question is, is my concern about PostgreSQL valid?
If so, how can we fix it?

#0  __lll_lock_wait_private () at 
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:95
#1  0x7f67fe20ccba in _L_lock_12808 () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x7f67fe20a6b5 in __GI___libc_malloc (bytes=15) at malloc.c:2887
#3  0x7f67fe21072a in __GI___strdup (s=0x7f67fe305dd8 "/etc/localtime") at 
strdup.c:42
#4  0x7f67fe239f51 in tzset_internal (always=, 
explicit=explicit@entry=1)
at tzset.c:444
#5  0x7f67fe23a913 in __tz_convert (timer=timer@entry=0x7ffce1c1b7f8, 
use_localtime=use_localtime@entry=1, tp=tp@entry=0x7f67fe54bde0 <_tmbuf>) 
at tzset.c:632
#6  0x7f67fe2387d1 in __GI_localtime (t=t@entry=0x7ffce1c1b7f8) at 
localtime.c:42
#7  0x0045627b in log_line_prefix (buf=buf@entry=0x7ffce1c1b8d0, 
line_prefix=, 
edata=) at ../../src/utils/error/elog.c:2059
#8  0x0045894d in send_message_to_server_log (edata=0x753320 
)
at ../../src/utils/error/elog.c:2084
#9  EmitErrorReport () at ../../src/utils/error/elog.c:1129
#10 0x00456d8e in errfinish (dummy=) at 
../../src/utils/error/elog.c:434
#11 0x00421f57 in die (sig=2) at protocol/child.c:925
#12 
#13 _int_malloc (av=0x7f67fe546760 , bytes=4176) at malloc.c:3302
#14 0x7f67fe20a6c0 in __GI___libc_malloc (bytes=4176) at malloc.c:2891

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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


Re: [HACKERS] gettimeofday is at the end of its usefulness?

2016-12-26 Thread Tom Lane
Haribabu Kommi  writes:
> Attached a patch that replaces most of the getimeofday function calls,
> except timeofday(user callable) and GetCurrentTimestamp functions.
> Didn't add any configure checks in case if the clock_gettime function is
> not available, the fallback logic to gettimeofday function call.

Well, of course, configure is the hard part.

I got interested in this area again pursuant to a question from Joel
Jacobson, and looked around to see if things had changed any since
2014.  One pleasant surprise is that Apple got around to implementing
clock_gettime() as of the current macOS release (10.12 "Sierra").
That means that pretty much all interesting platforms now have
clock_gettime(), which removes one possible objection to starting to
use it.  However, it seems like there is not a lot of commonality
to the best "clockid" to use.

In theory, according to the POSIX spec, CLOCK_MONOTONIC would be what we
want to use for time interval measurement (EXPLAIN ANALYZE), since that
would be impervious to possible clock setting changes.  But some
implementations might only optimize the more common CLOCK_REALTIME,
and I found that there are a lot of platform-specific clock IDs that
we might want to consider.

On Linux (RHEL6, 2.4GHz x86_64), I find that gettimeofday(),
clock_gettime(CLOCK_MONOTONIC), and clock_gettime(CLOCK_REALTIME)
all take about 40ns.  Of course gettimeofday() only has 1us resolution,
but the other two have perhaps 10ns resolution (I get no duplicate
readings in a tight loop).  Other documented clockids include
CLOCK_REALTIME_COARSE: about 10ns to read, but only 1ms resolution
CLOCK_MONOTONIC_COARSE: about 12ns to read, but only 1ms resolution
CLOCK_MONOTONIC_RAW: full resolution but very slow, ~145ns to read
So CLOCK_MONOTONIC seems to be the thing to use here.  It won't buy
us anything speed-wise but the extra resolution will be nice.
However, we need to do more research to see if this holds true on
other popular distros.

On macOS (10.12.2, 2.7GHz x86_64), clock_gettime(CLOCK_REALTIME)
is actually a shade faster than gettimeofday: 40ns versus 46ns.
But it's only giving 1us resolution, no better than gettimeofday.
CLOCK_MONOTONIC is also 1us and it takes 75ns to read.  But there's
a CLOCK_MONOTONIC_RAW that takes 44ns to read and seems to offer
full precision -- no duplicate readings in a tight loop.  There's
also CLOCK_MONOTONIC_RAW_APPROX which can be read in 23ns but
the resolution is only around half an ms.

I also tried FreeBSD 11.0 on another Mac (2.3GHz x86_64),
and found that gettimeofday as well as basically all their
clock_gettime variants run in 27 to 28 ns; and clock_gettime
reliably delivers full precision, except for CLOCK_SECOND which
is intentionally truncated to 1s precision.  So there would be
no need to work with anything but CLOCK_MONOTONIC here.

However, it seems that these impressive results date back only to
June 2012, cf
https://github.com/freebsd/freebsd/commit/13a9f42818f6b89a72b3e40923be809b490400d8
and at least as of that commit, only x86 and x86_64 had the fast
clock_gettime code.  Older FreeBSD, or FreeBSD on another architecture,
is likely to be a lot worse.  But I lack an installation to try.

I also tried OpenBSD 6.0 on that same Mac, and got pretty horrid
results: gettimeofday, CLOCK_REALTIME, and CLOCK_MONOTONIC all
take about 613ns to read.  Ouch.  And so does time(NULL); double
ouch.  Evidently there's no optimization on this platform and
what we're seeing is the minimum cost for a kernel call.  Still,
we do get better precision from clock_gettime than gettimeofday,
so we might as well switch.

So it seems like the configure support we'd need is to detect
whether clock_gettime is available (note on Linux there's also
a library requirement, -lrt), and we would also need a way to
provide a platform-specific choice of clockid; we at least need
enough smarts to use CLOCK_MONOTONIC_RAW on macOS.

regards, tom lane


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


Re: [HACKERS] [PATCH] Generic type subscription

2016-12-26 Thread Artur Zakirov
2016-12-26 18:49 GMT+03:00 Dmitry Dolgov <9erthali...@gmail.com>:
>> On 5 December 2016 at 12:03, Haribabu Kommi 
>> wrote:
>
>> Moved to next CF with "needs review" status.
>
> Looks like we stuck here little bit. Does anyone else have any
> suggestions/improvements, or this patch is in good enough shape?

Would you rebase the patch, please? It seems it is necessary. It can't
be applied now.

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


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


Re: [HACKERS] pg_stat_activity.waiting_start

2016-12-26 Thread Tom Lane
Joel Jacobson  writes:
> Maybe a good tradeoff then would be to let "wait_start" represent the
> very first time the txn started waiting?
> ...
> As long as the documentation is clear on "wait_start" meaning when the
> first wait started in the txn, I think that's useful enough to improve
> the situation, as one could then ask a query like "select all
> processes that have possibly been waiting for at least 5 seconds",
> which you cannot do today.

Meh.  You *can* do that now: query pg_stat_activity for waiting processes,
wait a couple seconds, query again, intersect the results.  I think really
the only useful improvement on that would be to be able to tell that the
process has been blocked continuously for X seconds, and what you're
proposing here won't do that.

In practice, there should never be waits on LWLocks (much less spinlocks)
that exceed order-of-milliseconds; if there are, either we chose the wrong
lock type or the system is pretty broken in general.  So maybe it's
sufficient if we provide a wait start time for heavyweight locks ...
though that still seems kind of ugly.  (Also, I don't recall the existing
code factorization there, but getting the start time into pg_stat_activity
without an extra gettimeofday call might be hard.  As I said, there is
one being done, but I'm not sure how accessible its result is.)

I did a bit more research over the weekend into the cost of gettimeofday
and clock_gettime, and have some new results that I'll post into that
thread shortly.  But the short answer is still "they're damn expensive
on some platforms, and not really cheap anywhere".

regards, tom lane


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


Re: [HACKERS] Incautious handling of overlength identifiers

2016-12-26 Thread Tom Lane
Alvaro Herrera  writes:
> I also admit it didn't occur to me to test the function(s) against
> overlength input when developing it.  I wouldn't object to adding code
> to deal with overlength identifies, but I'm not really sure I would
> choose truncation over reporting an error.  But whatever it'd be, it'd
> be at that level, not at the lower (hash function) level.

Yeah, I'm now convinced that whatever we do about this, if we do anything,
needs to be at a higher code level.  It's not hashname()'s job to prevent
use of overlength names.  I'll go remove the Assert.

regards, tom lane


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


Re: [HACKERS] propose to pushdown qual into EXCEPT

2016-12-26 Thread Tom Lane
I wrote:
> tl;dr: I now think what the patch proposes to do is legit.  There's a heck
> of a lot of work to be done on the comments it's falsified, though.

Hmm, wait a minute.  I mentioned before that what nodeSetOp.c actually
returns is N copies of the same representative tuple.  That in itself
doesn't break the proposed optimization, or at least so I argued ---
but the real question is which representative tuple does it pick?
The answer, as noted in the file header comment, is

 * Note that SetOp does no qual checking nor projection.  The delivered
 * output tuples are just copies of the first-to-arrive tuple in each
 * input group.

In HASHED mode, the first-to-arrive tuple must be from the lefthand
input, which would mean that it's passed the pushed-down qual, so
all is well.  (If no LHS tuples exist in a given group, then EXCEPT
won't output anything, so the fact that it could have collected a
representative tuple from the RHS doesn't matter.)

However, in SORTED mode, I don't see that there's anything particularly
guaranteeing the order in which tuples arrive within a sort group.
If the sort isn't stable, and I don't think all our sorting paths are,
it would be possible to return an RHS tuple as the representative one.

This breaks the proposed optimization because it would become possible
to return a tuple that doesn't pass the pushed-down qual at all.

There are at least two ways this could be dealt with.  We could add the
flag column as a low-order sort column so that it's still guaranteed that
LHS tuples arrive before RHS ones within a group.  (This'd complicate
matters in generate_nonunion_path because now the sort keys would be
different from the setop node's own grouping keys, but it's certainly
possible.)  Or we could fix it at runtime by complicating
setop_retrieve_direct so that it replaces the representative tuple
with the first LHS tuple when that arrives.  Either way, though,
more work is needed than just hacking the qual pushdown logic.

regards, tom lane


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


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
2016-12-26 19:13 GMT+01:00 Pavel Stehule :

>
>
> 2016-12-26 18:20 GMT+01:00 Fabien COELHO :
>
>>
>>
>>
>> And how would it interact with some "fully persistent/shared" variables?
>>
>>
>>
>> I have not any use case for this. I don't know about any similar feature
>> in other database systems. Oracle uses dbms_pipe or dbms_alert for
>> interprocess communication.
>>
>> I am thinking so it is possible to implement. If it is not ACID, then it
>> can work as custom statistic counters. If it should be ACID? Then is better
>> to use table. What I know, now is preferred share nothing design in
>> parallel systems - so for me, it looks like little bit dangerous feature -
>> and I see only one use case - custom statistics - where possible race
>> condition is not hard issue.
>>
>> But I don't plan to implement it in first stage. There should be strong
>> use case for implementing any complex feature in shared memory. But any
>> implementation doesn't breaking to implement it in feature.
>>
>
for custom statistic some extension based on bg worker can be better -
sharing variables are risk of race conditions - and developers are people
only.

I have not a clean opinion about this - the implementation should not be
hard, but I am not sure if this gun I would to give to users.

Regards

Pavel


>
>> Regards
>>
>> Pavel
>>
>>
>> --
>> Fabien.
>
>
>


Re: [HACKERS] Incautious handling of overlength identifiers

2016-12-26 Thread Alvaro Herrera
Tom Lane wrote:
> I wrote:
> > Another idea worth considering is to just make the low-level functions
> > do truncation ...
> 
> After further thought, there's a bigger-picture issue here, which
> is whether the inputs to the SQL functions in question are intended to
> be raw user input --- in which case, not only would truncation be
> an appropriate service, but probably so would downcasing --- or whether
> they are more likely to be coming from a catalog scan, in which case
> you don't want any of that stuff.  Nobody's going to be happy if we
> start making them add quote_ident() around references to name columns.
> I think the privilege-inquiry functions are almost certainly mostly
> used in the latter style; there might be more room for debate about,
> say, pg_get_serial_sequence.

I expect that uses of pg_get_object_address() (one of the affected
interfaces) would mostly be through an event trigger or a similar
internal mechanism, that hopefully should appropriately quote names and
not produce anything overlength.  At least, pg_identify_object() (which
is what I mostly had in mind) complies.  I think removing the assert is
a good enough solution, myself.

I also admit it didn't occur to me to test the function(s) against
overlength input when developing it.  I wouldn't object to adding code
to deal with overlength identifies, but I'm not really sure I would
choose truncation over reporting an error.  But whatever it'd be, it'd
be at that level, not at the lower (hash function) level.

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


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


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
2016-12-26 18:20 GMT+01:00 Fabien COELHO :

>
> Hello Pavel,
>
> I don't understand to "expensive" word.
>>
>
> Hmmm...
>
> How much often you create/drop these variables?
>>
>
> Hmmm... As for "session variables" à la MY/MS-SQL, ISTM that a variable is
> "created" each time it is asked for, and it disappears completely at the
> end of the session... So you can have some kind of minimal SQL scripting
> with variables executed server-side, without resorting to a PL. Also useful
> in interactive mode, although there the performance is less an issue.
>
> The deployment cycle is similar to functions.  I don't
>> propose any feature, that can enforce bloating of system catalogue.
>>
>
> Hmmm
>
> The variable metadata will be persistent across sessions. I can imagine the
>> local (temporal) session variable where life cycle of metadata will be
>> related to session like our current temporary tables. But it is not in
>> this
>> proposal.
>>
>
> So it seems that I misunderstood a key detail in your proposal. Please
> accept my apology for my slow witedness. It is better to discuss in front
> of a white board...
>

sure, mainly with my language skills


>
> Now I understand that you want to create a kind of "persistant" session
> variable... that is at each new session the variable is instancianted in
> the session "automatically", whether the session will use it or not... Or
> is it instanciated on demand, i.e. when explicitely accessed?
>

I am starting with simple default solution - variable is initialized on
demand (when it is used first time in session). Attention: in my concept -
initialization and creation are different things.

In future initialization can be joined with login - we can have a autologin
function (trigger), or we can have a login initialized variables (and if
default expr is function call, then these functions can be used as
autologin functions).



>
> Could you confirm the interpretation?  ISTM that "on demand" would be
> better.
>

Now, what I propose, and what is implemented in prototype is "on demand"


>
> As you gathered I was understanding that the "CREATE VARIABLE & GRANTS"
> were to be issued in each session for each variable used, inducing catalog
> changes each time, hence my quite heavy ranting...
>
> What I understand now is still a somehow strange object, but nevertheless
> the CREATE, DROP, GRANT, REVOKE at least are more justified because then
> object is somehow really persistent in the database, even if not with a
> value.
>

metadata are persistent like functions, sequences - the value is related to
session, only value.


> So I'll have to think about it...
>
> A few more questions:
>
> Bar the grantability, why wouldn't simple session variables work for this
> purpose? That is what is the added value of having them "declared"
> permanently, compared to created on demand, if the creation is really light
> weight?
>

the rights should be persistent, and should be attached to some persistent
object. Hypothetically, we can introduce new kind of objects, but it
disallow future usage of direct DML and SELECT statements.


>
> ISTM that in the Oracle package version, they are linked to PL/SQL, they
> are not part of SQL layer itself, so maybe they are only created when some
> PL/SQL from the package is invoked, and not created otherwise?
>

PL/SQL is different creature - it is living outside SQL catalogue - in
packages. I would not to reimplemented it for PL/pgSQL from following
reasons: we have schemas (that can be used as Oracle packages), we have a
extensions (that can be used partially as Oracle's packages), we have a mix
PL languages - more time I mixed PLpgSQL and PLPerlu. So mapping 1:1 from
Oracle is not good for Postgres.


>
> How would this feature interact with a kind of non persistent "simple"
> session variables that are found in MY/MS/Oracle SQL? One of my concern is
> that such a feature should not preclude other kind of session variables.
>

depends .. In my terminology your proposal is "untrusted temporary local
session variables" - it can share 50% of code - more if implementation will
be based on getter/setter function, less if it will be based on gram
implementation.

These variables should not be declared explicitly - it can be declared
implicitly by setting. They should not use any schema - bat can use
getter/setter functions

so you can write

select setvar('@var1', 10);
select getvar('@var1')

I little bit afraid of direct using the variables in query - inside special
functions we (and users) have control to choose volatility: direct using
can do some unexpected behave for users.


> And how would it interact with some "fully persistent/shared" variables?
>

I have not any use case for this. I don't know about any similar feature in
other database systems. Oracle uses dbms_pipe or dbms_alert for
interprocess communication.

I am thinking so it is possible to implement. If it is not ACID, then it
can work as custom statistic 

Re: [HACKERS] Incautious handling of overlength identifiers

2016-12-26 Thread Tom Lane
I wrote:
> Another idea worth considering is to just make the low-level functions
> do truncation ...

After further thought, there's a bigger-picture issue here, which
is whether the inputs to the SQL functions in question are intended to
be raw user input --- in which case, not only would truncation be
an appropriate service, but probably so would downcasing --- or whether
they are more likely to be coming from a catalog scan, in which case
you don't want any of that stuff.  Nobody's going to be happy if we
start making them add quote_ident() around references to name columns.
I think the privilege-inquiry functions are almost certainly mostly
used in the latter style; there might be more room for debate about,
say, pg_get_serial_sequence.

Since the low-level functions need to support both use cases, asking
them to handle truncation is wrong, just as much as it would be to
ask them to do downcasing.

If we take these SQL functions as being meant for use with inputs
coming from catalogs, then they don't need to do truncation for
user-friendliness purposes; it's perfectly fine to treat overlength
inputs as "name not found" cases.  So that says we could just remove
that Assert and decide we're done.

regards, tom lane


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


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Fabien COELHO


Hello Pavel,


I don't understand to "expensive" word.


Hmmm...


How much often you create/drop these variables?


Hmmm... As for "session variables" à la MY/MS-SQL, ISTM that a variable is 
"created" each time it is asked for, and it disappears completely at the 
end of the session... So you can have some kind of minimal SQL scripting 
with variables executed server-side, without resorting to a PL. Also 
useful in interactive mode, although there the performance is less an 
issue.



The deployment cycle is similar to functions.  I don't
propose any feature, that can enforce bloating of system catalogue.


Hmmm


The variable metadata will be persistent across sessions. I can imagine the
local (temporal) session variable where life cycle of metadata will be
related to session like our current temporary tables. But it is not in this
proposal.


So it seems that I misunderstood a key detail in your proposal. Please 
accept my apology for my slow witedness. It is better to discuss in front 
of a white board...


Now I understand that you want to create a kind of "persistant" session 
variable... that is at each new session the variable is instancianted in 
the session "automatically", whether the session will use it or not... Or 
is it instanciated on demand, i.e. when explicitely accessed?


Could you confirm the interpretation?  ISTM that "on demand" would be 
better.


As you gathered I was understanding that the "CREATE VARIABLE & GRANTS" 
were to be issued in each session for each variable used, inducing catalog 
changes each time, hence my quite heavy ranting...


What I understand now is still a somehow strange object, but nevertheless 
the CREATE, DROP, GRANT, REVOKE at least are more justified because then 
object is somehow really persistent in the database, even if not with a 
value.


So I'll have to think about it...

A few more questions:

Bar the grantability, why wouldn't simple session variables work for this 
purpose? That is what is the added value of having them "declared" 
permanently, compared to created on demand, if the creation is really 
light weight?


ISTM that in the Oracle package version, they are linked to PL/SQL, they 
are not part of SQL layer itself, so maybe they are only created when some 
PL/SQL from the package is invoked, and not created otherwise?


How would this feature interact with a kind of non persistent "simple" 
session variables that are found in MY/MS/Oracle SQL? One of my concern is 
that such a feature should not preclude other kind of session variables.


And how would it interact with some "fully persistent/shared" variables?

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


Re: [HACKERS] Incautious handling of overlength identifiers

2016-12-26 Thread Tom Lane
Michael Paquier  writes:
> On Sat, Dec 24, 2016 at 7:44 AM, Joe Conway  wrote:
>> On 12/23/2016 12:44 PM, Tom Lane wrote:
>>> An alternative worth considering, especially for the back branches,
>>> is simply to remove the Assert in hashname().  That would give us
>>> the behavior that non-developers see anyway, which is that these
>>> functions always fail to match overlength names, whether or not
>>> the names would have matched after truncation.  Trying to apply
>>> truncation more consistently could be left as an improvement
>>> project for later.

>> That sounds reasonable to me.

> +1 for just removing the assertion on back-branches. On HEAD, it seems
> right to me to keep the assertion. However it is not possible to just
> switch those routines from text to name as a table could be defined
> with its schema name. So at minimum this would require adjusting
> textToQualifiedNameList() & similar routines in charge of putting in
> shape the name lists.

textToQualifiedNameList() already does truncate, and I suspect everyplace
that deals in qualified names does as well.  The problem places are those
where someone just took a text parameter, did text_to_cstring on it, and
started treating the result as a Name (e.g. by passing it to a catalog
lookup function).  Since we've intentionally allowed C strings to be used
as Names in most places, it's not even immediately obvious that this is
wrong.

I'm currently inclined to think that removing the assertion from hashname()
is right even for HEAD, because it's not very relevant to the operation
of that routine (you'll get back some hash value even for overlength
strings), and because it's a pretty useless way of enforcing truncation.
It basically will only catch you if you try to do a syscache lookup to
resolve a name --- if you do a catalog heap or index scan, those paths
contain no such gotcha.  And I'm disinclined to introduce one.

The real problem with trying to enforce this through length assertions
in low-level routines is that they'll only reveal a bug if you actually
happen to test the appropriate calling code path with an overlength name.
We've obviously failed to do that in the past and I have little faith that
we'd do it in the future.

So that's why I was thinking about whether we could do this through some
datatype-based approach, whereby we could hope to catch incorrect coding
reliably through compiler checks.  But given our history of allowing C
strings as names, I'm afraid that any such change would be enormously
invasive and not worth the trouble.

Another idea worth considering is to just make the low-level functions
do truncation, ie the fix in hashname would look more like

-   Assert(keylen < NAMEDATALEN);
+   if (keylen >= NAMEDATALEN)
+   keylen = pg_mbcliplen(key, keylen, NAMEDATALEN - 1);

and we'd need something similar in the name comparison functions.
But that would be slightly annoying from a performance standpoint.
Not so much the extra pg_mbcliplen calls, because we could assume
those wouldn't happen in any performance-interesting cases; but
there are no strlen calls in the name comparison functions right now,
so we'd have to add some, and those would get hit every time through.

regards, tom lane


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


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
2016-12-26 17:33 GMT+01:00 Fabien COELHO :

>
> please, can send link?
>>
>
> My badly interpreted PL/SQL example was on the same page you point to
> below:
>
> so some better documentation
>> https://docs.oracle.com/cd/E11882_01/appdev.112/e25519/packa
>> ges.htm#LNPLS99926
>>
>
> There is a private 'number_hired' which given its name I thought was
> counting the number of employee, but it was just counting the number of
> "hire_employee" calls in the current session... Not very interesting.
>
> I am sure, so package variables are not shared between sessions/backends
>>
>
> Indeed, I misinterpreted the Oracle documentation example.
>
>
> [ grantable function example to access a private session variable... ]
>>>
>>
>> I am sorry, it is not secure. Theoretically it can work if you have
>> granted order of function calls, but if not?
>>
>
> I'm not sure I understand.
>
> If you do not grant/revoke permissions as you want on the functions, then
> it can be invoked by anybody.
>
> My point is that it is *possible* to tune permissions so as to control
> exactly who may access a private session variable.
>
> That is exactly the same with a grantable session variable if you do not
> have done the necessary grant/revoke, there is no difference?
>

If you use pattern DECLARE IF NOT EXISTS, you cannot be sure so some other
did it. It is working only if you create variables in session as first.

Only if object is fixed in schema, then object is trustworthy - because you
have to have rights to modify schema. In my proposal only trustworthy user
can create the variable in some schema. Not trustworthy user can use public
schema, or we can support temporary objects (similar to your proposal) in
hypothetical schema "private". I have strong tools in Postgres for
enforcing security, and I would to use it.

Regards

Pavel


>
> --
> Fabien.
>


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Fabien COELHO



please, can send link?


My badly interpreted PL/SQL example was on the same page you point to 
below:



so some better documentation
https://docs.oracle.com/cd/E11882_01/appdev.112/e25519/packages.htm#LNPLS99926


There is a private 'number_hired' which given its name I thought was 
counting the number of employee, but it was just counting the number of 
"hire_employee" calls in the current session... Not very interesting.



I am sure, so package variables are not shared between sessions/backends


Indeed, I misinterpreted the Oracle documentation example.



[ grantable function example to access a private session variable... ]


I am sorry, it is not secure. Theoretically it can work if you have 
granted order of function calls, but if not?


I'm not sure I understand.

If you do not grant/revoke permissions as you want on the functions, then 
it can be invoked by anybody.


My point is that it is *possible* to tune permissions so as to control 
exactly who may access a private session variable.


That is exactly the same with a grantable session variable if you do not 
have done the necessary grant/revoke, there is no difference?


--
Fabien.


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


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
2016-12-26 17:15 GMT+01:00 Fabien COELHO :

>
> Hello Pavel,
>
>   SET ROLE Admin;
>>>   DECLARE @secure_variable INTEGER RESTRICT; -- only accessible to Admin
>>>
>>
> Why introduce another security system?
>>
>
> That is a good question.
>
> I would prefer to avoid it and just have simple session variables... but
> this is not what you want, so I'm trying to find a compromise which both
> gives you the feature you are seeking and would keep session variables as
> inexpensive, i.e. without catalog costs.
>
> A simplistic PUBLIC/PRIVATE permissions on simple session variable can be
> done with nothing (no additional data structures): (store: hash_put( id if private or nothing> || '@varname', value); consult: if exists( id> || '@varname') then return it else if exists('@varname') then return it
> else "error variable does not exist").
>
> Now if you can offer an inexpensive GRANT/REVOKE on simple session
> variables, i.e. without catalog changes, then I think I would agree to it,
> even if I would not like it much.
>

> The reason I "do not like much" is subjective. I associate SQL permission
> commands (GRANT, REVOKE...) to real SQL "objects" (i.e. persistent,
> transactional, secured things subject to CREATE ALTER DROP...). However
> light-weight session variables are not really like that.
>
> Also if you can have inexpensive GRANT/REVOKE then probably I would also
> have to accept "CREATE SESSION VARIABLE @foo", because it would be
> consistent to have it with GRANT/REVOKE. I would "not like it much" either
> to have CREATE for an non persistant object, but that is life...
>
> However I understood that for permissions you do need "pg_class", which
> means catalog changes on session variable creation, which means expensive
> for simple session variables, so not desirable.
>

I don't understand to "expensive" word. How much often you create/drop
these variables? The deployment cycle is similar to functions.  I don't
propose any feature, that can enforce bloating of system catalogue.

The variable metadata will be persistent across sessions. I can imagine the
local (temporal) session variable where life cycle of metadata will be
related to session like our current temporary tables. But it is not in this
proposal.

Regards

Pavel




> --
> Fabien.
>


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Fabien COELHO


Hello Pavel,


  SET ROLE Admin;
  DECLARE @secure_variable INTEGER RESTRICT; -- only accessible to Admin



Why introduce another security system?


That is a good question.

I would prefer to avoid it and just have simple session variables... but 
this is not what you want, so I'm trying to find a compromise which both 
gives you the feature you are seeking and would keep session variables as 
inexpensive, i.e. without catalog costs.


A simplistic PUBLIC/PRIVATE permissions on simple session variable can be 
done with nothing (no additional data structures): (store: hash_put(id if private or nothing> || '@varname', value); consult: if exists(id> || '@varname') then return it else if exists('@varname') then return 
it else "error variable does not exist").


Now if you can offer an inexpensive GRANT/REVOKE on simple session 
variables, i.e. without catalog changes, then I think I would agree to it, 
even if I would not like it much.


The reason I "do not like much" is subjective. I associate SQL permission 
commands (GRANT, REVOKE...) to real SQL "objects" (i.e. persistent, 
transactional, secured things subject to CREATE ALTER DROP...). However 
light-weight session variables are not really like that.


Also if you can have inexpensive GRANT/REVOKE then probably I would also 
have to accept "CREATE SESSION VARIABLE @foo", because it would be 
consistent to have it with GRANT/REVOKE. I would "not like it much" either 
to have CREATE for an non persistant object, but that is life...


However I understood that for permissions you do need "pg_class", which 
means catalog changes on session variable creation, which means expensive 
for simple session variables, so not desirable.


--
Fabien.


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


Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

2016-12-26 Thread Alvaro Herrera
Alvaro Herrera wrote:
> Jaime Casanova wrote:
> 
> > * The isolation test for partial_index fails (attached the regression.diffs)
> 
> Hmm, I had a very similar (if not identical) failure with indirect
> indexes; in my case it was a bug in RelationGetIndexAttrBitmap() -- I
> was missing to have HOT considerate the columns in index predicate, that
> is, the second pull_varattnos() call.

Sorry, I meant:

  Hmm, I had a very similar (if not identical) failure with indirect
  indexes; in my case it was a bug in RelationGetIndexAttrBitmap() -- I
  was missing to have HOT [take into account] the columns in index predicate, 
that
  is, the second pull_varattnos() call.

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


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


Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

2016-12-26 Thread Alvaro Herrera
Jaime Casanova wrote:

> * The isolation test for partial_index fails (attached the regression.diffs)

Hmm, I had a very similar (if not identical) failure with indirect
indexes; in my case it was a bug in RelationGetIndexAttrBitmap() -- I
was missing to have HOT considerate the columns in index predicate, that
is, the second pull_varattnos() call.

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


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


Re: [HACKERS] [PATCH] Generic type subscription

2016-12-26 Thread Dmitry Dolgov
> On 5 December 2016 at 12:03, Haribabu Kommi 
 wrote:

> > Moved to next CF with "needs review" status.

Looks like we stuck here little bit. Does anyone else have any
suggestions/improvements, or this patch is in good enough shape?


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
2016-12-26 15:53 GMT+01:00 Fabien COELHO :

>
> Hello Pavel,
>
> AFAICS they are shared between backends, [...] They constitute a
>>> consistent design.
>>>
>>
>> no
>> http://stackoverflow.com/questions/2383061/scope-of-oracle-
>> package-level-variables
>>
>
> If stackoverflow says so, too bad for me:-) Now I do not understand the
> point of the example I read on Oracle documentation: why having an employee
> count accessed by some functions if it is reset on each new session?
>

please, can send link?

so some better documentation
https://docs.oracle.com/cd/E11882_01/appdev.112/e25519/packages.htm#LNPLS99926

I am sure, so package variables are not shared between sessions/backends -
bacause Oracle uses different mechanism for interprocess communication -
wrote it in Orafce

"When a session references a package item, Oracle Database instantiates the
package for that session. Every session that references a package has its
own instantiation of that package."




>
> So I do retract "it constitute a consistent design". It looks more like a
> PL/SQL confined hack.
>
> Note that Oracle also seems to have session variables with set with DEFINE
> and referenced with 
>
> [...] That could look like:
>>>
>>>   SET ROLE Admin;
>>>   DECLARE @secure_variable INTEGER RESTRICT; -- only accessible to Admin
>>>   SET @secure_variable = 3;
>>>
>>>   SET ROLE BasicUser;
>>>   SELECT @secure_variable; -- say NULL or error does not exist...
>>>
>>> what will be if BasicUser does DECLARE @secure_variable
>>
>
> Then there would be a distinct global @secure_variable unrelated to the
> previous one, that would be hidden from Admin who would see its own private
> @secure_variable. Maybe "restrict" is not the right word, though, let us
> use "private".
>
>SET ROLE User1;
>-- use @var: does not exist in scope error
>DECLARE @var INTEGER PRIVATE;
>SET @var = 1;
>-- use @var: get 1
>
>SET ROLE User2;
>-- use @var: does not exist in scope error
>DECLARE @var INTEGER PUBLIC;
>SET @var = 2;
>-- use @var; get 2
>
>SET ROLE User1;
>-- use @var: get 1 (private version)
>
>SET ROLE User3;
>-- use @var: get 2 (public version created by User2).
>
>
> There are not any granularity of rights - you cannot to grant access ...
>>
>
> Indeed, at least directly. With the above version you can just control
> whether everybody or only the owner has access.
>
> However with some minimal more effort the owner of a private session
> variable could provide a grantable function for accessing this variable:
> the benefit would be that the function is permanent, i.e. would not need to
> be granted each time the variable is used, it could be done once and for
> all.
>
>   CREATE FUNCTION setSecret(INT) SECURITY DEFINER ... AS $$
> DECLARE IF NOT EXISTS @secret TEXT PRIVATE;
> SET @secret = $1;
>   $$ LANGUAGE SQL;
>

>   CREATE FUNCTION useSecret(TEXT) SECURITY DEFINER TEXT AS $$
>  -- would fail if @secret has not been set yet...
>  SELECT sha256sum(@secret || ':' || $1);
>   $$ LANGUAGE SQL;
>
>   CREATE FUNCTION getSecret() RETURNS TEXT SECURITY DEFINER AS $$
> DECLARE IF NOT EXISTS @secret TEXT PRIVATE;
> SELECT @secret;
>   $$ LANGUAGE SQL;
>
>   -- then
>   REVOKE/GRANT ... ON FUNCTION set/use/getSecret(...);
>
> I am sorry, I don't see benefit in your proposal.
>>
>
> The benefit I see is to have MS/MY-SQL/Oracle like light-weight
> (inexpensive, untransactional) session variables and still a minimal access
> control which might be enough for significant use cases.
>
> If more is really needed, consider the function hack, or maybe some
> one-row table with all the power of grant. Ok, the table solution is more
> heavy weight, but then this is also for a special requirement, and it would
> work as well for persistence.
>
> Probably there will be only one agreement, so there are not agreement
>> between us :(
>>
>
> It seems so. I do believe that I am trying to propose a solution which
> take into account your use case as I understand it (you did not confirm nor
> infirm) which is to store securely but not safely some kind of temporary
> data between different function calls with SECURITY DEFINER within the same
> session.
>
> I'm trying to avoid "special-case" medium-weight (i.e. pg_class-based)
> session variables with permissions, which could preclude MY/MS-SQL/Oracle
> like light-weight session variables which are I think interesting in their
> own right.
>

 I am sorry, it is not secure. Theoretically it can work if you have
granted order of function calls, but if not?

regards

Pavel


>
> --
> Fabien.
>


Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

2016-12-26 Thread Fabien COELHO


Hello Craig,


Please put me (ringerc) down as a reviewer.


Done.

Please do not loose time reviewing stupid version 1... skip to version 2 
directly:-)


Also, note that the query-location part may be considered separately from 
the pg_stat_statements part which uses it.


--
Fabien.


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


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Fabien COELHO


Hello Pavel,

AFAICS they are shared between backends, [...] They constitute a 
consistent design.


no
http://stackoverflow.com/questions/2383061/scope-of-oracle-package-level-variables


If stackoverflow says so, too bad for me:-) Now I do not understand the 
point of the example I read on Oracle documentation: why having an 
employee count accessed by some functions if it is reset on each new 
session?


So I do retract "it constitute a consistent design". It looks more like a 
PL/SQL confined hack.


Note that Oracle also seems to have session variables with set with DEFINE 
and referenced with 



[...] That could look like:

  SET ROLE Admin;
  DECLARE @secure_variable INTEGER RESTRICT; -- only accessible to Admin
  SET @secure_variable = 3;

  SET ROLE BasicUser;
  SELECT @secure_variable; -- say NULL or error does not exist...


what will be if BasicUser does DECLARE @secure_variable


Then there would be a distinct global @secure_variable unrelated to the 
previous one, that would be hidden from Admin who would see its own 
private @secure_variable. Maybe "restrict" is not the right word, though, 
let us use "private".


   SET ROLE User1;
   -- use @var: does not exist in scope error
   DECLARE @var INTEGER PRIVATE;
   SET @var = 1;
   -- use @var: get 1

   SET ROLE User2;
   -- use @var: does not exist in scope error
   DECLARE @var INTEGER PUBLIC;
   SET @var = 2;
   -- use @var; get 2

   SET ROLE User1;
   -- use @var: get 1 (private version)

   SET ROLE User3;
   -- use @var: get 2 (public version created by User2).



There are not any granularity of rights - you cannot to grant access ...


Indeed, at least directly. With the above version you can just control 
whether everybody or only the owner has access.


However with some minimal more effort the owner of a private session 
variable could provide a grantable function for accessing this variable: 
the benefit would be that the function is permanent, i.e. would not need 
to be granted each time the variable is used, it could be done once and 
for all.


  CREATE FUNCTION setSecret(INT) SECURITY DEFINER ... AS $$
DECLARE IF NOT EXISTS @secret TEXT PRIVATE;
SET @secret = $1;
  $$ LANGUAGE SQL;

  CREATE FUNCTION useSecret(TEXT) SECURITY DEFINER TEXT AS $$
 -- would fail if @secret has not been set yet...
 SELECT sha256sum(@secret || ':' || $1);
  $$ LANGUAGE SQL;

  CREATE FUNCTION getSecret() RETURNS TEXT SECURITY DEFINER AS $$
DECLARE IF NOT EXISTS @secret TEXT PRIVATE;
SELECT @secret;
  $$ LANGUAGE SQL;

  -- then
  REVOKE/GRANT ... ON FUNCTION set/use/getSecret(...);


I am sorry, I don't see benefit in your proposal.


The benefit I see is to have MS/MY-SQL/Oracle like light-weight 
(inexpensive, untransactional) session variables and still a minimal 
access control which might be enough for significant use cases.


If more is really needed, consider the function hack, or maybe some 
one-row table with all the power of grant. Ok, the table solution is more 
heavy weight, but then this is also for a special requirement, and it 
would work as well for persistence.


Probably there will be only one agreement, so there are not agreement 
between us :(


It seems so. I do believe that I am trying to propose a solution which 
take into account your use case as I understand it (you did not confirm 
nor infirm) which is to store securely but not safely some kind of 
temporary data between different function calls with SECURITY DEFINER 
within the same session.


I'm trying to avoid "special-case" medium-weight (i.e. pg_class-based) 
session variables with permissions, which could preclude MY/MS-SQL/Oracle 
like light-weight session variables which are I think interesting in their 
own right.


--
Fabien.


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


Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

2016-12-26 Thread Craig Ringer
On 23 Dec. 2016 17:53, "Fabien COELHO"  wrote:


Yes. I'll try to put together a patch and submit it to the next CF.
>

Here it is. I'll add this to the next CF.


Great. Please put me (ringerc) down as a reviewer. I'll get to this as soon
as holidays settle down. It'd be very useful for some things I'm working on
too, and is much better then my early draft of similar functionality.


Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

2016-12-26 Thread Craig Ringer
On 21 Dec. 2016 11:44, "Robert Haas"  wrote:

On Tue, Dec 20, 2016 at 6:18 AM, Fabien COELHO  wrote:
> Would this approach be acceptable, or is modifying Nodes a no-go area?
>
> If it is acceptable, I can probably put together a patch and submit it.
>
> If not, I suggest to update the documentation to tell that
> pg_stat_statements does not work properly with combined queries.

I think you've found a bug, but I'm a little doubtful about your
proposed fix.  However, I haven't studied the code, so I don't know
what other approach might be better.



FWIW this issue with multi-statements also causes issues with
ProcessUtility_hook. It gets the char* querytext of the whole
multistatement. Then gets invoked once for each utility command within. It
has no information about which statement text the current invocation
corresponds to.

Having a.pointer into the query text for the start and end would be good
there too. Not as good as doing away with multis entirely as a bad hack but
that's not practical for BC and protocol reasons.

BTW we should be sure the somewhat wacky semantics of multi-statements with
embedded commits are documented. I'll check tomorrow.


Re: [HACKERS] IF (NOT) EXISTS in psql-completion

2016-12-26 Thread Pavel Stehule
2016-12-26 9:40 GMT+01:00 Kyotaro HORIGUCHI :

> > Thanks for reviewing but I ran out of time for this CF..
> >
> > I'm going to move this to the next CF.
>
> I splitted the patch into small pieces. f3fd531 conflicted to
> this so rebased onto the current master HEAD.
>
> 0001 is psql_completion refactoring.
> 0002-0003 are patches prividing new infrastructures.
> 0004 is README for the infrastructures.
> 0005 is letter-case correction of SET/RESET/SHOW using 0002.
> 0006-0008 are improvements of recursive syntaxes using 0001 and 0004.
> 0009-0016 are simplifying (maybe) completion code per syntax.
>
> The last one (0017) is the IF(NOT)EXIST modifications. It
> suggests if(not)exists for syntaxes already gets object
> suggestion. So some kind of objects like operator, cast and so
> don't get an if.. suggestion. Likewise, I intentionally didn't
> modified siggestions for "TEXT SEARCH *".
>
>
lot of patches. I hope I look on these patches this week.

Regards

Pavel


> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
>
>> A possible compromise I have proposed is to have some declared access
>> restrictions on simple session variables, so that say only the owner can
>> access it, but they should stay and look like light-weight session
>> variables nevertheless. That could look like:
>>
>>   SET ROLE Admin;
>>   DECLARE @secure_variable INTEGER RESTRICT; -- only accessible to Admin
>>
>
Why introduce any another security system?

Regards

Pavel


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
2016-12-26 13:08 GMT+01:00 Fabien COELHO :

>
>
> Hello Pavel,
>
> you are talk about light session variables like MSSQL or MySQL (with same
>> syntax), I am talking about secure session variables like Oracle package
>> variables (with similar access syntax).
>>
>
> Hmmm. I do not know this Oracle stuff... After looking at the online
> documentation, my understanding of "Oracle package variables" refers to
> full fledged database objects, in particular they are not session limited.
> The example I found is about a variable holding the total number of
> employees, with functions hire & fire (well, they call it remove_emp) to
> update them when inserting or deleting an employee.
>
> AFAICS they are shared between backends, subjects to transactions and
> permissions, constraints and so on. So they look more like the first
> category I outlined, and probably they cost as any persistent database
> object, which make sense. They constitute a consistent design.
>

no
http://stackoverflow.com/questions/2383061/scope-of-oracle-package-level-variables



>
>
> This is * not * what you are proposing.
>
>
> [...] I have two important reasons why I insist on pg_class base.
>>
>> 1. security .. it is really fundamental part
>> 2. possibility to static check by plpgsql_check - without entry in
>> pg_class
>> (or other catalogue table) I have not any valid information about type,
>> existence of any variable.
>>
>
> Hmmm. I'm not quite convinced that putting session variables in pg_class
> is a good idea, because it adds significant costs for the use case of
> "standard" simple session variables, which is quite more probable than
> session-but-with-permissions variables.
>
> As far as security is concerned, ISTM that a limited but still useful
> access control can be implemented for a key-value store with simple session
> variables, see below.
>
> As far as typing is concerned, ISTM that it can be done as well for
> session variables by going through text and using casts when setting and
> getting values, or through some other simple ad-hoc checking.
>
> Although I am not supporter (due possible issues with plpgsql_checks) of
>> MySQL or MSSQL style variables I am not strongly against this
>> implementation with same syntax. But it is different feature, with
>> different benefits and costs.
>>
>
> I didn't proposed the packages (and package variables) due issues in
>> multilingual PostgreSQL environment and because it is redundant to
>> PostgreSQL schemas.
>>
>
> Instead I proposed >>secure global session variables<< (global like global
>> temporary tables).
>>
>
> That's where I'm leaving you and start disagreeing, because it is not
> consistent: you are proposing session variables that do not look like
> session variable and are somehow costly.
>
> I could agree with real "secure global variables" as in Oracle packages, a
> consistent kind of database object which stores a persistent value safely
> and securely. That would cost, but that is life in a database, you have
> great things for a price. Probably that could be implemented as a row in
> some special table, or as a one-row table, or whatever.
>
> I could also agree with à la MS or MY-SQL session variables that look like
> session variables, with limited ambition, light-weight and inexpensive.
>
> I disagree with having a half-backed stuff, where something looks like a
> database object (i.e. CREATE/ALTER/DROP/GRANT/REVOKE) but is really a
> session object with strange properties. I also disagree to the pg_class
> approach as it creates in effect an expensive session object while a simple
> session object would cost much less and would be much more useful.
>
> To summarize, I still think that your design is not consistent, even if it
> makes sense for some degree wrt the implementation.
>
> A possible compromise I have proposed is to have some declared access
> restrictions on simple session variables, so that say only the owner can
> access it, but they should stay and look like light-weight session
> variables nevertheless. That could look like:
>
>   SET ROLE Admin;
>   DECLARE @secure_variable INTEGER RESTRICT; -- only accessible to Admin
>   SET @secure_variable = 3;
>
>   SET ROLE BasicUser;
>   SELECT @secure_variable; -- say NULL or error does not exist...
>
>
what will be if BasicUser does DECLARE @secure_variable

There are not any granularity of rights - you cannot to grant access ...




>   SET ROLE Admin;
>   SELECT @secure_variable; 3
>

I am sorry, I don't see benefit in your proposal. Probably there will be
only one agreement, so there are not agreement between us :(

Regards

Pavel


>   ...
>
> Currently light session variables can be implemented as not big extension.
>>
>
> Sure. I would review that as well.
>
> --
> Fabien.


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Fabien COELHO



Hello Pavel,


you are talk about light session variables like MSSQL or MySQL (with same
syntax), I am talking about secure session variables like Oracle package
variables (with similar access syntax).


Hmmm. I do not know this Oracle stuff... After looking at the online 
documentation, my understanding of "Oracle package variables" refers to 
full fledged database objects, in particular they are not session 
limited. The example I found is about a variable holding the total number 
of employees, with functions hire & fire (well, they call it remove_emp) 
to update them when inserting or deleting an employee.


AFAICS they are shared between backends, subjects to transactions and 
permissions, constraints and so on. So they look more like the first 
category I outlined, and probably they cost as any persistent database 
object, which make sense. They constitute a consistent design.



This is * not * what you are proposing.



[...] I have two important reasons why I insist on pg_class base.

1. security .. it is really fundamental part
2. possibility to static check by plpgsql_check - without entry in pg_class
(or other catalogue table) I have not any valid information about type,
existence of any variable.


Hmmm. I'm not quite convinced that putting session variables in pg_class 
is a good idea, because it adds significant costs for the use case of 
"standard" simple session variables, which is quite more probable than 
session-but-with-permissions variables.


As far as security is concerned, ISTM that a limited but still useful 
access control can be implemented for a key-value store with simple 
session variables, see below.


As far as typing is concerned, ISTM that it can be done as well for 
session variables by going through text and using casts when setting and 
getting values, or through some other simple ad-hoc checking.



Although I am not supporter (due possible issues with plpgsql_checks) of
MySQL or MSSQL style variables I am not strongly against this
implementation with same syntax. But it is different feature, with
different benefits and costs.



I didn't proposed the packages (and package variables) due issues in
multilingual PostgreSQL environment and because it is redundant to
PostgreSQL schemas.


Instead I proposed >>secure global session variables<< (global like 
global temporary tables).


That's where I'm leaving you and start disagreeing, because it is not 
consistent: you are proposing session variables that do not look like 
session variable and are somehow costly.


I could agree with real "secure global variables" as in Oracle packages, a 
consistent kind of database object which stores a persistent value safely 
and securely. That would cost, but that is life in a database, you have 
great things for a price. Probably that could be implemented as a row in 
some special table, or as a one-row table, or whatever.


I could also agree with à la MS or MY-SQL session variables that look like 
session variables, with limited ambition, light-weight and inexpensive.


I disagree with having a half-backed stuff, where something looks like a 
database object (i.e. CREATE/ALTER/DROP/GRANT/REVOKE) but is really a 
session object with strange properties. I also disagree to the pg_class 
approach as it creates in effect an expensive session object while a 
simple session object would cost much less and would be much more useful.


To summarize, I still think that your design is not consistent, even if it 
makes sense for some degree wrt the implementation.


A possible compromise I have proposed is to have some declared access 
restrictions on simple session variables, so that say only the owner can 
access it, but they should stay and look like light-weight session 
variables nevertheless. That could look like:


  SET ROLE Admin;
  DECLARE @secure_variable INTEGER RESTRICT; -- only accessible to Admin
  SET @secure_variable = 3;

  SET ROLE BasicUser;
  SELECT @secure_variable; -- say NULL or error does not exist...

  SET ROLE Admin;
  SELECT @secure_variable; 3
  ...

Currently light session variables can be implemented as not big 
extension.


Sure. I would review that as well.

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


Re: [HACKERS] Declarative partitioning - another take

2016-12-26 Thread Amit Langote
Sorry about the delay in replying.

On 2016/12/23 8:08, Robert Haas wrote:
> On Thu, Dec 22, 2016 at 3:35 AM, Amit Langote
>  wrote:
>> While working on that, I discovered yet-another-bug having to do with the
>> tuple descriptor that's used as we route a tuple down a partition tree. If
>> attnums of given key attribute(s) are different on different levels, it
>> would be incorrect to use the original slot's (one passed by ExecInsert())
>> tuple descriptor to inspect the original slot's heap tuple, as we go down
>> the tree.  It might cause spurious "partition not found" at some level due
>> to looking at incorrect field in the input tuple because of using the
>> wrong tuple descriptor (root table's attnums not always same as other
>> partitioned tables in the tree).  Patch 0001 fixes that including a test.
> 
> I committed this, but I'm a bit uncomfortable with it: should the
> TupleTableSlot be part of the ModifyTableState rather than the EState?

Done that way in 0001 of the attached patches.  So, instead of making the
standalone partition_tuple_slot a field of EState (with the actual
TupleTableSlot in its tupleTable), it is now allocated within
ModifyTableState and CopyState, and released when ModifyTable node or
CopyFrom finishes, respectively.

>> It also addresses the problem I mentioned previously that once
>> tuple-routing is done, we failed to switch to a slot with the leaf
>> partition's tupdesc (IOW, continued to use the original slot with root
>> table's tupdesc causing spurious failures due to differences in attums
>> between the leaf partition and the root table).
>>
>> Further patches 0002, 0003 and 0004 fix bugs that I sent one-big-patch for
>> in my previous message.  Each patch has a test for the bug it's meant to fix.
> 
> Regarding 0002, I think that this is kind of a strange fix.  Wouldn't
> it be better to get hold of the original tuple instead of reversing
> the conversion?  And what of the idea of avoiding the conversion in
> the (probably very common) case where we can?

To get hold of the original tuple, how about adding an argument orig_slot
to ExecConstraints()?  I've implemented that approach in the new 0002.

Regarding the possibility of avoiding the conversion in very common cases,
I think that could be done considering the following:  If the mapping from
the attribute numbers of the parent table to that of a child table is an
identity map, we don't need to convert tuples.  Currently however,
convert_tuples_by_name() also requires tdtypeid of the input and output
TupleDescs to be equal.  The reason cited for that is that we may fail to
"inject the right OID into the tuple datum" if the types don't match.  In
case of partitioning, hasoid status must match between the parent and its
partitions at all times, so the aforementioned condition is satisfied
without requiring that tdtypeid are same.  And oid column (if present) is
always located at a given position in HeapTuple, so need not map that.

Based on the above argument, patch 0006  teaches convert_tuples_by_name()
to *optionally* not require tdtypeid of input and output tuple descriptors
to be equal.  It's implemented by introducing a new argument to
convert_tuples_by_name() named 'consider_typeid'.  We pass 'false' only
for the partitioning cases.


(Perhaps, the following should be its own new thread)

I noticed that ExecProcessReturning() doesn't work properly after tuple
routing (example shows how returning tableoid currently fails but I
mention some other issues below):

create table p (a int, b int) partition by range (a);
create table p1 partition of p for values from (1) to (10);
insert into p values (1) returning tableoid::regclass, *;
 tableoid | a | b
--+---+---
 -| 1 |
(1 row)

INSERT 0 1

I tried to fix that in 0007 to get:

insert into p values (1) returning tableoid::regclass, *;
 tableoid | a | b
--+---+---
 p| 1 |
(1 row)

INSERT 0 1

But I think it *may* be wrong to return the root table OID for tuples
inserted into leaf partitions, because with select we get partition OIDs:

select tableoid::regclass, * from p;
 tableoid | a | b
--+---+---
 p1   | 1 |
(1 row)

If so, that means we should build the projection info (corresponding to
the returning list) for each target partition somehow.  ISTM, that's going
to have to be done within the planner by appropriate inheritance
translation of the original returning targetlist.

Thanks,
Amit
>From 89f8740195189cc77391bdb844f5092c0440f061 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 26 Dec 2016 11:53:19 +0900
Subject: [PATCH 1/7] Allocate partition_tuple_slot in respective nodes

...instead of making it part of EState and its tuple table.
Respective nodes means ModifyTableState and CopyState for now.

Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
 src/backend/commands/copy.c| 30 +-
 src/backend/executor/execMain.c  

Re: [HACKERS] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2016-12-26 Thread Amit Langote
On 2016/12/26 19:06, Amit Langote wrote:
> I suspect the following is a bug:

A better subject line could be: "ALTER TABLE INHERIT and the oid column"

Thanks,
Amit




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


[HACKERS] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2016-12-26 Thread Amit Langote
I suspect the following is a bug:

create table foo (a int) with oids;
CREATE TABLE
create table bar (a int);
CREATE TABLE
alter table bar inherit foo;
ERROR:  table "bar" without OIDs cannot inherit from table "foo" with OIDs

alter table bar set with oids;
ALTER TABLE
alter table bar inherit foo;
ALTER TABLE

alter table foo set without oids;
ERROR:  relation 16551 has non-inherited attribute "oid"

Because:

select attinhcount from pg_attribute where attrelid = 'bar'::regclass and
attname = 'oid';
 attinhcount
-
   0
(1 row)

Which also means "oid" can be safely dropped from bar breaking the
invariant that if the parent table has oid column, its child tables must too:

alter table bar drop oid;  -- or, alter table bar set without oids;
ALTER TABLE

Attached patches modifies MergeAttributesIntoExisting() such that we
increment attinhcount not only for user attributes, but also for the oid
system column if one exists.

Thoughts?

Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a7ac85e7ab..225a1fe9a1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10901,6 +10901,30 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
 		}
 	}
 
+	/*
+	 * Must update the child's pg_attribute entry for the "oid" column,
+	 * if any.
+	 */
+	if (tupleDesc->tdhasoid)
+	{
+		Form_pg_attribute childatt;
+
+		tuple = SearchSysCacheCopyAttName(RelationGetRelid(child_rel), "oid");
+		Assert(HeapTupleIsValid(tuple));
+		childatt = (Form_pg_attribute) GETSTRUCT(tuple);
+		childatt->attinhcount++;
+		/* See the comment above. */
+		if (child_is_partition)
+		{
+			Assert(childatt->attinhcount == 1);
+			childatt->attislocal = false;
+		}
+
+		simple_heap_update(attrrel, >t_self, tuple);
+		CatalogUpdateIndexes(attrrel, tuple);
+		heap_freetuple(tuple);
+	}
+
 	heap_close(attrrel, RowExclusiveLock);
 }
 
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 38ea8e86f3..d1d20167c6 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1814,3 +1814,28 @@ drop cascades to table part_40_inf
 drop cascades to table part_40_inf_ab
 drop cascades to table part_40_inf_cd
 drop cascades to table part_40_inf_null
+-- check that oid column is handled properly during alter table inherit
+create table _parent (a int) with oids;
+create table _child (a int) with oids;
+alter table _child inherit _parent;
+-- fail
+alter table _child set without oids;
+ERROR:  cannot drop inherited column "oid"
+-- should return 1
+select attinhcount from pg_attribute where attrelid = '_child'::regclass and attname = 'oid';
+ attinhcount 
+-
+   1
+(1 row)
+
+alter table _parent set without oids;
+-- should return 0
+select attinhcount from pg_attribute where attrelid = '_child'::regclass and attname = 'oid';
+ attinhcount 
+-
+   0
+(1 row)
+
+-- cleanup
+drop table _parent cascade;
+NOTICE:  drop cascades to table _child
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index e22a14ebda..01e04169e8 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -588,3 +588,18 @@ explain (costs off) select * from range_list_parted where a >= 30;
 
 drop table list_parted cascade;
 drop table range_list_parted cascade;
+
+-- check that oid column is handled properly during alter table inherit
+create table _parent (a int) with oids;
+create table _child (a int) with oids;
+alter table _child inherit _parent;
+-- fail
+alter table _child set without oids;
+-- should return 1
+select attinhcount from pg_attribute where attrelid = '_child'::regclass and attname = 'oid';
+alter table _parent set without oids;
+-- should return 0
+select attinhcount from pg_attribute where attrelid = '_child'::regclass and attname = 'oid';
+
+-- cleanup
+drop table _parent cascade;

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


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
2016-12-26 10:54 GMT+01:00 Pavel Stehule :

> Hi
>
>
>>
>> In both case, the syntax should be nice and elegant... i.e. not only
>> based on functions, probably it should use some prefix convention (@, $)...
>> For the light weight option.
>>
>>   DECLARE @someday DATE [ =  ] [visibility restriction?];
>>   ... then use @now as a possible value anywhere, which will be
>>   substituted quite early in the execution process, before planning.
>>   -- update a variable value:
>>   [SET, ASSIGN, ... nothing?] @someday = ;
>>
>> Ok, that is basically more or less the mysql syntax, too bad, but I think
>> it makes sense for a lightweight object which should not look like a
>> database object at all to avoid confusion.
>>
>> As far as implementation is concerned, I would use a TEXT to TEXT hash
>> table, and implicit cast the result when substituting.
>>
>>   @var   ->   'text value of var'::type_it_was_declared_with
>>
>
> We are talking about two different features (although the part of name can
> be same):
>
> you are talk about light session variables like MSSQL or MySQL (with same
> syntax), I am talking about secure session variables like Oracle package
> variables (with similar access syntax).
>
> Theoretically, there can be implemented both  - but cannot be implemented
> together. Its are partially different features. My proposal is clearly
> related to analogy with Oracle package variables and should to help to
> people who does migration from Oracle, or who writing application in Oracle
> style - database first, almost logic in database.
>
> I have two important reasons why I insist on pg_class base.
>
> 1. security .. it is really fundamental part
>

Dynamic created variables (like MySQL) cannot be safe - anybody can create
variables with self preferred visibility.



> 2. possibility to static check by plpgsql_check - without entry in
> pg_class (or other catalogue table) I have not any valid information about
> type, existence of any variable.
>
> Although I am not supporter (due possible issues with plpgsql_checks) of
> MySQL or MSSQL style variables I am not strongly against this
> implementation with same syntax. But it is different feature, with
> different benefits and costs.
>
> I didn't proposed the packages (and package variables) due issues in
> multilingual PostgreSQL environment and because it is redundant to
> PostgreSQL schemas. Instead I proposed >>secure global session variables<<
> (global like global temporary tables).
>
> Currently light session variables can be implemented as not big extension.
> Secure session variables depends on pg_class internals.
>
> I am not sure if we need a special symbols - it is traditional only.
> Set/Get functions can do same work - years we use same technique for
> sequences. Setter function is simply. Currently is impossible to write
> elegant getter function - because the analyzer has limited work with "any"
> returning functions.
>
> Can be nice to have special hook for functions that returns "any" to push
> there some other external informations.
>
> Regards
>
> Pavel
>
>
>
>
>>
>> --
>> Fabien.
>>
>
>


Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Pavel Stehule
Hi


>
> In both case, the syntax should be nice and elegant... i.e. not only based
> on functions, probably it should use some prefix convention (@, $)...
> For the light weight option.
>
>   DECLARE @someday DATE [ =  ] [visibility restriction?];
>   ... then use @now as a possible value anywhere, which will be
>   substituted quite early in the execution process, before planning.
>   -- update a variable value:
>   [SET, ASSIGN, ... nothing?] @someday = ;
>
> Ok, that is basically more or less the mysql syntax, too bad, but I think
> it makes sense for a lightweight object which should not look like a
> database object at all to avoid confusion.
>
> As far as implementation is concerned, I would use a TEXT to TEXT hash
> table, and implicit cast the result when substituting.
>
>   @var   ->   'text value of var'::type_it_was_declared_with
>

We are talking about two different features (although the part of name can
be same):

you are talk about light session variables like MSSQL or MySQL (with same
syntax), I am talking about secure session variables like Oracle package
variables (with similar access syntax).

Theoretically, there can be implemented both  - but cannot be implemented
together. Its are partially different features. My proposal is clearly
related to analogy with Oracle package variables and should to help to
people who does migration from Oracle, or who writing application in Oracle
style - database first, almost logic in database.

I have two important reasons why I insist on pg_class base.

1. security .. it is really fundamental part
2. possibility to static check by plpgsql_check - without entry in pg_class
(or other catalogue table) I have not any valid information about type,
existence of any variable.

Although I am not supporter (due possible issues with plpgsql_checks) of
MySQL or MSSQL style variables I am not strongly against this
implementation with same syntax. But it is different feature, with
different benefits and costs.

I didn't proposed the packages (and package variables) due issues in
multilingual PostgreSQL environment and because it is redundant to
PostgreSQL schemas. Instead I proposed >>secure global session variables<<
(global like global temporary tables).

Currently light session variables can be implemented as not big extension.
Secure session variables depends on pg_class internals.

I am not sure if we need a special symbols - it is traditional only.
Set/Get functions can do same work - years we use same technique for
sequences. Setter function is simply. Currently is impossible to write
elegant getter function - because the analyzer has limited work with "any"
returning functions.

Can be nice to have special hook for functions that returns "any" to push
there some other external informations.

Regards

Pavel




>
> --
> Fabien.
>


Re: [HACKERS] Parallel bitmap heap scan

2016-12-26 Thread Dilip Kumar
On Wed, Dec 21, 2016 at 3:23 PM, Andres Freund  wrote:

Sorry for the delayed response.

> If we go there, it seems better to also wrap the memory context based approach
> in the allocator.

One option is we can keep default allocator in the simple hash, and if
the caller doesn't supply any functions we can use default functions
as I have shown below.

+/* default allocator function */
+static void *
+SH_DEFAULT_ALLOC(Size size, void *args)
+{
+ MemoryContext context = (MemoryContext) args;
+
+ return MemoryContextAllocExtended(context, size,
+  MCXT_ALLOC_HUGE | MCXT_ALLOC_ZERO);
+}
+
+/* default allocator function */
+static void
+SH_DEFAULT_FREE(void *pointer, void *args)
+{
+ pfree(pointer);
+}
+

+SH_CREATE(MemoryContext ctx, uint32 nelements, SH_ALLOCATOR *alloc)
 {
- SH_TYPE*tb;
- uint64 size;
+ SH_TYPE *tb;
+ uint64 size;

  tb = MemoryContextAllocZero(ctx, sizeof(SH_TYPE));
  tb->ctx = ctx;
@@ -294,9 +336,18 @@ SH_CREATE(MemoryContext ctx, uint32 nelements)

  SH_COMPUTE_PARAMETERS(tb, size);

- tb->data = MemoryContextAllocExtended(tb->ctx,
-  sizeof(SH_ELEMENT_TYPE) * tb->size,
-  MCXT_ALLOC_HUGE | MCXT_ALLOC_ZERO);
+ if (!alloc)
+ {
+ tb->alloc = palloc(sizeof(SH_ALLOCATOR));
+ tb->alloc->HashAlloc = SH_DEFAULT_ALLOC;
+ tb->alloc->HashFree = SH_DEFAULT_FREE;
+ tb->alloc->args = tb->ctx;
+ }
+ else
+ tb->alloc = alloc;
+
+ tb->data = tb->alloc->HashAlloc(sizeof(SH_ELEMENT_TYPE) * tb->size,
+ tb->alloc->args);

Other the another option is, that we can always make caller to provide
an allocator. But this way every new user for simple hash need to take
care of having allocator.

What is your opinion?


>This also needs docs, including a warning that just
> using an allocator in shared memory does *NOT* allow the hash table to be
> used in shared memory in the general case.

Make sense.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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


Re: [HACKERS] Protect syscache from bloating with negative cache entries

2016-12-26 Thread Kyotaro HORIGUCHI
At Wed, 21 Dec 2016 10:21:09 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20161221.102109.51106943.horiguchi.kyot...@lab.ntt.co.jp>
> At Tue, 20 Dec 2016 15:10:21 -0500, Tom Lane  wrote in 
> <23492.1482264...@sss.pgh.pa.us>
> > The bigger picture here though is that we used to have limits on syscache
> > size, and we got rid of them (commit 8b9bc234a, see also
> > https://www.postgresql.org/message-id/flat/5141.1150327541%40sss.pgh.pa.us)
> > not only because of the problem you mentioned about performance falling
> > off a cliff once the working-set size exceeded the arbitrary limit, but
> > also because enforcing the limit added significant overhead --- and did so
> > whether or not you got any benefit from it, ie even if the limit is never
> > reached.  Maybe the present patch avoids imposing a pile of overhead in
> > situations where no pruning is needed, but it doesn't really look very
> > promising from that angle in a quick once-over.
> 
> Indeed. As mentioned in the mail at the beginning of this thread,
> it hits the whole-cache scanning if at least one negative cache
> exists even it is not in a relation with the target relid, and it
> can be significantly long on a fat cache.
> 
> Lists of negative entries like CatCacheList would help but needs
> additional memeory.
> 
> > BTW, I don't see the point of the second patch at all?  Surely, if
> > an object is deleted or updated, we already have code that flushes
> > related catcache entries.  Otherwise the caches would deliver wrong
> > data.
> 
> Maybe you take the patch wrongly. Negative entires won't be
> flushed by any means. Deletion of a namespace causes cascaded
> object deletion according to dependency then finaly goes to
> non-neative cache invalidation. But a removal of *negative
> entries* in RELNAMENSP won't happen.
> 
> The test script for the case (gen2.pl) does the following thing,
> 
> CREATE SCHEMA foo;
> SELECT * FROM foo.invalid;
> DROP SCHEMA foo;
> 
> Removing the schema foo leaves a negative cache entry for
> 'foo.invalid' in RELNAMENSP.
> 
> However, I'm not sure the above situation happens so frequent
> that it is worthwhile to amend.

Since 1753b1b conflicts this patch, I rebased this onto the
current master HEAD. I'll register this to the next CF.

The points of discussion are the following, I think.

1. The first patch seems working well. It costs the time to scan
   the whole of a catcache that have negative entries for other
   reloids. However, such negative entries are created by rather
   unusual usages. Accesing to undefined columns, and accessing
   columns on which no statistics have created. The
   whole-catcache scan occurs on ATTNAME, ATTNUM and
   STATRELATTINH for every invalidation of a relcache entry.

2. The second patch also works, but flushing negative entries by
   hash values is inefficient. It scans the bucket corresponding
   to given hash value for OIDs, then flushing negative entries
   iterating over all the collected OIDs. So this costs more time
   than 1 and flushes involving entries that is not necessary to
   be removed. If this feature is valuable but such side effects
   are not acceptable, new invalidation category based on
   cacheid-oid pair would be needed.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From ee0cc13f70d79f23ec9507cf977228bba091bc49 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 15 Dec 2016 17:43:03 +0900
Subject: [PATCH 1/2] Cleanup negative cache of pg_statistic when dropping a
 relation.

Accessing columns that don't have statistics causes leaves negative
entries in catcache for pg_statstic, but there's no chance to remove
them. Especially when repeatedly creating then dropping temporary
tables bloats catcache so much that memory pressure can be
significant. This patch removes negative entries in STATRELATTINH,
ATTNAME and ATTNUM when corresponding relation is dropped.
---
 src/backend/utils/cache/catcache.c |  57 +++-
 src/backend/utils/cache/syscache.c | 277 +++--
 src/include/utils/catcache.h   |   3 +
 src/include/utils/syscache.h   |   2 +
 4 files changed, 265 insertions(+), 74 deletions(-)

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 6016d19..c1d9d2f 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -304,10 +304,11 @@ CatCachePrintStats(int code, Datum arg)
 
 		if (cache->cc_ntup == 0 && cache->cc_searches == 0)
 			continue;			/* don't print unused caches */
-		elog(DEBUG2, "catcache %s/%u: %d tup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
+		elog(DEBUG2, "catcache %s/%u: %d tup, %d negtup, %ld srch, %ld+%ld=%ld hits, %ld+%ld=%ld loads, %ld invals, %ld lsrch, %ld lhits",
 			 cache->cc_relname,
 			 cache->cc_indexoid,
 			 cache->cc_ntup,
+			 

Re: [HACKERS] proposal: session server side variables

2016-12-26 Thread Fabien COELHO


Hello Pavel,


On Sat, 24 Dec 2016, Pavel Stehule wrote:

Maybe you could consider removing the part of the message that you are not 
responding to, so that it would be easier for the reader to see your 
answers and comments.



Hmmm. So I understand that you would like to do something like:

  - call a secure function which sets a session variable with restricted
permissions
  - do some things which cannot access or change the variable
  - call another secure function which can access, update, remove the
variable...


I'm still not clear with your use case. Did I read you correctly? ISTM 
that the above use case could be managed with insert/update/delete in a 
table with chosen permissions from the functions...




Yep, but if you need persistant and transactional then probably you can
accept less performant...


When you accept less performance, then you can use temporary tables. You
can easy wrap it by few polymorphic functions.


Probably. This is probably true as well from what I understood from your 
use case.




The namespace issue is unclear to me. Would a variable name clash with a
table name? It should if you want to be able write "SELECT stuff FROM
variablename", which may or may not be a good idea.


It is based on history and experience - one fundamental issue of languages
for stored procedures is a conflict of variables and SQL identifiers.


I agree that this is a pain, which could be solved by using a prefix, say 
$ for instance.


When variables are based on pg_class, there are not possibility to any 
new conflict.


If variables are based on pg_class, ISTM that they will cost anyway.


To sum up my current opinion, taking into accounts your use case and Tom & 
Jim argments about performance, I think that variables should be either:


 - full-featured database objects well integrated in the database logic: 
CREATE/ALTER/DROP, in pg_class, subject to standard permissions, 
constraints, transactions, possibly persistent... Basically like a one-row 
table, although the implementation should be more efficient, I agree.


** OR **

 - very light-weight, a simple server process key-value store, which would 
not use CREATE/ALTER/DROP which suggest otherwise, they would not be 
subject to permissions nor transactions nor persistence but die with the 
session, goodbye. A possible concession to permissions would be to have a 
per-role store, and/or some visibility/accessibility declaration at 
creation time, but certainly not GRANT/RESTORE syntax which suggest a 
database persistent object.



I'm very reserved about anything in between these two options, which looks 
like a database object but is not really one, so I think that it create 
confusion.



In both case, the syntax should be nice and elegant... i.e. not only based 
on functions, probably it should use some prefix convention (@, $)...

For the light weight option.

  DECLARE @someday DATE [ =  ] [visibility restriction?];
  ... then use @now as a possible value anywhere, which will be
  substituted quite early in the execution process, before planning.
  -- update a variable value:
  [SET, ASSIGN, ... nothing?] @someday = ;

Ok, that is basically more or less the mysql syntax, too bad, but I think 
it makes sense for a lightweight object which should not look like a 
database object at all to avoid confusion.


As far as implementation is concerned, I would use a TEXT to TEXT hash 
table, and implicit cast the result when substituting.


  @var   ->   'text value of var'::type_it_was_declared_with

--
Fabien.


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