Re: [HACKERS] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Tuesday, October 20, 2015 1:11 PM
> To: Robert Haas
> Cc: Tom Lane; Kaigai Kouhei(海外 浩平); Kyotaro HORIGUCHI;
> pgsql-hackers@postgresql.org; Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/10/20 5:34, Robert Haas wrote:
> > On Mon, Oct 19, 2015 at 3:45 AM, Etsuro Fujita
> >  wrote:
> >> As Tom mentioned, just recomputing the original join tuple is not good
> >> enough.  We would need to rejoin the test tuples for the baserels even if
> >> ROW_MARK_COPY is in use.  Consider:
> >>
> >> A=# BEGIN;
> >> A=# UPDATE t SET a = a + 1 WHERE b = 1;
> >> B=# SELECT * from t, ft1, ft2
> >>   WHERE t.a = ft1.a AND t.b = ft2.b AND ft1.c = ft2.c FOR UPDATE;
> >> A=# COMMIT;
> >>
> >> where the plan for the SELECT FOR UPDATE is
> >>
> >> LockRows
> >> -> Nested Loop
> >> -> Seq Scan on t
> >> -> Foreign Scan on 
> >>  Remote SQL: SELECT * FROM ft1 JOIN ft2 WHERE ft1.c = ft2.c AND 
> >> ft1.a
> >> = $1 AND ft2.b = $2
> >>
> >> If an EPQ recheck is invoked by the A's UPDATE, just recomputing the
> >> original join tuple from the whole-row image that you proposed would output
> >> an incorrect result in the EQP recheck since the value a in the updated
> >> version of a to-be-joined tuple in t would no longer match the value ft1.a
> >> extracted from the whole-row image if the A's UPDATE has committed
> >> successfully.  So I think we would need to rejoin the tuples populated from
> >> the whole-row images for the baserels ft1 and ft2, by executing the
> >> secondary plan with the new parameter values for a and b.
> 
> > No.  You just need to populate fdw_recheck_quals correctly, same as
> > for the scan case.
> 
> Yeah, I think we can probably do that for the case where a pushed-down
> join clause is an inner-join one, but I'm not sure that we can do that
> for the case where that clause is an outer-join one.  Maybe I'm missing
> something, though.
>
Please check my message yesterday. The non-nullable side of outer-join is
always visible regardless of the join-clause pushed down, as long as it
satisfies the scan-quals pushed-down.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


-- 
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] checkpointer continuous flushing

[Amit Kapila]

On Mon, Oct 19, 2015 at 4:06 AM, Andres Freund  wrote:
>
>
> I wonder if mmap() && msync(MS_ASYNC) isn't a better replacement for
> sync_file_range(SYNC_FILE_RANGE_WRITE) than posix_fadvise(DONTNEED). It
> might even be possible to later approximate that on windows using
> FlushViewOfFile().
>

I think this idea is worth exploring especially because we can have
Windows equivalent for this optimisation.  Will this option by any
chance can lead to increase in memory usage as mmap has to
map the file/'s?


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Checkpoint throttling issues

[Amit Kapila]

On Tue, Oct 20, 2015 at 1:05 AM, Alvaro Herrera 
wrote:
>
> Fabien COELHO wrote:
>
> > >4) It's a bit dubious to only pgstat_send_bgwriter() when on schedule.
> >
> > No opinion!
>
> My guess here, without looking, is that this was based on the idea of
> "oops, we're late here for the checkpoint, let's do as less work as
> possible to avoid getting even later", and thus skip some "unnecessary"
> tasks.
>

I also think thats what could be the reason for writing the code that way,
it seems to me that processing config file when not on schedule has
an advantage such that if user has modified checkpoint related parameters
(like checkpoint_completion_target), that could improve the checkpoint
behaviour.  Now there could be some negative impact of this change as
well if user changes full_page_writes parameter, as to process that, it
needs to write a WAL record which could be costly when the checkpoint is
not on schedule, but I think the odds of that are less, so there doesn't
seem to be any harm in changing this behaviour.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] SuperUser check in pg_stat_statements

[Feike Steenbergen]

You can create a Security Definer Funtion which can then be executed by
then non-superuser monitoring role:

(Assuming you have a role monitoring and pg_stat_statements is installed in
schema public)

-- connected as a superuser
CREATE FUNCTION pg_stat_statements()
RETURNS SETOF pg_stat_statements
LANGUAGE SQL
SET search_path='public'
SECURITY DEFINER
AS
$BODY$
SELECT *
  FROM pg_stat_statements;
$BODY$;

REVOKE ALL ON FUNCTION pg_stat_statements() FROM public;
GRANT EXECUTE ON FUNCTION pg_stat_statements() TO monitoring;

-- connected as monitoring
SELECT * FROM pg_stat_statements();

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Syed, Rahila]

Hello,

>I think that you should add the flag or something which indicates whether this 
>backend is running VACUUM or not, into PgBackendStatus.
>pg_stat_vacuum_progress should display the entries of only backends with that 
>flag set true. This design means that you need to set the flag to true when 
>starting VACUUM and reset at the end of VACUUM progressing.
Please find attached  updated patch which adds a flag in PgBackendStatus which 
indicates whether this backend in running VACUUM.
Also, pgstat_report_progress function is changed to make it generic for all 
commands reporting progress.

Thank you,
Rahila Syed



__
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.


Vacuum_progress_checker_v5.patch
Description: Vacuum_progress_checker_v5.patch

-- 
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] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On Mon, Oct 19, 2015 at 3:45 AM, Etsuro Fujita
 wrote:

As Tom mentioned, just recomputing the original join tuple is not good
enough.  We would need to rejoin the test tuples for the baserels even if
ROW_MARK_COPY is in use.  Consider:

A=# BEGIN;
A=# UPDATE t SET a = a + 1 WHERE b = 1;
B=# SELECT * from t, ft1, ft2
   WHERE t.a = ft1.a AND t.b = ft2.b AND ft1.c = ft2.c FOR UPDATE;
A=# COMMIT;

where the plan for the SELECT FOR UPDATE is

LockRows
-> Nested Loop
 -> Seq Scan on t
 -> Foreign Scan on 
  Remote SQL: SELECT * FROM ft1 JOIN ft2 WHERE ft1.c = ft2.c AND ft1.a
= $1 AND ft2.b = $2

If an EPQ recheck is invoked by the A's UPDATE, just recomputing the
original join tuple from the whole-row image that you proposed would output
an incorrect result in the EQP recheck since the value a in the updated
version of a to-be-joined tuple in t would no longer match the value ft1.a
extracted from the whole-row image if the A's UPDATE has committed
successfully.  So I think we would need to rejoin the tuples populated from
the whole-row images for the baserels ft1 and ft2, by executing the
secondary plan with the new parameter values for a and b.


Robert Haas wrote:

No.  You just need to populate fdw_recheck_quals correctly, same as
for the scan case.


I wrote:

Yeah, I think we can probably do that for the case where a pushed-down
join clause is an inner-join one, but I'm not sure that we can do that
for the case where that clause is an outer-join one.  Maybe I'm missing
something, though.


On 2015/10/20 15:42, Kouhei Kaigai wrote:

Please check my message yesterday. The non-nullable side of outer-join is
always visible regardless of the join-clause pushed down, as long as it
satisfies the scan-quals pushed-down.


Sorry, my explanation was not correct.  (Needed to take in caffeine.) 
What I'm concerned about is the following:


SELECT * FROM localtab JOIN (ft1 LEFT JOIN ft2 ON ft1.x = ft2.x) ON 
localtab.id = ft1.id FOR UPDATE OF ft1


LockRows
-> Nested Loop
 Join Filter: (localtab.id = ft1.id)
 -> Seq Scan on localtab
 -> Foreign Scan on 
  Remote SQL: SELECT * FROM ft1 LEFT JOIN ft2 WHERE ft1.x = 
ft2.x FOR UPDATE OF ft1


Assume that ft1 performs late row locking.  If an EPQ recheck was 
invoked due to a concurrent transaction on the remote server that 
changed only the value x of the ft1 tuple previously retrieved, then we 
would have to generate a fake ft1/ft2-join tuple with nulls for ft2. 
(Assume that the ft2 tuple previously retrieved was not a null tuple.) 
However, I'm not sure how we can do that in ForeignRecheck; we can't 
know for example, which one is outer and which one is inner, without an 
alternative local join execution plan.  Maybe I'm missing something, though.


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] Multi-column distinctness.

[Simon Riggs]

On 19 October 2015 at 20:16, Tomas Vondra 
wrote:

> Hello Kyotaro-san,
>
> On 09/11/2015 06:58 PM, Tomas Vondra wrote:
> >
>
>> Maybe the best solution is to abandon the ALTER TABLE approach
>> entirely, and instead invent a new set of commands
>>
>>CREATE STATISTICS
>>DROP STATISTICS
>>
>> (ALTER STATISTICS seems a bit excessive at this point).
>>
>> Another thing is that perhaps we should add names for statistics,
>> just like we do for constraints, for example. Otherwise the DROP
>> STATISTICS handling is rather awkward - for example if the user
>> creates stats twice by mistake, he's unable to drop just one of them.
>>
>
> Do you think this modified syntax makes sense? I'll have time to hack on
> this over the next few days.


Koyotaro's changes to force COLUMN to be required won't get through.

ISTM that we could use COLLECT STATISTICS instead of ADD STATISTICS, and
use REMOVE STATISTICS instead of DROP STATISTICS. That way we can use ALTER
TABLE rather than inventing a new command. 5 minute change...

Unless there is some better reason for a whole new command?

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

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

[HACKERS] Duplicated assignment of slot_name in walsender.c

[Bernd Helmle]

walsender.c, CreateReplicationSlot() currently has this:

slot_name = NameStr(MyReplicationSlot->data.name);

if (cmd->kind == REPLICATION_KIND_LOGICAL)
{
[...]
}
else if (cmd->kind == REPLICATION_KIND_PHYSICAL && cmd->reserve_wal)
{
[...]
}

slot_name = NameStr(MyReplicationSlot->data.name);

The 2nd assignment to slot_name looks unnecessary?

-- 
Thanks

Bernd


-- 
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] Parallel Aggregate

[David Rowley]

On 13 October 2015 at 20:57, Haribabu Kommi 
wrote:

> On Tue, Oct 13, 2015 at 5:53 PM, David Rowley
>  wrote:
> > On 13 October 2015 at 17:09, Haribabu Kommi 
> > wrote:
> >>
> >> On Tue, Oct 13, 2015 at 12:14 PM, Robert Haas 
> >> wrote:
> >> > Also, I think the path for parallel aggregation should probably be
> >> > something like FinalizeAgg -> Gather -> PartialAgg -> some partial
> >> > path here.  I'm not clear whether that is what you are thinking or
> >> > not.
> >>
> >> No. I am thinking of the following way.
> >> Gather->partialagg->some partial path
> >>
> >> I want the Gather node to merge the results coming from all workers,
> >> otherwise
> >> it may be difficult to merge at parent of gather node. Because in case
> >> the partial
> >> group aggregate is under the Gather node, if any of two workers are
> >> returning
> >> same group key data, we need to compare them and combine it to make it a
> >> single group. If we are at Gather node, it is possible that we can
> >> wait till we get
> >> slots from all workers. Once all workers returns the slots we can
> compare
> >> and merge the necessary slots and return the result. Am I missing
> >> something?
> >
> >
> > My assumption is the same as Robert's here.
> > Unless I've misunderstood, it sounds like you're proposing to add logic
> into
> > the Gather node to handle final aggregation? That sounds like a
> modularity
> > violation of the whole node concept.
> >
> > The handling of the final aggregate stage is not all that different from
> the
> > initial aggregate stage. The primary difference is just that your calling
> > the combine function instead of the transition function, and the values
>
> Yes, you are correct, till now i am thinking of using transition types as
> the
> approach, because of that reason only I proposed it as Gather node to
> handle
> the finalize aggregation.
>
> > being aggregated are aggregates states rather than the type of the values
> > which were initially aggregated. The handling of GROUP BY is all the
> same,
> > yet you only apply the HAVING clause during final aggregation. This is
> why I
> > ended up implementing this in nodeAgg.c instead of inventing some new
> node
> > type that's mostly a copy and paste of nodeAgg.c [1]
>
> After going through your Partial Aggregation / GROUP BY before JOIN patch,
> Following is my understanding of parallel aggregate.
>
> Finalize [hash] aggregate
> -> Gather
>   -> Partial [hash] aggregate
>
> The data that comes from the Gather node contains the group key and
> grouping results.
> Based on these we can generate another hash table in case of hash
> aggregate at
> finalize aggregate and return the final results. This approach works
> for both plain and
> hash aggregates.
>
> For group aggregate support of parallel aggregate, the plan should be
> as follows.
>
> Finalize Group aggregate
> ->sort
> -> Gather
>   -> Partial group aggregate
>->sort
>
> The data that comes from Gather node needs to be sorted again based on
> the grouping key,
> merge the data and generates the final grouping result.
>
> With this approach, we no need to change anything in Gather node. Is
> my understanding correct?
>
>
Our understandings are aligned.

Regards

David Rowley

--
 David Rowley   http://www.2ndQuadrant.com/

 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Beena Emerson]

On Mon, Oct 19, 2015 at 8:47 PM, Masahiko Sawada 
wrote:

>
> Hi,
>
> Attached patch is a rough patch which supports multi sync replication
> by another approach I sent before.
>
> The new GUC parameters are:
> * synchronous_standby_num, which specifies the number of standby
> servers using sync rep. (default is 0)
> * synchronous_replication_method, which specifies replication method;
> priority or quorum. (default is priority)
>
> The behaviour of 'priority' and 'quorum' are same as what we've been
> discussing.
> But I write overview of these here again here.
>
> [Priority Method]
> The standby server has each different priority, and the active standby
> servers having the top N priroity are become sync standby server.
> If synchronous_standby_names = '*', the all active standby server
> would be sync standby server.
> If you want to set up standby like 9.5 or before, you can set
> synchronous_standby_num = 1.
>
>

I used the following setting with 2 servers A and D connected:

synchronous_standby_names = 'A,B,C,D'
synchronous_standby_num = 2
synchronous_replication_method = 'priority'

Though s_r_m = 'quorum' worked fine, changing it to 'priority' caused
segmentation fault.

Regards,

Beena Emerson

Have a Great Day!

Re: [HACKERS] PoC: Partial sort

[Alexander Korotkov]

On Fri, Oct 16, 2015 at 7:11 PM, Alexander Korotkov 
wrote:

> On Sun, Jun 7, 2015 at 11:01 PM, Peter Geoghegan  wrote:
>
>> On Sun, Jun 7, 2015 at 8:10 AM, Andreas Karlsson 
>> wrote:
>> > Are you planning to work on this patch for 9.6?
>>
>> FWIW I hope so. It's a nice patch.
>>
>
> I'm trying to to whisk dust. Rebased version of patch is attached. This
> patch isn't passing regression tests because of plan changes. I'm not yet
> sure about those changes: why they happens and are they really regression?
> Since I'm not very familiar with planning of INSERT ON CONFLICT and RLS,
> any help is appreciated.
>

Planner regression is fixed in the attached version of patch. It appears
that get_cheapest_fractional_path_for_pathkeys() behaved wrong when no
ordering is required.

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


partial-sort-basic-4.patch
Description: Binary data

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

[HACKERS] [PROPOSAL] Improvements of Hunspell dictionaries support

[Artur Zakirov]

Hi.

Introduction


PostgreSQL full-text search extension uses dictionaries from the various 
open source spell checker software to perform word normalization.


Currently, Ispell, MySpell and Hunspell dictionaries are supported.

Dictionaries requires two files: a dictionary file and an affix file. A 
dictionary file contains a list of words. Each word may be followed by 
one or more affix flags. An affix file contains a lot of parameters, 
definitions, prefix and suffix classes used in a dictionary file.


Most complete and actively developed are Hunspell dictionaries 
(http://hunspell.sourceforge.net/). OpenOffice and LibreOffice projects 
recently switched from MySpell to Hunspell dictionaries.


But PostgreSQL is unable to load recent version of Hunsplell 
dictionaries for several languages.


It is because affix files of these dictionaries grow too big. 
Traditionally affix rules are named by one extended ASCII (8-bit) 
symbol. And if there is more than 192 rules, some syntax extension is 
needed.


And to handle these dictionaries Hunspell have FLAG parameter with the 
following values:

* FLAG long - sets the double extended ASCII character flag type
* FLAG num - sets the decimal number flag type (from 1 to 65000)

These flag types are used in affix files of such dictionaries as ar, 
br_fr, ca, ca_valencia, da_dk, en_ca, en_gb, en_us, fr, gl_es, is, 
ne_np, nl_nl, si_lk (from 
http://cgit.freedesktop.org/libreoffice/dictionaries/tree/). But 
PostgreSQL does not support FLAG parameter and can not load these 
dictionaries.


There is also AF parameter which allows to substitute affix flag sets 
with ordinal numbers in affix and dictionary files.


FLAG and AF parameters are not supported by PostgreSQL. Supporting these 
parameters allows to load dictionaries listed above into PostgreSQL 
database and use them in full text search.


Proposed Changes


Internal representation of the dictionary in the PostgreSQL doesn't 
impose too strict limits on the number of affix rules. There are a 
flagval array, which size must be increased from 256 to 65000.


All other changes is the changes in the affix file parsing code to 
properly parse long and numeric flags.


I've already implemented support for FLAG long, it require relatively 
small patch size (60 lines). Support for FLAG num would require 
comparable amount of code.


These changes would allow to use recent versions of Hunspell 
dictionaries for following dictionaries:

br_fr, ca, ca_valencia, da_dk, gl_es, is, ne_np, nl_nl, si_lk.

Implementation of AF flag would allow to support also following 
dictionaries:

ar, en_ca, en_gb, en_us, fr, hu_hu.

Expected Results


These changes would allow to use more recent and complete spelling 
dictionaries to perform word stemming during full-text indexing.


--
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] COPY FREEZE and PD_ALL_VISIBLE

[Robert Haas]

On Sun, Oct 18, 2015 at 5:23 PM, Jeff Janes  wrote:
> I'm planning on adding a todo item to have COPY FREEZE set PD_ALL_VISIBLE.
> Or is there some reason this can't be done?
>
> Since the whole point of COPY FREEZE is to avoid needing to rewrite the
> entire table, it seems rather perverse that the first time the table is
> vacuumed, it needs to rewrite the entire table.

*facepalm*

I don't know how hard that is to implement, but +1 for trying to
figure out a way.

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


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

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Robert Haas]

On Sun, Oct 18, 2015 at 4:56 PM, Shay Rojansky  wrote:
> Here's a patch that adds back the GUC, with default/min/max 0 and
> GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE.
>
> This is my first pg patch, please be gentle with any screwups :)

Why, you dummy.

No, actually, this looks fine.  I've committed it and back-patched it
to 9.5.  I took the liberty of making some cosmetic changes, however.

Thanks for preparing this patch.

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


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

Re: [HACKERS] SuperUser check in pg_stat_statements

[rajan]

Hey Lukas,

Thanks. Able to see the queries from all users. Can you explain the
monitoring.get_stat_statements()?



--
View this message in context: 
http://postgresql.nabble.com/SuperUser-check-in-pg-stat-statements-tp5870589p5870733.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

[HACKERS] Typos in plannodes.h

[Etsuro Fujita]

Hi,

I found typos in plannodes.h: s/scan.plan.quals/scan.plan.qual/g  Please
find attached a patch.

Best regards,
Etsuro Fujita
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 92fd8e4..6b28c8e 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -513,7 +513,7 @@ typedef struct WorkTableScan
  * describing what is in the scan tuple's columns.
  *
  * fdw_recheck_quals should contain any quals which the core system passed to
- * the FDW but which were not added to scan.plan.quals; that is, it should
+ * the FDW but which were not added to scan.plan.qual; that is, it should
  * contain the quals being checked remotely.  This is needed for correct
  * behavior during EvalPlanQual rechecks.
  *
@@ -529,7 +529,7 @@ typedef struct ForeignScan
 	List	   *fdw_exprs;		/* expressions that FDW may evaluate */
 	List	   *fdw_private;	/* private data for FDW */
 	List	   *fdw_scan_tlist; /* optional tlist describing scan tuple */
-	List	   *fdw_recheck_quals;	/* original quals not in scan.plan.quals */
+	List	   *fdw_recheck_quals;	/* original quals not in scan.plan.qual */
 	Bitmapset  *fs_relids;		/* RTIs generated by this scan */
 	bool		fsSystemCol;	/* true if any "system column" is needed */
 } ForeignScan;

-- 
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] [RFC] overflow checks optimized away

[Michael Paquier]

On Fri, Oct 16, 2015 at 11:29 PM, Michael Paquier wrote:
> Well, I have played a bit with this patch and rebased it as attached.
> One major change is the use of the variables PG_INT* that have been
> added in 62e2a8d. Some places were not updated with those new checks,
> in majority a couple of routines in int.c (I haven't finished
> monitoring the whole code though). Also, I haven't played yet with my
> compilers to optimize away some of the checks and break them, but I'll
> give it a try with clang and gcc. For now, I guess that this patch is
> a good thing to begin with though, I have checked that code compiles
> and regression tests pass.

Hm. Looking at the options of clang
(http://clang.llvm.org/docs/UsersManual.html), there is no actual
equivalent of fno-wrapv and strict-overflow, there are a couple of
sanitizer functions though (-fsanitize=unsigned-integer-overflow
-fsanitize=signed-integer-overflow) that can be used at run time, but
that's not really useful for us.

I also looked at the definition of SHRT_MIN/MAX in a couple of places
(OSX, Linux, MinGW, Solaris, OpenBSD, FreeBSD, MSVC), and they are
always set as respectively -7fff-1 and 7fff. Hence do we really need
to worry about those two having potentially a different length, are
there opinions about having customs PG_SHRT_MIN/PG_SHRT_MAX in c.h?

Except that, attached is a result of all the hacking I have been doing
regarding this item:
- Addition of some macros to check overflows for INT16
- Addition of a couple of regression tests (Does testing int2inc &
friends make sense?)
- Addition of consistent overflow checks in more code paths, previous
patch missing a couple of places in int8.c, int.c and btree_gist (+
alpha). I have screened the code for existing "out of range" errors.
I'll add that to the next CF, perhaps this will interest somebody.
Regards,
-- 
Michael
diff --git a/contrib/btree_gist/btree_int2.c b/contrib/btree_gist/btree_int2.c
index 54dc1cc..202bd21 100644
--- a/contrib/btree_gist/btree_int2.c
+++ b/contrib/btree_gist/btree_int2.c
@@ -102,7 +102,7 @@ int2_dist(PG_FUNCTION_ARGS)
 	ra = Abs(r);
 
 	/* Overflow check. */
-	if (ra < 0 || (!SAMESIGN(a, b) && !SAMESIGN(r, a)))
+	if (ra < 0 || PG_INT16_SUB_OVERFLOWS(a, b))
 		ereport(ERROR,
 (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
  errmsg("smallint out of range")));
diff --git a/contrib/btree_gist/btree_int4.c b/contrib/btree_gist/btree_int4.c
index ddbcf52..890a7d4 100644
--- a/contrib/btree_gist/btree_int4.c
+++ b/contrib/btree_gist/btree_int4.c
@@ -103,7 +103,7 @@ int4_dist(PG_FUNCTION_ARGS)
 	ra = Abs(r);
 
 	/* Overflow check. */
-	if (ra < 0 || (!SAMESIGN(a, b) && !SAMESIGN(r, a)))
+	if (ra < 0 || PG_INT32_SUB_OVERFLOWS(a, b))
 		ereport(ERROR,
 (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
  errmsg("integer out of range")));
diff --git a/contrib/btree_gist/btree_int8.c b/contrib/btree_gist/btree_int8.c
index 44bf69a..91d4972 100644
--- a/contrib/btree_gist/btree_int8.c
+++ b/contrib/btree_gist/btree_int8.c
@@ -103,7 +103,7 @@ int8_dist(PG_FUNCTION_ARGS)
 	ra = Abs(r);
 
 	/* Overflow check. */
-	if (ra < 0 || (!SAMESIGN(a, b) && !SAMESIGN(r, a)))
+	if (ra < 0 || PG_INT64_SUB_OVERFLOWS(a, b))
 		ereport(ERROR,
 (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
  errmsg("bigint out of range")));
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 29f058c..9b4b890 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -3185,7 +3185,8 @@ ExecEvalArray(ArrayExprState *astate, ExprContext *econtext,
 /* Get sub-array details from first member */
 elem_ndims = this_ndims;
 ndims = elem_ndims + 1;
-if (ndims <= 0 || ndims > MAXDIM)
+
+if (PG_INT32_ADD_OVERFLOWS(elem_ndims,1) || ndims > MAXDIM)
 	ereport(ERROR,
 			(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 		  errmsg("number of array dimensions (%d) exceeds " \
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index c14ea23..6ad97da 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -113,8 +113,9 @@ array_append(PG_FUNCTION_ARGS)
 		ub = dimv[0] + lb[0] - 1;
 		indx = ub + 1;
 
-		/* overflow? */
-		if (indx < ub)
+		/* check for overflow in upper bound (indx + 1) */
+		if (PG_INT32_ADD_OVERFLOWS(dimv[0] ,lb[0]) ||
+			PG_INT32_ADD_OVERFLOWS(dimv[0] + lb[0], 1))
 			ereport(ERROR,
 	(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
 	 errmsg("integer out of range")));
@@ -168,7 +169,7 @@ array_prepend(PG_FUNCTION_ARGS)
 		lb0 = lb[0];
 
 		/* overflow? */
-		if (indx > lb[0])
+		if (PG_INT32_ADD_OVERFLOWS(lb[0], -1))
 			ereport(ERROR,
 	(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
 	 errmsg("integer out of range")));
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 4e927d8..cc4c570 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2763,12 

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Fri, Oct 16, 2015 at 9:51 AM, Robert Haas  wrote:
>
> On Mon, Oct 5, 2015 at 8:20 AM, Amit Kapila 
wrote:
> > [ new patch for heapam.c changes ]
>
> There's a second patch attached here as well, parallel-relaunch.patch,
> which makes it possible to relaunch workers with the same parallel
> context.  Currently, after you WaitForParallelWorkersToFinish(), you
> must proceed without fail to DestroyParallelContext().  With this
> rather simple patch, you have the option to instead go back and again
> LaunchParallelWorkers(), which is nice because it avoids the overhead
> of setting up a new DSM and filling it with all of your transaction
> state a second time.  I'd like to commit this as well, and I think we
> should revise execParallel.c to use it.
>

I have rebased the partial seq scan patch based on the above committed
patches.  Now for rescanning it reuses the dsm and to achieve that we
need to ensure that workers have been completely shutdown and then
reinitializes the dsm.  To ensure complete shutdown of workers, current
function  WaitForParallelWorkersToFinish is not sufficient as that just
waits for the last message to receive from worker backend, so I have
written a new function WaitForParallelWorkersToDie.  Also on receiving
'X' message in HandleParallelMessage, it just frees the worker handle
without ensuring if the worker is died due to which later it will be
difficult
to even find whether worker is died or not, so I have removed that code
from HandleParallelMessage.  Another change is that after receiving last
tuple in Gather node, it just shutdown down the workers without
destroying the dsm.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


parallel_seqscan_partialseqscan_v22.patch
Description: Binary data

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

Re: [HACKERS] Minor comment fix

[Robert Haas]

On Mon, Oct 19, 2015 at 12:25 AM, Amit Langote
 wrote:
> Attached fixes the following in comment above ExecCheckIndexConstraints:
>
> s/no no conflict/no conflict/g

Thanks, committed.

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


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

[HACKERS] Documentation for min_wal_size and max_wal_size

[Albe Laurenz]

Wouldn't it be better to have these two parameters documented next to each 
other,
as in the attached patch?

Yours,
Laurenz Albe


0001-Move-documentation-for-min_wal_size-before-max_wal_s.patch
Description: 0001-Move-documentation-for-min_wal_size-before-max_wal_s.patch

-- 
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] checkpointer continuous flushing

[Andres Freund]

On 2015-10-19 21:14:55 +0200, Fabien COELHO wrote:
> >In my performance testing it showed that calling PerformFileFlush() only
> >at segment boundaries and in CheckpointWriteDelay() can lead to rather
> >spikey IO - not that surprisingly. The sync in CheckpointWriteDelay() is
> >problematic because it only is triggered while on schedule, and not when
> >behind.
> 
> When behind, the PerformFileFlush should be called on segment
> boundaries.

That means it's flushing up to a gigabyte of data at once. Far too
much. The implementation pretty always will go behind schedule for some
time. Since sync_file_range() doesn't flush in the foreground I don't
think it's important to do the flushing in concert with sleeping.

> >My testing seems to show that just adding a limit of 32 buffers to
> >FileAsynchronousFlush() leads to markedly better results.
> 
> Hmmm. 32 buffers means 256 KB, which is quite small.

Why? The aim is to not overwhelm the request queue - which is where the
coalescing is done. And usually that's rather small. If you flush much more
sync_file_range starts to do work in the foreground.

> >I wonder if mmap() && msync(MS_ASYNC) isn't a better replacement for
> >sync_file_range(SYNC_FILE_RANGE_WRITE) than posix_fadvise(DONTNEED). It
> >might even be possible to later approximate that on windows using
> >FlushViewOfFile().
> 
> I'm not sure that mmap/msync can be used for this purpose, because there is
> no real control it seems about where the file is mmapped.

I'm not following? Why does it matter where a file is mapped?

I have had a friend (Christian Kruse, thanks!)  confirm that at least on
OSX msync(MS_ASYNC) triggers writeback. A freebsd dev confirmed that
that should be the case on freebsd too.

> >Having defined(HAVE_SYNC_FILE_RANGE) || defined(HAVE_POSIX_FADVISE) in
> >so many places looks ugly, I want to push that to the underlying
> >functions. If we add a different flushing approach we shouldn't have to
> >touch several places that don't actually really care.
> 
> I agree that it is pretty ugly, but I do not think that you can remove them
> all.

Sure, never said all. But most.

> >I've replaced the NextBufferToWrite() logic with a binaryheap.h heap -
> >seems to work well, with a bit less code actually.
> 
> Hmmm. I'll check. I'm still unconvinced that using a tree for a 2-3 element
> set in most case is an improvement.

Yes, it'll not matter that much in many cases. But I rather disliked the
NextBufferToWrite() implementation, especially that it walkes the array
multiple times. And I did see setups with ~15 tablespaces.

> >I've also noticed that sleeping logic in CheckpointWriteDelay() isn't
> >particularly good. In high throughput workloads the 100ms sleep is too
> >long, leading to bursty IO behaviour. If 1k+ buffers a written out a
> >second 100ms is a rather long sleep. For another that we only sleep
> >100ms when the write rate is low makes the checkpoint finish rather
> >quickly - on a slow disk (say microsd) that can cause unneccesary
> >slowdowns for concurrent activity.  ISTM we should calculate the sleep
> >time in a better way.
> 
> I also noted this point, but I'm not sure how to have a better approach, so
> I let it as it is. I tried 50 ms & 200 ms on some runs, without significant
> effect on performance for the test I ran then. The point of having not too
> small a value is that it provide some significant work to the IO subsystem
> without overflowing it.

I don't think that makes much sense. All a longer sleep achieves is
creating a larger burst of writes afterwards. We should really sleep
adaptively.


Greetings,

Andres Freund


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

Re: [HACKERS] tab completion for extension versions

[Robert Haas]

On Sun, Oct 18, 2015 at 6:59 PM, Jeff Janes  wrote:
> This patch adds "VERSION" to the list of words completed after "create
> extension foo", and adds the list of available versions of foo after "create
> extension foo version".
>
> There is no point in filtering out the already installed version, as the
> entire statement is doomed already if any version of the extension is
> already installed.

Makes sense to me.  Committed.

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


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

Re: [HACKERS] a raft of parallelism-related bug fixes

[Robert Haas]

On Sat, Oct 17, 2015 at 6:17 PM, Robert Haas  wrote:
> It's good to have your perspective on how this can be improved, and
> I'm definitely willing to write more documentation.  Any lack in that
> area is probably due to being too close to the subject area, having
> spent several years on parallelism in general, and 200+ emails on
> parallel sequential scan in particular.  Your point about the lack of
> a good header file comment for execParallel.c is a good one, and I'll
> rectify that next week.

Here is a patch to add a hopefully-useful file header comment to
execParallel.c.  I included one for nodeGather.c as well, which seems
to be contrary to previous practice, but actually it seems like
previous practice is not the greatest: surely it's not self-evident
what all of the executor nodes do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index 3bb8206..d99e170 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -6,6 +6,14 @@
  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
+ * This file contains routines that are intended to support setting up,
+ * using, and tearing down a ParallelContext from within the PostgreSQL
+ * executor.  The ParallelContext machinery will handle starting the
+ * workers and ensuring that their state generally matches that of the
+ * leader; see src/backend/access/transam/README.parallel for details.
+ * However, we must save and restore relevant executor state, such as
+ * any ParamListInfo associated witih the query, buffer usage info, and
+ * the actual plan to be passed down to the worker.
  *
  * IDENTIFICATION
  *	  src/backend/executor/execParallel.c
diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c
index 7e2272f..017adf2 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -6,6 +6,20 @@
  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
+ * A Gather executor launches parallel workers to run multiple copies of a
+ * plan.  It can also run the plan itself, if the workers are not available
+ * or have not started up yet.  It then merges all of the results it produces
+ * and the results from the workers into a single output stream.  Therefore,
+ * it will normally be used with a plan where running multiple copies of the
+ * same plan does not produce duplicate output, such as PartialSeqScan.
+ *
+ * Alternatively, a Gather node can be configured to use just one worker
+ * and the single-copy flag can be set.  In this case, the Gather node will
+ * run the plan in one worker and will not execute the plan itself.  In
+ * this case, it simply returns whatever tuples were returned by the worker.
+ * If a worker cannot be obtained, then it will run the plan itself and
+ * return the results.  Therefore, a plan used with a single-copy Gather
+ * node not be parallel-aware.
  *
  * IDENTIFICATION
  *	  src/backend/executor/nodeGather.c

-- 
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] SQL function to report log message

[Robert Haas]

On Tue, Oct 20, 2015 at 11:29 AM, Pavel Stehule  wrote:
> 2015-10-20 17:15 GMT+02:00 Robert Haas :
>> On Tue, Oct 20, 2015 at 11:09 AM, Pavel Stehule 
>> wrote:
>> > Probably it was my request. I don't like to using NULL as value, that
>> > should
>> > be ignored. The "hint" is clean, there NULL can be ignored, but what
>> > about
>> > DETAIL or MESSAGE?
>>
>> If the field is required - as MESSAGE is - then its absence is an
>> error.  If the field is optional, treat a NULL if the parameter were
>> not supplied.
>
> I understand well, what was proposed. Personally I see small risk, but I am
> thinking so can be useful if users can choose between two possibilities
> (strict, and NULL tolerant). For some adhoc work it can be useful.

You haven't made any attempt to explain why that behavior would be
useful to anyone except that saying some information might be lost.
But what field of an error report can sensibly be populated with the
word NULL, and nothing else?

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


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

Re: [HACKERS] Multi-column distinctness.

[Tomas Vondra]

On 10/20/2015 05:59 PM, Tom Lane wrote:

Robert Haas  writes:

On Tue, Oct 20, 2015 at 10:51 AM, Tomas Vondra
 wrote:

ISTM that we could use COLLECT STATISTICS instead of ADD STATISTICS, and
use REMOVE STATISTICS instead of DROP STATISTICS. That way we can use
ALTER TABLE rather than inventing a new command. 5 minute change...



That seems like a neat idea, actually. I'm not sure COLLECT is a good choice
as it suggest the statistics is actually built, but that only happens during
ANALYZE. But otherwise this seems to solve the issues with keywords and it's
quite simple.



But ADD is no better there.  I think ALTER TABLE .. COLLECT STATISTICS
isn't any worse than ALTER TABLE ... CLUSTER ON index_name.  In both
cases, it means, when you do this operation, do it this way.



I would suggest that instead of DROP or REMOVE, the opposite should be
ALTER TABLE .. NO COLLECT STATISTICS.


Why is this an improvement over using already-existing keywords?


The problem is that the actual syntax is ADD [COLUMN], so we can't 
simply use ADD STATISTICS as that would mean a conflict in the grammar. 
Resolving it means either making COLUMN non-optional, or adding 
STATISTICS to reserved keywords - both options break existing code.



regards

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


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

Re: [HACKERS] pgbench throttling latency limit

[Andres Freund]

On 2014-08-14 15:01:53 +0200, Fabien COELHO wrote:
> 
> Add --limit to limit latency under throttling
> 
> Under throttling, transactions are scheduled for execution at certain times.
> Transactions may be far behind schedule and the system may catch up with the
> load later. This option allows to change this behavior by skipping
> transactions which are too far behind schedule, and count those as skipped.
> 
> The idea is to help simulate a latency-constrained environment such as a
> database used by a web server.

I was just trying to run tests with this, but as far as I can see it
doesn't really work:

pgbench postgres -M prepared -c 72 -j 72 -P 5 -T 3600 -R4 -L100
...
progress: 240.0 s, 40191.8 tps, lat 1.250 ms stddev 0.965, lag 0.501 ms, 0 
skipped
progress: 245.0 s, 39722.1 tps, lat 1.128 ms stddev 0.946, lag 0.435 ms, 0 
skipped
progress: 250.0 s, 40074.5 tps, lat 1.059 ms stddev 0.745, lag 0.391 ms, 0 
skipped
progress: 255.0 s, 40001.4 tps, lat 1.038 ms stddev 0.680, lag 0.377 ms, 0 
skipped
progress: 260.0 s, 40147.6 tps, lat 1.161 ms stddev 0.950, lag 0.448 ms, 0 
skipped
progress: 265.0 s, 39980.1 tps, lat 1.186 ms stddev 0.862, lag 0.457 ms, 0 
skipped
progress: 270.0 s, 40090.9 tps, lat 1.292 ms stddev 1.239, lag 0.544 ms, 0 
skipped
progress: 275.0 s, 33847.8 tps, lat 26.617 ms stddev 41.681, lag 25.317 ms, 
26698 skipped
progress: 280.0 s, 20237.9 tps, lat 96.041 ms stddev 11.983, lag 92.510 ms, 
97745 skipped
progress: 285.0 s, 24385.0 tps, lat 94.490 ms stddev 10.865, lag 91.514 ms, 
80944 skipped
progress: 290.0 s, 27349.6 tps, lat 92.755 ms stddev 10.905, lag 90.136 ms, 
62268 skipped
progress: 295.0 s, 28382.1 tps, lat 92.752 ms stddev 10.238, lag 90.212 ms, 
58253 skipped
progress: 300.0 s, 28798.3 tps, lat 92.673 ms stddev 10.506, lag 90.172 ms, 
56741 skipped
progress: 305.0 s, 29346.6 tps, lat 91.659 ms stddev 10.982, lag 89.210 ms, 
53163 skipped
progress: 310.0 s, 30072.9 tps, lat 91.190 ms stddev 11.071, lag 88.802 ms, 
48370 skipped
progress: 315.0 s, 30733.2 tps, lat 90.893 ms stddev 11.312, lag 88.548 ms, 
47020 skipped
progress: 320.0 s, 31170.9 tps, lat 89.498 ms stddev 12.132, lag 87.192 ms, 
43403 skipped
progress: 325.0 s, 33399.0 tps, lat 85.795 ms stddev 15.196, lag 83.639 ms, 
32923 skipped
progress: 330.0 s, 22969.8 tps, lat 91.929 ms stddev 14.762, lag 88.805 ms, 
84780 skipped
progress: 335.0 s, 18913.3 tps, lat 95.236 ms stddev 14.523, lag 91.444 ms, 
104960 skipped
progress: 340.0 s, 20061.2 tps, lat 95.258 ms stddev 13.284, lag 91.660 ms, 
100396 skipped
progress: 345.0 s, 20405.3 tps, lat 94.781 ms stddev 13.794, lag 91.255 ms, 
98510 skipped
progress: 350.0 s, 20596.0 tps, lat 94.661 ms stddev 13.345, lag 91.189 ms, 
95426 skipped
progress: 355.0 s, 13635.7 tps, lat 96.998 ms stddev 38.039, lag 91.691 ms, 
132598 skipped
progress: 360.0 s, 16648.0 tps, lat 95.138 ms stddev 26.329, lag 90.809 ms, 
117129 skipped
progress: 365.0 s, 18392.1 tps, lat 94.857 ms stddev 23.917, lag 90.980 ms, 
106244 skipped

100k skipped transactions at a rate limit of 40k? That doesn't seem right.

(that's master server/pgbench as of 5be94a9eb15a)

Regards,

Andres


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

Re: [HACKERS] Declarative partitioning

[Thom Brown]

On 20 October 2015 at 18:34, Amit Langote  wrote:
>
>
> On Wednesday, 21 October 2015, Thom Brown  wrote:
>>
>> On 18 August 2015 at 12:23, Amit Langote  wrote:
>> > Hi Thom,
>> >
>> > On Tue, Aug 18, 2015 at 8:02 PM, Thom Brown  wrote:
>> >>
>> >>
>> >> Wow, didn't expect to see that email this morning.
>> >>
>> >> A very quick test:
>> >>
>> >> CREATE TABLE purchases (purchase_id serial, purchase_time timestamp,
>> >> item
>> >> text) partition by range on ((extract(year from
>> >> purchase_time)),(extract(month from purchase_time)));
>> >> ERROR:  referenced relation "purchases" is not a table or foreign table
>> >>
>> >
>> > Thanks for the quick test.
>> >
>> > Damn, I somehow missed adding the new relkind to a check in
>> > process_owned_by(). Will fix this and look for any such oversights.
>>
>> This doesn't seem to have materialised.  Are you still working on this?
>
>
> Yes, I will be posting to this thread soon. Sorry about the silence.

Thanks.  I'd like to test it again.

Thom


-- 
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] Documentation for min_wal_size and max_wal_size

[Peter Eisentraut]

On 10/20/15 10:27 AM, Albe Laurenz wrote:
> Wouldn't it be better to have these two parameters documented next to each 
> other,
> as in the attached patch?

I was also about to fix that.  I did it slightly differently, to restore
the original alphabetical ordering.



-- 
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] pgbench throttling latency limit

[Andres Freund]

On 2015-10-20 20:55:46 +0200, Andres Freund wrote:
> On 2014-08-14 15:01:53 +0200, Fabien COELHO wrote:
> > 
> > Add --limit to limit latency under throttling
> > 
> > Under throttling, transactions are scheduled for execution at certain times.
> > Transactions may be far behind schedule and the system may catch up with the
> > load later. This option allows to change this behavior by skipping
> > transactions which are too far behind schedule, and count those as skipped.
> > 
> > The idea is to help simulate a latency-constrained environment such as a
> > database used by a web server.
> 
> I was just trying to run tests with this, but as far as I can see it
> doesn't really work:
> 
> pgbench postgres -M prepared -c 72 -j 72 -P 5 -T 3600 -R4 -L100

> progress: 365.0 s, 18392.1 tps, lat 94.857 ms stddev 23.917, lag 90.980 ms, 
> 106244 skipped
> 
> 100k skipped transactions at a rate limit of 40k? That doesn't seem right.

Argh. It's just because I used -P5. It's a bit confusing that the other
options are per second, and this is per interval...

Andres


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

Re: [HACKERS] Declarative partitioning

[Thom Brown]

On 18 August 2015 at 12:23, Amit Langote  wrote:
> Hi Thom,
>
> On Tue, Aug 18, 2015 at 8:02 PM, Thom Brown  wrote:
>>
>>
>> Wow, didn't expect to see that email this morning.
>>
>> A very quick test:
>>
>> CREATE TABLE purchases (purchase_id serial, purchase_time timestamp, item
>> text) partition by range on ((extract(year from
>> purchase_time)),(extract(month from purchase_time)));
>> ERROR:  referenced relation "purchases" is not a table or foreign table
>>
>
> Thanks for the quick test.
>
> Damn, I somehow missed adding the new relkind to a check in
> process_owned_by(). Will fix this and look for any such oversights.

This doesn't seem to have materialised.  Are you still working on this?

Thom


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

[Amit Langote]

On Wednesday, 21 October 2015, Thom Brown  wrote:

> On 18 August 2015 at 12:23, Amit Langote  > wrote:
> > Hi Thom,
> >
> > On Tue, Aug 18, 2015 at 8:02 PM, Thom Brown  > wrote:
> >>
> >>
> >> Wow, didn't expect to see that email this morning.
> >>
> >> A very quick test:
> >>
> >> CREATE TABLE purchases (purchase_id serial, purchase_time timestamp,
> item
> >> text) partition by range on ((extract(year from
> >> purchase_time)),(extract(month from purchase_time)));
> >> ERROR:  referenced relation "purchases" is not a table or foreign table
> >>
> >
> > Thanks for the quick test.
> >
> > Damn, I somehow missed adding the new relkind to a check in
> > process_owned_by(). Will fix this and look for any such oversights.
>
> This doesn't seem to have materialised.  Are you still working on this?
>

Yes, I will be posting to this thread soon. Sorry about the silence.

Thanks,
Amit

Re: [HACKERS] bugs and bug tracking

[Joshua D. Drake]

On 10/13/2015 11:41 AM, Bruce Momjian wrote:


FYI, I think we already have two limits for the first line summary of
commit messages.  The limits are 64 for commit message subjects and 50
characters for gitweb summary pages --- anything longer is truncated.

My commit template shows me the limits as I am typing the commit text to
remind me of the limits:

-- email subject limit -
-- gitweb summary limit --



So where are we at on this?

jD

--
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
New rule for social situations: "If you think to yourself not even
JD would say this..." Stop and shut your mouth. It's going to be bad.


--
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] Multi-column distinctness.

[Tom Lane]

Tomas Vondra  writes:
> On 10/20/2015 05:59 PM, Tom Lane wrote:
>> Why is this an improvement over using already-existing keywords?

> The problem is that the actual syntax is ADD [COLUMN], so we can't 
> simply use ADD STATISTICS as that would mean a conflict in the grammar. 
> Resolving it means either making COLUMN non-optional, or adding 
> STATISTICS to reserved keywords - both options break existing code.

I'm unconvinced that it cannot be made to work.  The proposal was
something like
  ALTER TABLE t ADD STATISTICS ON column-names ...
no?  ON is already fully reserved, which means that this is not ambiguous.
Or if you would rather not use ON, another way of making it not ambiguous
would be to put the column-names list in parentheses.

It's entirely possible that some refactoring of the grammar would
be needed to make it work, of course.

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] [DOCS] max_worker_processes on the standby

[Alvaro Herrera]

Robert Haas wrote:
> On Sat, Oct 17, 2015 at 5:37 PM, Petr Jelinek  wrote:
> > I agree with that sentiment.
> >
> > Attached patch adds variable to the shmem which is used for module
> > activation tracking - set to true in ActiveCommitTs() and false in
> > DeactivateCommitTs(). All the checks inside the commit_ts code were changed
> > to use this new variable. I also removed the static variable Alvaro added in
> > previous commit because it's not needed anymore.
> 
> That sounds good to me.  On a quick read-through it looks OK too.

A revised version is attached.  Two changes on top of Petr's patch:

1. In the two "get" routines, we were reading the flag without grabbing
the lock.  This is okay in a master server, because the flag cannot
change in flight, but in a standby it is possible to have the module
be deactivated while TS data is being queried.  To fix this, simply move
the check for the active shmem flag a few lines down to be inside the
locked section.

There are two other places that also read the flag without grabbing the
lock.  These look okay to me, so I added comments stating so.

2. In TransactionIdGetCommitTsData() we were grabbing lock, reading some
data, releasing lock, then examining the "cached" value in shmem without
a lock to see if it matched the function argument; if it's match, grab
lock again and return the correct data.  In the original coding this
made sense because there was no locked section prior to reading the
cache, but after the patch this was pointless.  Make it simpler by
moving the read of the cache inside the locked section too.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 24b8291..b21a313 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -78,13 +78,21 @@ static SlruCtlData CommitTsCtlData;
 #define CommitTsCtl ()
 
 /*
- * We keep a cache of the last value set in shared memory.  This is protected
- * by CommitTsLock.
+ * We keep a cache of the last value set in shared memory.
+ *
+ * This is also good place to keep the activation status.  We keep this
+ * separate from the GUC so that the standby can activate the module if the
+ * primary has it active independently of the value of the GUC.
+ *
+ * This is protected by CommitTsLock.  In some places, we use commitTsActive
+ * without acquiring the lock; where this happens, a comment explains the
+ * rationale for it.
  */
 typedef struct CommitTimestampShared
 {
 	TransactionId xidLastCommit;
 	CommitTimestampEntry dataLastCommit;
+	bool	commitTsActive;
 } CommitTimestampShared;
 
 CommitTimestampShared *commitTsShared;
@@ -93,14 +101,6 @@ CommitTimestampShared *commitTsShared;
 /* GUC variable */
 bool		track_commit_timestamp;
 
-/*
- * When this is set, commit_ts is force-enabled during recovery.  This is so
- * that a standby can replay WAL records coming from a master with the setting
- * enabled.  (Note that this doesn't enable SQL access to the data; it's
- * effectively write-only until the GUC itself is enabled.)
- */
-static bool		enable_during_recovery;
-
 static void SetXidCommitTsInPage(TransactionId xid, int nsubxids,
 	 TransactionId *subxids, TimestampTz ts,
 	 RepOriginId nodeid, int pageno);
@@ -109,7 +109,7 @@ static void TransactionIdSetCommitTs(TransactionId xid, TimestampTz ts,
 static int	ZeroCommitTsPage(int pageno, bool writeXlog);
 static bool CommitTsPagePrecedes(int page1, int page2);
 static void ActivateCommitTs(void);
-static void DeactivateCommitTs(bool do_wal);
+static void DeactivateCommitTs(void);
 static void WriteZeroPageXlogRec(int pageno);
 static void WriteTruncateXlogRec(int pageno);
 static void WriteSetTimestampXlogRec(TransactionId mainxid, int nsubxids,
@@ -149,10 +149,14 @@ TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
 	TransactionId newestXact;
 
 	/*
-	 * No-op if the module is not enabled, but allow writes in a standby
-	 * during recovery.
+	 * No-op if the module is not active.
+	 *
+	 * An unlocked read here is fine, because in a standby (the only place
+	 * where the flag can change in flight) this routine is only called by
+	 * the recovery process, which is also the only process which can change
+	 * the flag.
 	 */
-	if (!track_commit_timestamp && !enable_during_recovery)
+	if (!commitTsShared->commitTsActive)
 		return;
 
 	/*
@@ -283,30 +287,45 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
 	TransactionId oldestCommitTs;
 	TransactionId newestCommitTs;
 
-	/* Error if module not enabled */
-	if (!track_commit_timestamp)
-		ereport(ERROR,
-(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("could not get commit timestamp data"),
-			  errhint("Make sure the configuration parameter \"%s\" is set.",
-	  "track_commit_timestamp")));
-
 	/* 

Re: [HACKERS] Dangling Client Backend Process

[Robert Haas]

On Tue, Oct 20, 2015 at 12:11 PM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>> I don't think that proc_exit(1) is the right way to exit here.  It's
>> not very friendly to exit without at least attempting to give the
>> client a clue about what has gone wrong.  I suggest something like
>> this:
>>
>> ereport(FATAL,
>> (errcode(ERRCODE_ADMIN_SHUTDOWN),
>>  errmsg("terminating connection due to postmaster shutdown")));
>
> Agreed, but I don't think "shutdown" is the right word to use here --
> that makes it sound like it was orderly.  Perhaps "crash"?

Well, that's a little speculative.  "due to unexpected postmaster exit"?

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


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

Re: [HACKERS] Multi-column distinctness.

[Robert Haas]

On Tue, Oct 20, 2015 at 11:59 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Oct 20, 2015 at 10:51 AM, Tomas Vondra
>>  wrote:
> ISTM that we could use COLLECT STATISTICS instead of ADD STATISTICS, and
> use REMOVE STATISTICS instead of DROP STATISTICS. That way we can use
> ALTER TABLE rather than inventing a new command. 5 minute change...
>
>>> That seems like a neat idea, actually. I'm not sure COLLECT is a good choice
>>> as it suggest the statistics is actually built, but that only happens during
>>> ANALYZE. But otherwise this seems to solve the issues with keywords and it's
>>> quite simple.
>
>> But ADD is no better there.  I think ALTER TABLE .. COLLECT STATISTICS
>> isn't any worse than ALTER TABLE ... CLUSTER ON index_name.  In both
>> cases, it means, when you do this operation, do it this way.
>
>> I would suggest that instead of DROP or REMOVE, the opposite should be
>> ALTER TABLE .. NO COLLECT STATISTICS.
>
> Why is this an improvement over using already-existing keywords?

Well, if we can use existing keywords, that's better still.

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


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

Re: [HACKERS] Dangling Client Backend Process

[Alvaro Herrera]

Robert Haas wrote:
> On Tue, Oct 20, 2015 at 12:11 PM, Alvaro Herrera
>  wrote:

> > Agreed, but I don't think "shutdown" is the right word to use here --
> > that makes it sound like it was orderly.  Perhaps "crash"?
> 
> Well, that's a little speculative.  "due to unexpected postmaster exit"?

WFM.

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


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

Re: [HACKERS] checkpoint_segments upgrade recommendation?

[Peter Eisentraut]

On 10/17/15 10:25 AM, Michael Paquier wrote:
> I think that we should just suggest a reverse formula of the maximum
> soft limit of checkpoint_segments for max_wal_size in the release notes
> of 9.5, basically:
> (3 * your_old_checkpoint_segments + 1) * 16MB = max_wal_size

How about this patch?

(Actually, I'd remove the + 1 to make the numbers come out rounder.)

From ca0ed24ac6a770d637740142a1aed3d76a70a3b3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 20 Oct 2015 14:09:03 -0400
Subject: [PATCH] doc: Add advice on updating checkpoint_segments to
 max_wal_size

---
 doc/src/sgml/release-9.5.sgml | 16 
 1 file changed, 16 insertions(+)

diff --git a/doc/src/sgml/release-9.5.sgml b/doc/src/sgml/release-9.5.sgml
index d5b68e7..37de29d 100644
--- a/doc/src/sgml/release-9.5.sgml
+++ b/doc/src/sgml/release-9.5.sgml
@@ -153,6 +153,22 @@ Migration to Version 9.5
 
 
 
+ 
+  The configuration parameter checkpoint_segments has
+  been removed.  Its place it taken by the new
+  setting .  If you had previously
+  tuned checkpoint_segments, the following formula will
+  give you an approximately equivalent setting:
+
+max_wal_size = (3 * checkpoint_segments + 1) * 16MB
+
+  Note that the default setting for max_wal_size is
+  much higher than the default checkpoint_segments used
+  to be, so setting this might no longer be necessary.
+ 
+
+
+
 
-- 
2.6.1


-- 
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] Multi-column distinctness.

[Tom Lane]

I wrote:
> Tomas Vondra  writes:
>> The problem is that the actual syntax is ADD [COLUMN], so we can't 
>> simply use ADD STATISTICS as that would mean a conflict in the grammar. 
>> Resolving it means either making COLUMN non-optional, or adding 
>> STATISTICS to reserved keywords - both options break existing code.

> I'm unconvinced that it cannot be made to work.

Specifically, this works just fine for me as a variant of alter_table_cmd:

| ADD_P STATISTICS ON '(' name_list ')'

and I can get this to work too

| DROP STATISTICS ON '(' name_list ')'

if I expand out the "DROP opt_column" productions to be two separate
productions with and without COLUMN, as was done long ago for ADD COLUMN.

These also work without ON, actually, though it does not work to leave out
the parens.  (I think that has to do with the conflict against multiple
ALTER TABLE subcommands, not against ADD COLUMN.)

It doesn't look like we could easily stick a "name" in between STATISTICS
and ON, but I'm unconvinced that that's necessary.  Can't we just insist
that there be only one statistics declaration for a given column list?

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] SQL function to report log message

[Robert Haas]

On Tue, Oct 20, 2015 at 11:09 AM, Pavel Stehule  wrote:
> Probably it was my request. I don't like to using NULL as value, that should
> be ignored. The "hint" is clean, there NULL can be ignored, but what about
> DETAIL or MESSAGE?

If the field is required - as MESSAGE is - then its absence is an
error.  If the field is optional, treat a NULL if the parameter were
not supplied.

> I am strong in my opinion about PLpgSQL RAISE statement behave, but on
> second hand, proposed function should not be 100% same as RAISE stmt. More
> we can simply add a parameter like "ignore_nulls"

I would be willing to bet you a drink that 99.9% of people will want
the behavior Jim is advocating, so I don't think this should be
configurable.

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


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

Re: [HACKERS] ROWS FROM(): A Foolish (In)Consistency?

[Robert Haas]

On Tue, Oct 20, 2015 at 11:03 AM, David Fetter  wrote:
> On Tue, Oct 20, 2015 at 10:52:05AM -0400, Robert Haas wrote:
>> On Mon, Oct 19, 2015 at 8:02 PM, Jim Nasby  wrote:
>> > On 10/19/15 1:07 PM, David Fetter wrote:
>> >>
>> >> What I'd like to do is lift the restriction on ROWS FROM(), which
>> >> currently requires that the stuff inside the parentheses set-returning
>> >> functions, so constructs something like the following would actually work:
>> >>
>> >> SELECT *
>> >> FROM
>> >> ROWS FROM (
>> >>  (VALUES (...), ..., (...)),
>> >>  (SELECT ... ),
>> >>  (INSERT ... RETURNING ... ),
>> >>  my_srf()
>> >> )
>> >> AS t(...)
>> >>
>> >> would actually work.
>> >
>> >
>> > There's been a few places where I would have found that handy.
>>
>> Why not just use a subquery with UNION ALL?
>
> Because UNION ALL glues the queries vertically, not horizontally.

Ah.  I get it now.  Thanks for clarifying.

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


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

Re: [HACKERS] Multi-column distinctness.

[Tom Lane]

Robert Haas  writes:
> On Tue, Oct 20, 2015 at 10:51 AM, Tomas Vondra
>  wrote:
 ISTM that we could use COLLECT STATISTICS instead of ADD STATISTICS, and
 use REMOVE STATISTICS instead of DROP STATISTICS. That way we can use
 ALTER TABLE rather than inventing a new command. 5 minute change...

>> That seems like a neat idea, actually. I'm not sure COLLECT is a good choice
>> as it suggest the statistics is actually built, but that only happens during
>> ANALYZE. But otherwise this seems to solve the issues with keywords and it's
>> quite simple.

> But ADD is no better there.  I think ALTER TABLE .. COLLECT STATISTICS
> isn't any worse than ALTER TABLE ... CLUSTER ON index_name.  In both
> cases, it means, when you do this operation, do it this way.

> I would suggest that instead of DROP or REMOVE, the opposite should be
> ALTER TABLE .. NO COLLECT STATISTICS.

Why is this an improvement over using already-existing keywords?

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] [COMMITTERS] pgsql: Tab complete CREATE EXTENSION .. VERSION.

[David Fetter]

On Tue, Oct 20, 2015 at 02:28:15PM +, Robert Haas wrote:
> Tab complete CREATE EXTENSION .. VERSION.
> 
> Jeff Janes

Would this be worth back-patching to 9.5?  It seems like a pretty
isolated change with a pretty helpful utility.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

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


-- 
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] Multi-column distinctness.

[Robert Haas]

On Tue, Oct 20, 2015 at 10:51 AM, Tomas Vondra
 wrote:
>> Koyotaro's changes to force COLUMN to be required won't get through.
>>
>> ISTM that we could use COLLECT STATISTICS instead of ADD STATISTICS, and
>> use REMOVE STATISTICS instead of DROP STATISTICS. That way we can use
>> ALTER TABLE rather than inventing a new command. 5 minute change...
>
> That seems like a neat idea, actually. I'm not sure COLLECT is a good choice
> as it suggest the statistics is actually built, but that only happens during
> ANALYZE. But otherwise this seems to solve the issues with keywords and it's
> quite simple.

But ADD is no better there.  I think ALTER TABLE .. COLLECT STATISTICS
isn't any worse than ALTER TABLE ... CLUSTER ON index_name.  In both
cases, it means, when you do this operation, do it this way.

I would suggest that instead of DROP or REMOVE, the opposite should be
ALTER TABLE .. NO COLLECT STATISTICS.

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


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

Re: [HACKERS] Typos in plannodes.h

[Robert Haas]

On Tue, Oct 20, 2015 at 7:45 AM, Etsuro Fujita
 wrote:
> I found typos in plannodes.h: s/scan.plan.quals/scan.plan.qual/g  Please
> find attached a patch.

Oops.  Good catch.  Committed.

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


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

Re: [HACKERS] Multi-column distinctness.

[Simon Riggs]

On 20 October 2015 at 11:59, Tom Lane  wrote:

> Robert Haas  writes:
> > On Tue, Oct 20, 2015 at 10:51 AM, Tomas Vondra
> >  wrote:
>  ISTM that we could use COLLECT STATISTICS instead of ADD STATISTICS,
> and
>  use REMOVE STATISTICS instead of DROP STATISTICS. That way we can use
>  ALTER TABLE rather than inventing a new command. 5 minute change...
>
> >> That seems like a neat idea, actually. I'm not sure COLLECT is a good
> choice
> >> as it suggest the statistics is actually built, but that only happens
> during
> >> ANALYZE. But otherwise this seems to solve the issues with keywords and
> it's
> >> quite simple.
>
> > But ADD is no better there.  I think ALTER TABLE .. COLLECT STATISTICS
> > isn't any worse than ALTER TABLE ... CLUSTER ON index_name.  In both
> > cases, it means, when you do this operation, do it this way.
>
> > I would suggest that instead of DROP or REMOVE, the opposite should be
> > ALTER TABLE .. NO COLLECT STATISTICS.
>
> Why is this an improvement over using already-existing keywords?
>

The earlier patch changed the grammar for the DROP (column) subcommand,
which I am saying is not acceptable.

So by using an alternate keyword we are able to keep the existing syntax
untouched.

I suggested the word COLLECT since that is another word commonly used in
conjunction with STATISTICS (at least in DB2 and Teradata).

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

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

Re: [HACKERS] ROWS FROM(): A Foolish (In)Consistency?

[David Fetter]

On Tue, Oct 20, 2015 at 11:16:13AM -0400, Robert Haas wrote:
> On Tue, Oct 20, 2015 at 11:03 AM, David Fetter  wrote:
> > On Tue, Oct 20, 2015 at 10:52:05AM -0400, Robert Haas wrote:
> >> On Mon, Oct 19, 2015 at 8:02 PM, Jim Nasby  
> >> wrote:
> >> > On 10/19/15 1:07 PM, David Fetter wrote:
> >> >>
> >> >> What I'd like to do is lift the restriction on ROWS FROM(), which
> >> >> currently requires that the stuff inside the parentheses set-returning
> >> >> functions, so constructs something like the following would actually 
> >> >> work:
> >> >>
> >> >> SELECT *
> >> >> FROM
> >> >> ROWS FROM (
> >> >>  (VALUES (...), ..., (...)),
> >> >>  (SELECT ... ),
> >> >>  (INSERT ... RETURNING ... ),
> >> >>  my_srf()
> >> >> )
> >> >> AS t(...)
> >> >>
> >> >> would actually work.
> >> >
> >> >
> >> > There's been a few places where I would have found that handy.
> >>
> >> Why not just use a subquery with UNION ALL?
> >
> > Because UNION ALL glues the queries vertically, not horizontally.
> 
> Ah.  I get it now.  Thanks for clarifying.

Sorry that didn't start out clearer.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

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


-- 
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] Dangling Client Backend Process

[Alvaro Herrera]

Robert Haas wrote:

> I don't think that proc_exit(1) is the right way to exit here.  It's
> not very friendly to exit without at least attempting to give the
> client a clue about what has gone wrong.  I suggest something like
> this:
> 
> ereport(FATAL,
> (errcode(ERRCODE_ADMIN_SHUTDOWN),
>  errmsg("terminating connection due to postmaster shutdown")));

Agreed, but I don't think "shutdown" is the right word to use here --
that makes it sound like it was orderly.  Perhaps "crash"?

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


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

Re: [HACKERS] [PATCH] SQL function to report log message

[Pavel Stehule]

2015-10-20 17:15 GMT+02:00 Robert Haas :

> On Tue, Oct 20, 2015 at 11:09 AM, Pavel Stehule 
> wrote:
> > Probably it was my request. I don't like to using NULL as value, that
> should
> > be ignored. The "hint" is clean, there NULL can be ignored, but what
> about
> > DETAIL or MESSAGE?
>
> If the field is required - as MESSAGE is - then its absence is an
> error.  If the field is optional, treat a NULL if the parameter were
> not supplied.
>

I understand well, what was proposed. Personally I see small risk, but I am
thinking so can be useful if users can choose between two possibilities
(strict, and NULL tolerant). For some adhoc work it can be useful.


>
> > I am strong in my opinion about PLpgSQL RAISE statement behave, but on
> > second hand, proposed function should not be 100% same as RAISE stmt.
> More
> > we can simply add a parameter like "ignore_nulls"
>
> I would be willing to bet you a drink that 99.9% of people will want
> the behavior Jim is advocating, so I don't think this should be
> configurable.
>

99.9% of people can think so it is good idea to the moment, when the
important information will be lost without any hint, why it was lost.

Default behave can be like Jim proposed.




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

[HACKERS] [PATCH] Typos in comments

[CharSyam]

I fixed some typos in posgres.
They are all in comments. :)

Thanks


PATCH.typos
Description: Binary data

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

Re: [HACKERS] Multi-column distinctness.

[Tomas Vondra]

Hi,

On 10/20/2015 09:11 PM, Tom Lane wrote:

I wrote:

Tomas Vondra  writes:

The problem is that the actual syntax is ADD [COLUMN], so we
can't simply use ADD STATISTICS as that would mean a conflict in
the grammar. Resolving it means either making COLUMN
non-optional, or adding STATISTICS to reserved keywords - both
options break existing code.



I'm unconvinced that it cannot be made to work.


Specifically, this works just fine for me as a variant of
alter_table_cmd:

| ADD_P STATISTICS ON '(' name_list ')'

and I can get this to work too

| DROP STATISTICS ON '(' name_list ')'

if I expand out the "DROP opt_column" productions to be two separate
productions with and without COLUMN, as was done long ago for ADD
COLUMN.

These also work without ON, actually, though it does not work to
leave out the parens. (I think that has to do with the conflict
against multiple ALTER TABLE subcommands, not against ADD COLUMN.)

It doesn't look like we could easily stick a "name" in between
STATISTICS and ON, but I'm unconvinced that that's necessary.


Well, it's definitely easier to reference the statistics by name (e.g. 
in the DROP command).



Can't we just insist that there be only one statistics declaration
for a given column list?


I would rather not, because I envision preferring different types of 
statistics for different types of queries. For example the statistics 
discussed in this particular thread only supports equality clauses, so 
this restriction would mean we can't also define histogram, we'll be 
unable to estimate queries with ranges.


regards

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


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

Re: [HACKERS] [PATCH] SQL function to report log message

[Pavel Stehule]

2015-10-20 20:05 GMT+02:00 Robert Haas :

> On Tue, Oct 20, 2015 at 11:29 AM, Pavel Stehule 
> wrote:
> > 2015-10-20 17:15 GMT+02:00 Robert Haas :
> >> On Tue, Oct 20, 2015 at 11:09 AM, Pavel Stehule <
> pavel.steh...@gmail.com>
> >> wrote:
> >> > Probably it was my request. I don't like to using NULL as value, that
> >> > should
> >> > be ignored. The "hint" is clean, there NULL can be ignored, but what
> >> > about
> >> > DETAIL or MESSAGE?
> >>
> >> If the field is required - as MESSAGE is - then its absence is an
> >> error.  If the field is optional, treat a NULL if the parameter were
> >> not supplied.
> >
> > I understand well, what was proposed. Personally I see small risk, but I
> am
> > thinking so can be useful if users can choose between two possibilities
> > (strict, and NULL tolerant). For some adhoc work it can be useful.
>
> You haven't made any attempt to explain why that behavior would be
> useful to anyone except that saying some information might be lost.
> But what field of an error report can sensibly be populated with the
> word NULL, and nothing else?
>

My previous idea was wrong (I didn't though well about all details). I am
sorry. The implementation of variadic parameters in Postgres requires some
default value - in this case the only one logical default value is NULL.
And in this case, when the default is used, the NULL shouldn't be
displayed. I propose following behave. The level and the message arguments
are mandatory (has not default value), others are optional. The level is
should not be NULL, the message can be NULL, and the NULL should be
displayed, any others are ignored if holds NULL.  A alternative is - only
the level will be mandatory, others will be optional, and then there are
not any exception for message.

Regards

Pavel


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

Re: [HACKERS] Multi-column distinctness.

[Simon Riggs]

On 20 October 2015 at 16:48, Tomas Vondra 
wrote:


> and I can get this to work too
>>
>> | DROP STATISTICS ON '(' name_list ')'
>>
>> if I expand out the "DROP opt_column" productions to be two separate
>> productions with and without COLUMN, as was done long ago for ADD
>> COLUMN.
>>
>> These also work without ON, actually, though it does not work to
>> leave out the parens. (I think that has to do with the conflict
>> against multiple ALTER TABLE subcommands, not against ADD COLUMN.)
>>
>> It doesn't look like we could easily stick a "name" in between
>> STATISTICS and ON, but I'm unconvinced that that's necessary.
>>
>
> Well, it's definitely easier to reference the statistics by name (e.g. in
> the DROP command).
>
> Can't we just insist that there be only one statistics declaration
>> for a given column list?
>>
>
> I would rather not, because I envision preferring different types of
> statistics for different types of queries. For example the statistics
> discussed in this particular thread only supports equality clauses, so this
> restriction would mean we can't also define histogram, we'll be unable to
> estimate queries with ranges.


Can we do something like this...

ADD STATISTICS ON (col list) USING (histogram, MFV)

so we have Types/Methods of statistic, rather than specific names for the
statistic entry?

Since this command doesn't actually ADD the statistics, it just creates a
specification used by the next ANALYZE, it would seem better to use a
different keyword than ADD, perhaps DECLARE STATISTICS ON... and DROP
STATISTICS ON

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

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

Re: [HACKERS] bugs and bug tracking

[Stephen Frost]

* Joshua D. Drake (j...@commandprompt.com) wrote:
> On 10/13/2015 11:41 AM, Bruce Momjian wrote:
> 
> >FYI, I think we already have two limits for the first line summary of
> >commit messages.  The limits are 64 for commit message subjects and 50
> >characters for gitweb summary pages --- anything longer is truncated.
> >
> >My commit template shows me the limits as I am typing the commit text to
> >remind me of the limits:
> >
> >-- email subject limit -
> >-- gitweb summary limit --
> 
> So where are we at on this?

On the line summary limits..?  Or the general bug tracking?

With regard to bug tracking, as I'm guessing that's what you were
getting at with the query, I have the VM built and the BTS software
mostly installed, but I don't expect to get back to working on it before
PGConf.EU, which is next week.  I need to set up the DNS and the email
routing along with general testing; perhaps I'll get that done while in
Austria or the week after.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

[Jim Nasby]

On 10/4/15 6:10 PM, Tom Lane wrote:

Andrew Dunstan  writes:

>Sorry, I'm a bit late to this party. Does what you have committed mean
>people are less likely to see "Out of Memory" coming from
>pg_stat_statements? If not, what can be done about them short of a
>restart? And what bad effects follow from an event generating them?

The main thing we've done that will alleviate that is increase the size of
query text file that the garbage-collection routine can cope with from
MaxAllocSize (1GB) to MaxAllocHugeSize (at least 2GB, lots more on 64bit
machines, though on 32-bit you probably can't get to 2GB anyway ...).


FWIW, I've verified on $CLIENT's system that this works as Tom 
described. The truncation happened somewhere a bit north of 3GB, which 
seems odd as this is a 64 bit system. But at least there were no OOM errors.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
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] Typos in comments

[Jim Nasby]

On 10/20/15 11:08 AM, CharSyam wrote:

I fixed some typos in posgres.
They are all in comments. :)


These all look good to me. RFC.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
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] Improvements of Hunspell dictionaries support

[Jim Nasby]

On 10/20/15 9:00 AM, Artur Zakirov wrote:

Internal representation of the dictionary in the PostgreSQL doesn't
impose too strict limits on the number of affix rules. There are a
flagval array, which size must be increased from 256 to 65000.


Is that per dictionary entry, fixed at 64k? That seems pretty excessive, 
if that's the case...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
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] Why no CONSTANT for row variables in plpgsql?

[Jim Nasby]

On 10/19/15 7:12 PM, Tom Lane wrote:

Jim Nasby  writes:

What did seem odd is that while processing the DECLARE section there
were plpgsql datums for tt.a and tt.b. I would have expected the
assignment to produce a row datum of type tt.


Yeah, that's the thing that's weird about plpgsql's ROW datums.

What the row datum mechanism is actually good for IMO is representing
multiple targets for FOR and INTO constructs, ie
 SELECT ... INTO a,b,c;
If you look at the representation of INTO, it only allows one target
datum, and the reason that's OK is it uses a row datum for cases like
this.  The row member datums are just the scalar variables a,b,c,
which can also be accessed directly.

IMO, we ought to get rid of the use of that representation for
composite-type variables and use the RECORD code paths for them,
whether they are declared as type record or as named composite
types.  That would probably make it easier to handle this case,
and it'd definitely make it easier to deal with some other weak
spots like ALTER TYPE changes to composite types.  However, last
time I proposed that, it was shot down on the grounds that it might
hurt performance in some cases.  (Which is likely true, although
that argument ignores the fact that other cases might get better.)


That also means there would only need to be changes to RECORD to allow 
CONSTANT, default, etc.


Do you know offhand what the starting point for changing that would be? 
build_datatype()?

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
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] a raft of parallelism-related bug fixes

[Simon Riggs]

On 17 October 2015 at 18:17, Robert Haas  wrote:


> It's good to have your perspective on how this can be improved, and
> I'm definitely willing to write more documentation.  Any lack in that
> area is probably due to being too close to the subject area, having
> spent several years on parallelism in general, and 200+ emails on
> parallel sequential scan in particular.  Your point about the lack of
> a good header file comment for execParallel.c is a good one, and I'll
> rectify that next week.
>

Not on your case in a big way, just noting the need for change there.

I'll help as well, but if you could start with enough basics to allow me to
ask questions that will help. Thanks.

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

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

Re: [HACKERS] SuperUser check in pg_stat_statements

[Lukas Fittl]

Rajan,

I'll reply off-list since this isn't the right discussion for -hackers.

Best,
Lukas

On Tue, Oct 20, 2015 at 7:02 AM, rajan  wrote:

> Hey Lukas,
>
> Thanks. Able to see the queries from all users. Can you explain the
> monitoring.get_stat_statements()?
>
>
>
> --
> View this message in context:
> http://postgresql.nabble.com/SuperUser-check-in-pg-stat-statements-tp5870589p5870733.html
> Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Lukas Fittl

Skype: lfittl
Phone: +1 415 321 0630

Re: [HACKERS] [PATCH] SQL function to report log message

[Robert Haas]

On Mon, Oct 19, 2015 at 7:59 PM, Jim Nasby  wrote:
> I fail to see how doing
>
> HINT: NULL
>
> is much better than just not raising a HINT at all...

I'm not a huge fan of this patch, as previously noted, but I certainly
agree that if we're going to do it, we should ignore a null argument,
not print out the word "NULL".  Who would ever want that behavior?

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


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

Re: [HACKERS] ROWS FROM(): A Foolish (In)Consistency?

[Robert Haas]

On Mon, Oct 19, 2015 at 8:02 PM, Jim Nasby  wrote:
> On 10/19/15 1:07 PM, David Fetter wrote:
>>
>> What I'd like to do is lift the restriction on ROWS FROM(), which
>> currently requires that the stuff inside the parentheses set-returning
>> functions, so constructs something like the following would actually work:
>>
>> SELECT *
>> FROM
>> ROWS FROM (
>>  (VALUES (...), ..., (...)),
>>  (SELECT ... ),
>>  (INSERT ... RETURNING ... ),
>>  my_srf()
>> )
>> AS t(...)
>>
>> would actually work.
>
>
> There's been a few places where I would have found that handy.

Why not just use a subquery with UNION ALL?

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


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

Re: [HACKERS] Multi-column distinctness.

[Tomas Vondra]

Hi,

On 10/20/2015 01:14 PM, Simon Riggs wrote:

On 19 October 2015 at 20:16, Tomas Vondra > wrote:

Hello Kyotaro-san,

On 09/11/2015 06:58 PM, Tomas Vondra wrote:
>

Maybe the best solution is to abandon the ALTER TABLE approach
entirely, and instead invent a new set of commands

CREATE STATISTICS
DROP STATISTICS

(ALTER STATISTICS seems a bit excessive at this point).

Another thing is that perhaps we should add names for statistics,
just like we do for constraints, for example. Otherwise the DROP
STATISTICS handling is rather awkward - for example if the user
creates stats twice by mistake, he's unable to drop just one of
them.


Do you think this modified syntax makes sense? I'll have time to
hack on this over the next few days.


Koyotaro's changes to force COLUMN to be required won't get through.

ISTM that we could use COLLECT STATISTICS instead of ADD STATISTICS, and
use REMOVE STATISTICS instead of DROP STATISTICS. That way we can use
ALTER TABLE rather than inventing a new command. 5 minute change...


That seems like a neat idea, actually. I'm not sure COLLECT is a good 
choice as it suggest the statistics is actually built, but that only 
happens during ANALYZE. But otherwise this seems to solve the issues 
with keywords and it's quite simple.




Unless there is some better reason for a whole new command?


Not really. The other proposal (adding names for statistics) does not 
require new command. The one thing that would require new command is 
building statistics on multiple tables (for join estimation), but I 
don't have any idea of how that would actually work.


regards

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


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

Re: [HACKERS] Dangling Client Backend Process

[Robert Haas]

On Tue, Oct 20, 2015 at 12:48 AM, Rajeev rastogi
 wrote:
> On  19 October 2015 21:37, Robert Haas [mailto:robertmh...@gmail.com] Wrote:
>
>>On Sat, Oct 17, 2015 at 4:52 PM, Alvaro Herrera
>> wrote:
>>> Andres Freund wrote:
 On 2015-10-14 17:33:01 +0900, Kyotaro HORIGUCHI wrote:
 > If I recall correctly, he concerned about killing the backends
 > running transactions which could be saved. I have a sympathy with
 > the opinion.

 I still don't. Leaving backends alive after postmaster has died
 prevents the auto-restart mechanism to from working from there on.
 Which means that we'll potentially continue happily after another
 backend has PANICed and potentially corrupted shared memory. Which
 isn't all that unlikely if postmaster isn't around anymore.
>>>
>>> I agree.  When postmaster terminates without waiting for all backends
>>> to go away, things are going horribly wrong -- either a DBA has done
>>> something stupid, or the system is misbehaving.  As Andres says, if
>>> another backend dies at that point, things are even worse -- the dying
>>> backend could have been holding a critical lwlock, for instance, or it
>>> could have corrupted shared buffers on its way out.  It is not
>>> sensible to leave the rest of the backends in the system still trying
>>> to run just because there is no one there to kill them.
>>
>>Yep.  +1 for changing this.
>
> Seems many people are in favor of this change.
> I have made changes to handle backend exit on postmaster death (after they 
> finished their work and waiting for new command).
> Changes are as per approach explained in my earlier mail i.e.
> 1. WaitLatchOrSocket called from secure_read and secure_write function will 
> wait on an additional event as WL_POSTMASTER_DEATH.
> 2. There is a possibility that the command is read without waiting on latch. 
> This case is handled by checking postmaster status after command read (i.e. 
> after ReadCommand).
>
> Attached is the patch.

I don't think that proc_exit(1) is the right way to exit here.  It's
not very friendly to exit without at least attempting to give the
client a clue about what has gone wrong.  I suggest something like
this:

ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
 errmsg("terminating connection due to postmaster shutdown")));

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


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

Re: [HACKERS] ROWS FROM(): A Foolish (In)Consistency?

[David Fetter]

On Tue, Oct 20, 2015 at 10:52:05AM -0400, Robert Haas wrote:
> On Mon, Oct 19, 2015 at 8:02 PM, Jim Nasby  wrote:
> > On 10/19/15 1:07 PM, David Fetter wrote:
> >>
> >> What I'd like to do is lift the restriction on ROWS FROM(), which
> >> currently requires that the stuff inside the parentheses set-returning
> >> functions, so constructs something like the following would actually work:
> >>
> >> SELECT *
> >> FROM
> >> ROWS FROM (
> >>  (VALUES (...), ..., (...)),
> >>  (SELECT ... ),
> >>  (INSERT ... RETURNING ... ),
> >>  my_srf()
> >> )
> >> AS t(...)
> >>
> >> would actually work.
> >
> >
> > There's been a few places where I would have found that handy.
> 
> Why not just use a subquery with UNION ALL?

Because UNION ALL glues the queries vertically, not horizontally.
ROWS FROM() turns things like:

A()
--
1
2
3
4
5

and

B()
--
a10
b9
c8
d7
e6
f5
g4

into

ROWS FROM(A(), B())
---
1a   10
2b   9
3c   8
4d   7
5e   6
NULL f   5
NULL g   4

UNION ALL turns combining A() and B() into an error because the output
row types don't match.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

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


-- 
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] SQL function to report log message

[Pavel Stehule]

2015-10-20 16:50 GMT+02:00 Robert Haas :

> On Mon, Oct 19, 2015 at 7:59 PM, Jim Nasby 
> wrote:
> > I fail to see how doing
> >
> > HINT: NULL
> >
> > is much better than just not raising a HINT at all...
>
> I'm not a huge fan of this patch, as previously noted, but I certainly
> agree that if we're going to do it, we should ignore a null argument,
> not print out the word "NULL".  Who would ever want that behavior?
>

Probably it was my request. I don't like to using NULL as value, that
should be ignored. The "hint" is clean, there NULL can be ignored, but what
about DETAIL or MESSAGE?

I am strong in my opinion about PLpgSQL RAISE statement behave, but on
second hand, proposed function should not be 100% same as RAISE stmt. More
we can simply add a parameter like "ignore_nulls"

Regards

Pavel



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

Re: [HACKERS] Why no CONSTANT for row variables in plpgsql?

[Tom Lane]

Jim Nasby  writes:
> On 10/19/15 7:12 PM, Tom Lane wrote:
>> IMO, we ought to get rid of the use of that representation for
>> composite-type variables and use the RECORD code paths for them,

> That also means there would only need to be changes to RECORD to allow 
> CONSTANT, default, etc.

> Do you know offhand what the starting point for changing that would be? 
> build_datatype()?

Well, definitely build_datatype would want to select PLPGSQL_TTYPE_REC not
PLPGSQL_TTYPE_ROW when seeing TYPTYPE_COMPOSITE.  I suspect that's just a
small tip of a large iceberg, though.

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] Multi-column distinctness.

[Tomas Vondra]

On 10/20/2015 11:28 PM, Simon Riggs wrote:

On 20 October 2015 at 16:48, Tomas Vondra > wrote:

>> On 10/20/2015 09:11 PM, Tom Lane wrote:


Can't we just insist that there be only one statistics declaration
for a given column list? >>


I would rather not, because I envision preferring different types of
statistics for different types of queries. For example the statistics
discussed in this particular thread only supports equality clauses,
so this restriction would mean we can't also define histogram, we'll
be unable to estimate queries with ranges.


Can we do something like this...

ADD STATISTICS ON (col list) USING (histogram, MFV)
so we have Types/Methods of statistic, rather than specific names for
the statistic entry?


That's how it works now (comparing columns and types of stats), but I 
find it awkward. That's why I proposed adding the name.




Since this command doesn't actually ADD the statistics, it just creates
a specification used by the next ANALYZE, it would seem better to use a
different keyword than ADD, perhaps DECLARE STATISTICS ON... and DROP
STATISTICS ON


Maybe, although we should not use DROP with DECLARE. Not only DROP has 
the same issue with name as ADD, but I think it's a good practice to 
commands "paired" ADD-DROP and DECLARE-X?


regards

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


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

Re: [HACKERS] Freeze avoidance of very large table.

[Masahiko Sawada]

On Sat, Oct 10, 2015 at 4:20 AM, Robert Haas  wrote:
> On Thu, Oct 8, 2015 at 1:52 PM, Andres Freund  wrote:
>> I don't see the problem? I mean catversion will reliably tell you which 
>> format the vm is in?
>
> Totally agreed.
>
>> We could additionally use the opportunity to as a metapage, but that seems 
>> like an independent thing.
>
> I agree with that, too.
>

Attached the updated v18 patch fixes some bugs.
Please review the patch.

Regards,

--
Masahiko Sawada
diff --git a/contrib/pgstattuple/pgstatapprox.c b/contrib/pgstattuple/pgstatapprox.c
index 22c5f7a..b1b6a06 100644
--- a/contrib/pgstattuple/pgstatapprox.c
+++ b/contrib/pgstattuple/pgstatapprox.c
@@ -87,7 +87,7 @@ statapprox_heap(Relation rel, output_type *stat)
 		 * If the page has only visible tuples, then we can find out the free
 		 * space from the FSM and move on.
 		 */
-		if (visibilitymap_test(rel, blkno, ))
+		if (visibilitymap_test(rel, blkno, , VISIBILITYMAP_ALL_VISIBLE))
 		{
 			freespace = GetRecordedFreeSpace(rel, blkno);
 			stat->tuple_len += BLCKSZ - freespace;
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 97ef618..f8aa18b 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1841,6 +1841,18 @@
  
 
  
+   relallfrozen
+   int4
+   
+   
+Number of pages that are marked all-frozen in the tables's
+visibility map. It is updated by VACUUM.
+ANALYZE, and a few DDL coomand such as
+CREATE INDEX.
+   
+ 
+
+ 
   reltoastrelid
   oid
   pg_class.oid
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 5549de7..bb63bb9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5900,7 +5900,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
   

-VACUUM performs a whole-table scan if the table's
+VACUUM performs a aggressive freezing if the table's
 pg_class.relfrozenxid field has reached
 the age specified by this setting.  The default is 150 million
 transactions.  Although users can set this value anywhere from zero to
@@ -5944,7 +5944,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
   

-VACUUM performs a whole-table scan if the table's
+VACUUM performs a aggressive freezing if the table's
 pg_class.relminmxid field has reached
 the age specified by this setting.  The default is 150 million multixacts.
 Although users can set this value anywhere from zero to two billions,
diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml
index b5d4050..c8ad27f 100644
--- a/doc/src/sgml/maintenance.sgml
+++ b/doc/src/sgml/maintenance.sgml
@@ -352,9 +352,9 @@
 Vacuum maintains a visibility map for each
 table to keep track of which pages contain only tuples that are known to be
 visible to all active transactions (and all future transactions, until the
-page is again modified).  This has two purposes.  First, vacuum
-itself can skip such pages on the next run, since there is nothing to
-clean up.
+page is again modified), and pages contain only tuples that are marked as
+frozen.  This has two purposes.  First, vacuum itself can skip such pages
+on the next run, since there is nothing to clean up.

 

@@ -438,23 +438,22 @@

 

-VACUUM normally skips pages that don't have any dead row
-versions, but those pages might still have row versions with old XID
-values.  To ensure all old row versions have been frozen, a
-scan of the whole table is needed.
+VACUUM skips pages that don't have any dead row
+versions, and pages that have only frozen rows.
+To ensure all old row versions have been frozen, a scan of all pages that
+are not marked as frozen is needed.
  controls when
-VACUUM does that: a whole table sweep is forced if
-the table hasn't been fully scanned for vacuum_freeze_table_age
-minus vacuum_freeze_min_age transactions. Setting it to 0
-forces VACUUM to always scan all pages, effectively ignoring
-the visibility map.
+VACUUM does that: a table sweep is forced if
+the table hasn't been ensured all row versions are frozen for
+vacuum_freeze_table_age minus vacuum_freeze_min_age
+transcations.

 

 The maximum time that a table can go unvacuumed is two billion
 transactions minus the vacuum_freeze_min_age value at
-the time VACUUM last scanned the whole table.  If it were to go
-unvacuumed for longer than
+the time VACUUM last scanned pages that are not marked as frozen
+If it were to go unvacuumed for longer than
 that, data loss could result.  To ensure that this does not happen,
 autovacuum is invoked on any table that might contain unfrozen rows with
 XIDs older than the age specified by the 

Re: [HACKERS] Freeze avoidance of very large table.

[Simon Riggs]

On 9 October 2015 at 15:20, Robert Haas  wrote:

> On Thu, Oct 8, 2015 at 1:52 PM, Andres Freund  wrote:
> > I don't see the problem? I mean catversion will reliably tell you which
> format the vm is in?
>
> Totally agreed.
>

This isn't an agreement competition, its a cool look at what might cause
problems for all of us.

If we want to avoid bugs in future then we'd better start acting like that
is actually true in practice.

Why should we wave away this concern? Will we wave away a concern next time
you personally raise one? Bruce would have me believe that we added months
onto 9.5 to improve robustness. So lets actually do that. Starting at the
first opportunity.

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

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

Re: [HACKERS] Multi-column distinctness.

[Tom Lane]

Tomas Vondra  writes:
> On 10/20/2015 11:28 PM, Simon Riggs wrote:
>> Since this command doesn't actually ADD the statistics, it just creates
>> a specification used by the next ANALYZE, it would seem better to use a
>> different keyword than ADD, perhaps DECLARE STATISTICS ON... and DROP
>> STATISTICS ON

> Maybe, although we should not use DROP with DECLARE. Not only DROP has 
> the same issue with name as ADD, but I think it's a good practice to 
> commands "paired" ADD-DROP and DECLARE-X?

If we spelled the drop case as "DROP STATISTICS name", I think the
conflict could be avoided.  I agree though that pairing DECLARE and DROP
seems rather unintuitive.

I am getting more attracted to your suggestion of making these things
stand-alone commands "CREATE STATISTICS" and "DROP STATISTICS".  Not only
does that fix the syntactic problems of shoehorning them into ALTER TABLE,
but it would be possible to design the syntax to allow for straightforward
extension to multiple-table cases.  Even if we don't know what we'd do
with that just yet, it seems to me like a possible future extension that
we should keep in mind.  And anything based on ALTER TABLE just cannot do
that reasonably.

So consider

CREATE STATISTICS name ON table(columnlist) [, table(columnlist) ...]
[ WITH options ]

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] Multi-column distinctness.

[Simon Riggs]

On 20 October 2015 at 21:06, Tom Lane  wrote:


> I am getting more attracted to your suggestion of making these things
> stand-alone commands "CREATE STATISTICS" and "DROP STATISTICS".  Not only
> does that fix the syntactic problems of shoehorning them into ALTER TABLE,
> but it would be possible to design the syntax to allow for straightforward
> extension to multiple-table cases.  Even if we don't know what we'd do
> with that just yet, it seems to me like a possible future extension that
> we should keep in mind.  And anything based on ALTER TABLE just cannot do
> that reasonably.
>

+1

Good argument, so now I understand and agree with Tomas' original suggestion

So consider
>
> CREATE STATISTICS name ON table(columnlist) [, table(columnlist) ...]
> [ WITH options ]
>

Seems good

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

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

Re: [HACKERS] Typos in plannodes.h

[Etsuro Fujita]

On 2015/10/21 0:13, Robert Haas wrote:

On Tue, Oct 20, 2015 at 7:45 AM, Etsuro Fujita
 wrote:

I found typos in plannodes.h: s/scan.plan.quals/scan.plan.qual/g  Please
find attached a patch.



Oops.  Good catch.  Committed.


Thanks for picking this up!

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] Foreign join pushdown vs EvalPlanQual

[Etsuro Fujita]

On 2015/10/20 13:11, Etsuro Fujita wrote:

On 2015/10/20 5:34, Robert Haas wrote:

On Mon, Oct 19, 2015 at 3:45 AM, Etsuro Fujita
 wrote:

As Tom mentioned, just recomputing the original join tuple is not good
enough.  We would need to rejoin the test tuples for the baserels
even if
ROW_MARK_COPY is in use.  Consider:

A=# BEGIN;
A=# UPDATE t SET a = a + 1 WHERE b = 1;
B=# SELECT * from t, ft1, ft2
  WHERE t.a = ft1.a AND t.b = ft2.b AND ft1.c = ft2.c FOR UPDATE;
A=# COMMIT;

where the plan for the SELECT FOR UPDATE is

LockRows
-> Nested Loop
-> Seq Scan on t
-> Foreign Scan on 
 Remote SQL: SELECT * FROM ft1 JOIN ft2 WHERE ft1.c = ft2.c
AND ft1.a
= $1 AND ft2.b = $2

If an EPQ recheck is invoked by the A's UPDATE, just recomputing the
original join tuple from the whole-row image that you proposed would
output
an incorrect result in the EQP recheck since the value a in the updated
version of a to-be-joined tuple in t would no longer match the value
ft1.a
extracted from the whole-row image if the A's UPDATE has committed
successfully.  So I think we would need to rejoin the tuples
populated from
the whole-row images for the baserels ft1 and ft2, by executing the
secondary plan with the new parameter values for a and b.



No.  You just need to populate fdw_recheck_quals correctly, same as
for the scan case.



Yeah, I think we can probably do that for the case where a pushed-down
join clause is an inner-join one, but I'm not sure that we can do that
for the case where that clause is an outer-join one.  Maybe I'm missing
something, though.


As I said yesterday, that opinion of me is completely wrong.  Sorry for 
the incorrectness.  Let me explain a little bit more.  I still think 
that even if ROW_MARK_COPY is in use, we would need to locally rejoin 
the tuples populated from the whole-row images for the foreign tables 
involved in a remote join, using a secondary plan.  Consider eg,


SELECT localtab.*, ft2 from localtab, ft1, ft2
 WHERE ft1.x = ft2.x AND ft1.y = localtab.y FOR UPDATE

In this case, since the output of the foreign join would not include any 
ft1 columns, I don't think we could do the same thing as for the scan 
case, even if populating fdw_recheck_quals correctly.  And I think we 
would need to rejoin the tuples, using a local join execution plan, 
which would have the parameterization for the to-be-pushed-down clause 
ft1.y = localtab.y.  I'm still missing something, though.


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] checkpoint_segments upgrade recommendation?

[Michael Paquier]

On Wed, Oct 21, 2015 at 3:11 AM, Peter Eisentraut  wrote:

> On 10/17/15 10:25 AM, Michael Paquier wrote:
> > I think that we should just suggest a reverse formula of the maximum
> > soft limit of checkpoint_segments for max_wal_size in the release notes
> > of 9.5, basically:
> > (3 * your_old_checkpoint_segments + 1) * 16MB = max_wal_size
>
> How about this patch?
>
> (Actually, I'd remove the + 1 to make the numbers come out rounder.)
>

Removing the + 1 is fine for me.

+  been removed.  Its place it taken by the new
"Its place is taken".

Other than those little things this looks fine to me.
Regards,
-- 
Michael

Re: [HACKERS] Debugging buildfarm pg_upgrade check failures

[Noah Misch]

On Sat, Jul 25, 2015 at 10:59:27AM -0400, Tom Lane wrote:
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=axolotl=2015-07-24%2020%3A29%3A18
> 
> What evidently happened there is that "pg_ctl start" gave up waiting for
> the postmaster to start too soon.  The postmaster log appears to contain
> 
> LOG:  database system was shut down at 2015-07-24 16:45:40 EDT
> FATAL:  the database system is starting up
> LOG:  MultiXact member wraparound protections are now enabled
> LOG:  database system is ready to accept connections
> 
> which indicates that it did successfully come up, but not till after one
> "PQping" probe from pg_ctl, which was rejected with "still starting up".

For the record, that was not a PQping() probe.  Since "connection to database
failed: FATAL:  the database system is starting up" appeared in the buildfarm
log, it was pg_upgrade attempting to connect after its pg_ctl had given up.

> Meanwhile we've got this log output from pg_ctl:
> 
> waiting for server to start stopped waiting
> pg_ctl: could not start server
> Examine the log output.
> 
> Counting the dots indicates that pg_ctl gave up after precisely 5 seconds.

Thanks for improving that.


-- 
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] Multi-tenancy with RLS

[Haribabu Kommi]

On Sat, Oct 10, 2015 at 1:54 AM, Stephen Frost  wrote:
> * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
>> On Fri, Oct 9, 2015 at 2:04 PM, Stephen Frost  wrote:
>> > * Robert Haas (robertmh...@gmail.com) wrote:
>> >> We've got one reloption for views already - security_barrier.  Maybe
>> >> we could have another one that effectively changes a particular view
>> >> from "security definer" as it is today to "security invoker".
>> >
>> > As I recall, there was a previous suggestion (honestly, I thought it was
>> > your idea) to have a reloption which made views "fully" security
>> > definer, in that functions in the view definition would run as the view
>> > owner instead of the view invoker.
>> >
>> > I liked that idea, though we would need to have a function to say "who
>> > is the 'outer' user?" (CURRENT_USER always being the owner with the
>> > above described reloption).
>> >
>> > I'm less sure about the idea of having a view which runs entirely as the
>> > view invoker, but I'm not against it either.
>>
>> I changed in function check_enable_rls to use the invoker id instead of 
>> owner id
>> for all the system objects, the catalog table policies are getting
>> applied and it is
>> working fine till now in my multi-tenancy testing.
>>
>> Currently I am writing tests to validate it against all user objects also.
>> If this change works for all user objects also, then we may not needed
>> the security invoker
>> reloption.
>
> The reloption would be to allow the user to decide which behavior they
> wanted, as there are use-cases for both.


Any_privilege_option:
Patch that adds 'any' type as a privilege option to verify whether the user
is having any privileges on the object, instead of specifying each and every
privilege type that object supports. Using of this option at grant and revoke
commands throw an error.

View_security_definer:
Patch that adds "security_definer" as a view option to specify whether the
view owner needs to be used for all operations on the view, otherwise the
current user is used.

Currently by default the view owner is used to check against all privileges,
so changing it as invoker instead of owner leads to backward compatibility
problems as permission denied on the base relation and etc. To minimize
the impact, currently the invoker id is used only when the view is rewritten
to base relation for 1) updatable views 2) while applying the row security
policies to the base relations.

Instead of the above change, if we treat all the views by default as security
definer, then to support multi-tenancy we need to change all the system views
as security_definer=false.

comments?

shared_catalog_tenancy:
Patch adds an initdb option -C or --shared-catalog-security to add row level
security policies on shared catalog tables that are eligible for tenancy.
With this option, user gets the tenancy at database level, means user can
get the database list that he has some privileges, but not all. It is
not possible
to disable the shared catalog security once it is set at initdb time.


database_catalog_tenancy:
Patch that adds an database option of "catalog security". This can be used
with alter database only not possible with create database command.
With this option, user gets the tenancy at table level. Once user enables
the catalog security at database level, row level security policies are created
on catalog tables that are eligible. User can disable catalog security if wants.

Known issues:
1. If user (U1) grants permissions on object (tbl1) to user (U2), the user U2
can get the information that there exists an user (U1) in the system, but
U2 cannot get the details of U1.

2. If user (U2) executes a query on an object (tbl2) which the user
(U2) don't have
permissions, as he cannot able to see that object from catalog views/tables,
but the query returns an error message as "permission denied", but in case
if multi-tenancy is enabled, the error message should be "relation
doesn't exist".

Pending items:
1. Need to add some more tests to verify all database catalog tables.
2. Documentation changes for database catalog tenancy.

Regards,
Hari Babu
Fujitsu Australia


1_any_privilege_option_v1.patch
Description: Binary data


2_view_security_definer_v1.patch
Description: Binary data


3_shared_catalog_tenancy_v1.patch
Description: Binary data


4_database_catalog_tenancy_v1.patch
Description: Binary data

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

Re: [HACKERS] Dangling Client Backend Process

[Rajeev rastogi]

On 20 October 2015 23:34, Robert Haas [mailto:robertmh...@gmail.com] Wrote:

>On Tue, Oct 20, 2015 at 12:11 PM, Alvaro Herrera
> wrote:
>> Robert Haas wrote:
>>> I don't think that proc_exit(1) is the right way to exit here.  It's
>>> not very friendly to exit without at least attempting to give the
>>> client a clue about what has gone wrong.  I suggest something like
>>> this:
>>>
>>> ereport(FATAL,
>>> (errcode(ERRCODE_ADMIN_SHUTDOWN),
>>>  errmsg("terminating connection due to postmaster
>>> shutdown")));
>>
>> Agreed, but I don't think "shutdown" is the right word to use here --
>> that makes it sound like it was orderly.  Perhaps "crash"?
>
>Well, that's a little speculative.  "due to unexpected postmaster exit"?

Agreed. Attached is the patch with changes.

Thanks and Regards,
Kumar Rajeev Rastogi




dangling_backend_process_v2.patch
Description: dangling_backend_process_v2.patch

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

[HACKERS] ATT_FOREIGN_TABLE and ATWrongRelkindError()

[Amit Langote]

Hi,

This may just be nitpicking but I noticed that ATWrongRelkindError() could
emit a better message in case of such errors during ALTER COLUMN DEFAULT
and  ALTER COLUMN SET STORAGE than "%s is of the wrong type" which is what
it would emit now. Just need to add a couple of cases to the switch there:

+ case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
+ msg = _("\"%s\" is not a table, view or foreign table");
+ break;

+ case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
+ msg = _("\"%s\" is not a table, materialized view, or foreign table");
+ break;

Attached adds those.

Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 403582c..a5bc508 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4336,6 +4336,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE | ATT_VIEW:
 			msg = _("\"%s\" is not a table or view");
 			break;
+		case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
+			msg = _("\"%s\" is not a table, view or foreign table");
+			break;
 		case ATT_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX:
 			msg = _("\"%s\" is not a table, view, materialized view, or index");
 			break;
@@ -4345,6 +4348,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 		case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
 			msg = _("\"%s\" is not a table, materialized view, or index");
 			break;
+		case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
+			msg = _("\"%s\" is not a table, materialized view, or foreign table");
+			break;
 		case ATT_TABLE | ATT_FOREIGN_TABLE:
 			msg = _("\"%s\" is not a table or foreign table");
 			break;

-- 
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] Foreign join pushdown vs EvalPlanQual

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Wednesday, October 21, 2015 12:31 PM
> To: Robert Haas
> Cc: Tom Lane; Kaigai Kouhei(海外 浩平); Kyotaro HORIGUCHI;
> pgsql-hackers@postgresql.org; Shigeru Hanada
> Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
> 
> On 2015/10/20 13:11, Etsuro Fujita wrote:
> > On 2015/10/20 5:34, Robert Haas wrote:
> >> On Mon, Oct 19, 2015 at 3:45 AM, Etsuro Fujita
> >>  wrote:
> >>> As Tom mentioned, just recomputing the original join tuple is not good
> >>> enough.  We would need to rejoin the test tuples for the baserels
> >>> even if
> >>> ROW_MARK_COPY is in use.  Consider:
> >>>
> >>> A=# BEGIN;
> >>> A=# UPDATE t SET a = a + 1 WHERE b = 1;
> >>> B=# SELECT * from t, ft1, ft2
> >>>   WHERE t.a = ft1.a AND t.b = ft2.b AND ft1.c = ft2.c FOR UPDATE;
> >>> A=# COMMIT;
> >>>
> >>> where the plan for the SELECT FOR UPDATE is
> >>>
> >>> LockRows
> >>> -> Nested Loop
> >>> -> Seq Scan on t
> >>> -> Foreign Scan on 
> >>>  Remote SQL: SELECT * FROM ft1 JOIN ft2 WHERE ft1.c = ft2.c
> >>> AND ft1.a
> >>> = $1 AND ft2.b = $2
> >>>
> >>> If an EPQ recheck is invoked by the A's UPDATE, just recomputing the
> >>> original join tuple from the whole-row image that you proposed would
> >>> output
> >>> an incorrect result in the EQP recheck since the value a in the updated
> >>> version of a to-be-joined tuple in t would no longer match the value
> >>> ft1.a
> >>> extracted from the whole-row image if the A's UPDATE has committed
> >>> successfully.  So I think we would need to rejoin the tuples
> >>> populated from
> >>> the whole-row images for the baserels ft1 and ft2, by executing the
> >>> secondary plan with the new parameter values for a and b.
> 
> >> No.  You just need to populate fdw_recheck_quals correctly, same as
> >> for the scan case.
> 
> > Yeah, I think we can probably do that for the case where a pushed-down
> > join clause is an inner-join one, but I'm not sure that we can do that
> > for the case where that clause is an outer-join one.  Maybe I'm missing
> > something, though.
> 
> As I said yesterday, that opinion of me is completely wrong.  Sorry for
> the incorrectness.  Let me explain a little bit more.  I still think
> that even if ROW_MARK_COPY is in use, we would need to locally rejoin
> the tuples populated from the whole-row images for the foreign tables
> involved in a remote join, using a secondary plan.  Consider eg,
> 
> SELECT localtab.*, ft2 from localtab, ft1, ft2
>   WHERE ft1.x = ft2.x AND ft1.y = localtab.y FOR UPDATE
> 
> In this case, since the output of the foreign join would not include any
> ft1 columns, I don't think we could do the same thing as for the scan
> case, even if populating fdw_recheck_quals correctly.
>
As an aside, could you introduce the reason why you think so? It is
significant point in discussion, if we want to reach the consensus.

It looks to me the above introduction mix up the target-list of user
query and the target-list of remote query.
If EPQ mechanism requires joined tuple on ft1 and ft2, FDW driver can
make a remote query as follows:
  SELECT ft2, ft1.y, ft1.x, ft2.x FROM ft1.x = ft2.x FOR UPDATE
Thus, fdw_scan_tlist has four target-entries, but later two items are
resjunk=true because ForeignScan node drops these columns by projection
when it returns a tuple to upper node.
On the other hands, the joined-tuple we're talking about in this context
is a tuple prior to projection; formed according to the fdw_scan_tlist.
So, it contains all the necessary information to run scan/join qualifiers
towards the joined-tuple. It is not affected by the target-list of user
query.

Even though I think the approach with joined-tuple reconstruction is
reasonable solution here, it is not a fair reason to introduce disadvantage
of Robert's suggestion.

> And I think we
> would need to rejoin the tuples, using a local join execution plan,
> which would have the parameterization for the to-be-pushed-down clause
> ft1.y = localtab.y.  I'm still missing something, though.
>
Also, please don't mix up "what we do" and "how we do".

It is "what we do" to discuss which format of tuples shall be returned
to the core backend from the extension, because it determines the role
of interface. If our consensus is to return a joined-tuple, we need to
design the interface according to the consensus.

On the other hands, it is "how we do" discussion whether we should
enforce all the FDW/CSP extension to have alternative plan, or not.
Once we got a consensus in "what we do" discussion, there are variable
options to solve the requirement by the consensus, however, we cannot
prioritize "how we do" without "what we do".

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

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

Re: [HACKERS] checkpointer continuous flushing

[Fabien COELHO]

Hello Andres,


In my performance testing it showed that calling PerformFileFlush() only
at segment boundaries and in CheckpointWriteDelay() can lead to rather
spikey IO - not that surprisingly. The sync in CheckpointWriteDelay() is
problematic because it only is triggered while on schedule, and not when
behind.


When behind, the PerformFileFlush should be called on segment
boundaries.


That means it's flushing up to a gigabyte of data at once. Far too
much.


Hmmm. I do not get it. There would not be gigabytes, there would be as 
much as was written since the last sleep, about 100 ms ago, which is not 
likely to be gigabytes?



The implementation pretty always will go behind schedule for some
time. Since sync_file_range() doesn't flush in the foreground I don't
think it's important to do the flushing in concert with sleeping.


For me it is important to avoid accumulating too large flushes, and that 
is the point of the call before sleeping.



My testing seems to show that just adding a limit of 32 buffers to
FileAsynchronousFlush() leads to markedly better results.


Hmmm. 32 buffers means 256 KB, which is quite small.


Why?


Because the point of sorting is to generate sequential writes so that the 
HDD has a lot of aligned stuff to write without moving the head, and 32 is 
rather small for that.



The aim is to not overwhelm the request queue - which is where the
coalescing is done. And usually that's rather small.


That is an argument. How small, though? It seems to be 128 by default, so 
I'd rather have 128? Also, it can be changed, so maybe it should really be 
a guc?


If you flush much more sync_file_range starts to do work in the 
foreground.


Argh, too bad. I would have hoped that the would just deal with in an 
asynchronous way, this is not a "fsync" call, just a flush advise.



I wonder if mmap() && msync(MS_ASYNC) isn't a better replacement for
sync_file_range(SYNC_FILE_RANGE_WRITE) than posix_fadvise(DONTNEED). It
might even be possible to later approximate that on windows using
FlushViewOfFile().


I'm not sure that mmap/msync can be used for this purpose, because there is
no real control it seems about where the file is mmapped.


I'm not following? Why does it matter where a file is mapped?


Because it should be in shared buffers where pg needs it? You probably 
should not want to mmap all pg data files in user space for a large 
database? Or if so, currently the OS keeps the data in memory if it has 
enough space, but if you got to mmap this cache management would be pg 
responsability, if I understand correctly mmap and your intentions.



I have had a friend (Christian Kruse, thanks!)  confirm that at least on
OSX msync(MS_ASYNC) triggers writeback. A freebsd dev confirmed that
that should be the case on freebsd too.


Good. My concern is how mmap could be used, though, not the flushing part.


Hmmm. I'll check. I'm still unconvinced that using a tree for a 2-3 element
set in most case is an improvement.


Yes, it'll not matter that much in many cases. But I rather disliked the
NextBufferToWrite() implementation, especially that it walkes the array
multiple times. And I did see setups with ~15 tablespaces.


ISTM that it is rather an argument for taking the tablespace into the 
sorting, not necessarily for a binary heap.



I also noted this point, but I'm not sure how to have a better approach, so
I let it as it is. I tried 50 ms & 200 ms on some runs, without significant
effect on performance for the test I ran then. The point of having not too
small a value is that it provide some significant work to the IO subsystem
without overflowing it.


I don't think that makes much sense. All a longer sleep achieves is
creating a larger burst of writes afterwards. We should really sleep
adaptively.


It sounds reasonable, but what would be the criterion?

--
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] [DOCS] max_worker_processes on the standby

[Robert Haas]

On Sat, Oct 17, 2015 at 5:37 PM, Petr Jelinek  wrote:
> I agree with that sentiment.
>
> Attached patch adds variable to the shmem which is used for module
> activation tracking - set to true in ActiveCommitTs() and false in
> DeactivateCommitTs(). All the checks inside the commit_ts code were changed
> to use this new variable. I also removed the static variable Alvaro added in
> previous commit because it's not needed anymore.

That sounds good to me.  On a quick read-through it looks OK too.

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


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

Re: [HACKERS] More work on SortSupport for text - strcoll() and strxfrm() caching

[Robert Haas]

On Sun, Oct 18, 2015 at 2:52 PM, Peter Geoghegan  wrote:
> On Thu, Oct 15, 2015 at 9:07 AM, Robert Haas  wrote:
>> Would you be willing to try coding it up that way so we can see how it looks?
>
> I attach a patch that does it that way.

That looks good to me.  I've committed this, and the other patch to
remove the obsolete comment.

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


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