Re: Delay locking partitions during query execution

[Amit Langote]

On 2019/01/28 10:26, David Rowley wrote:
> On Tue, 4 Dec 2018 at 00:42, David Rowley  
> wrote:
>> Over here and along similar lines to the above, but this time I'd like
>> to take this even further and change things so we don't lock *any*
>> partitions during AcquireExecutorLocks() and instead just lock them
>> when we first access them with ExecGetRangeTableRelation().  This
>> improves the situation when many partitions get run-time pruned, as
>> we'll never bother locking those at all since we'll never call
>> ExecGetRangeTableRelation() on them. We'll of course still lock the
>> partitioned table so that plan invalidation works correctly.
> 
> I started looking at this patch again and I do see a problem with it.
> Let me explain:
> 
> Currently during LockRelationOid() when we obtain a lock on a relation
> we'll check for rel cache invalidation messages. This means that
> during AcquireExecutorLocks(), as called from CheckCachedPlan(), we
> could miss invalidation messages since we're no longer necessarily
> locking the partitions.
> 
> I think this only creates problems for partition's reloptions being
> changed and cached plans possibly being not properly invalidated when
> that happens, but I might be wrong about that, but if I'm not, there
> still also might be more important reasons to ensure we invalidate the
> plan properly in the future.

It seems to me that plancache.c doesn't really need to perform
AcquireExecutorLocks()/LockRelationOid() to learn that a partition's
reloptions property has changed to discard a generic plan and build a new
one.  AFAICT, PlanCacheRelCallback() takes care of resetting a cached plan
by observing that an invalidation message that it received  either from
the same session or from another session belongs to one of the relations
in PlannedStmt.relationOids.  That list must already contain all
partitions' OIDs.

Session 1:
prepare q as select count(*) from p;
explain (costs off) execute q(1);
 QUERY PLAN

 Finalize Aggregate
   ->  Gather
 Workers Planned: 2
 ->  Partial Aggregate
   ->  Parallel Append
 ->  Parallel Seq Scan on p4
 ->  Parallel Seq Scan on p1
 ->  Parallel Seq Scan on p2
 ->  Parallel Seq Scan on p3
 ->  Parallel Seq Scan on p_def
(10 rows)


-- same session (p1 can no longer use parallel scan)
alter table p1 set (parallel_workers=0);
explain (costs off) execute q(1);
 QUERY PLAN

 Finalize Aggregate
   ->  Gather
 Workers Planned: 2
 ->  Partial Aggregate
   ->  Parallel Append
 ->  Seq Scan on p1
 ->  Parallel Seq Scan on p4
 ->  Parallel Seq Scan on p2
 ->  Parallel Seq Scan on p3
 ->  Parallel Seq Scan on p_def
(10 rows)

-- another session
alter table p1 reset (parallel_workers);

-- back to session 1 (p1 can now use parallel scan)
explain (costs off) execute q(1);
 QUERY PLAN

 Finalize Aggregate
   ->  Gather
 Workers Planned: 2
 ->  Partial Aggregate
   ->  Parallel Append
 ->  Parallel Seq Scan on p4
 ->  Parallel Seq Scan on p1
 ->  Parallel Seq Scan on p2
 ->  Parallel Seq Scan on p3
 ->  Parallel Seq Scan on p_def
(10 rows)

Thanks,
Amit

Re: Covering GiST indexes

[Andreas Karlsson]

On 11/26/18 5:56 PM, Andrey Borodin wrote:

Here's rebased version. Thanks!


Here is my review.

= General

The features seems useful and an obvious extension of covering B-trees, 
and a potentially useful feature together with exclusion constraints.


The patch still applies (with git apply -3), compiles and passes the 
test suite.


From some testing it seems like it works as expected.

= Code

* Have some minor style issues like that there should be spaces around 
|| (in gistcanreturn()) and ? and : (in gistFormTuple()).


* I do not see any need for adding the new parameter to gistFormTuple. 
As far as I can tell isTruncated is always equal to !isleaf.


* The comment below from gistFormTuple() does not look correct. No 
allocation is taking place.


/*
 * Allocate each included attribute.
 */

* Why do you set a supportCollation for the included columns? As far as 
I can tell the collation is only used by the various GiST support 
functions. Also in theory we should be able to skip initializing these 
array entires, but it is probably for the best that we do.


* I think you should check for the attno first in gistcanreturn() to 
take advantage of the short circuiting.


* I am no fan of the tupdesc vs truncTupdesc separation and think that 
it is a potential hazard, but I do not have any better suggestion right now.


* There is no test case for exclusion constraints, and I feel since that 
is one of the more important uses we should probably have at least one 
such test case.


= Minor comments

* I think that "the" should have been kept in the text below.

-Currently, only the B-tree index access method supports this 
feature.
+Currently, B-tree and GiST index access methods supports this 
feature.


* I am not a native speaker but I think it should be "use the INCLUDE 
clause" in the diff below, and I think it also should be "without any 
GiST operator class".


+   A GiST index can be covering, i.e. use INCLUDE 
clause.
+   Included columns can have data types without GiST operator class. 
Included

+   attributes will be stored uncompressed.

* I think you should write something like "Included attributes always 
support index-only scans." rather than "Included attributes can be 
returned." in the comment for gistcanreturn().


* Why the random noise in the diff below? I think using "(c3) INCLUDE 
(c4)" for both gist and rtree results in a cleaner patch.


 CREATE TABLE tbl (c1 int,c2 int, c3 box, c4 box);
 CREATE INDEX on tbl USING brin(c1, c2) INCLUDE (c3, c4);
-CREATE INDEX on tbl USING gist(c3) INCLUDE (c4);
+CREATE INDEX on tbl USING gist(c4) INCLUDE (c3);
 CREATE INDEX on tbl USING spgist(c3) INCLUDE (c4);
 CREATE INDEX on tbl USING gin(c1, c2) INCLUDE (c3, c4);
 CREATE INDEX on tbl USING hash(c1, c2) INCLUDE (c3, c4);
-CREATE INDEX on tbl USING rtree(c1, c2) INCLUDE (c3, c4);
+CREATE INDEX on tbl USING rtree(c4) INCLUDE (c3, c1);
 CREATE INDEX on tbl USING btree(c1, c2) INCLUDE (c3, c4);

Re: Emacs vs pg_indent's weird indentation for function declarations

[Michael Paquier]

On Mon, Jan 28, 2019 at 12:28:30AM -0500, Tom Lane wrote:
> Thomas Munro  writes:
>> That's ... annoying.  I wish indent wouldn't do that, because it means
>> that my declarations get moved around every time I write code.
> 
> If you can fix it, I'd vote for accepting the patch.  I don't personally
> have the desire to dig into the indent code that much ...

If you could get pgindent smarter in this area, it would be really
nice..
--
Michael


signature.asc
Description: PGP signature

Re: Prepare Transaction support for ON COMMIT DROP temporary tables

[Michael Paquier]

On Sat, Jan 19, 2019 at 10:39:43AM +0900, Michael Paquier wrote:
> I have not looked at the patch in details, but we should really be
> careful that if we do that the namespace does not remain behind when
> performing such transactions so as it cannot be dropped.  On my very
> recent lookups of this class of problems you can easily finish by
> blocking a backend from shutting down when dropping its temporary
> schema, with the client, say psql, already able to disconnect.  So as
> long as the 2PC transaction is not COMMIT PREPARED the backend-side
> wait will not be able to complete, blocking a backend slot in shared
> memory.  PREPARE TRANSACTION is very close to a simple commit in terms
> of its semantics, while COMMIT PREPARED is just here to finish
> releasing resources.

I have been looking at this patch, which conflicts on HEAD by the way
(Sorry!) still it is easy enough to get rid of the conflict, and from
what I can see it does not completely do its job.  Simply take the
following example:
=# begin;
BEGIN
=# create temp table aa (a int ) on commit drop;
CREATE TABLE
=# prepare transaction 'ad';
PREPARE TRANSACTION
=# \q

This causes the client to think that the session is finished, but if
you look closer at the backend it is still pending to close until the
transaction is COMMIT PREPARED:
michael   22126  0.0  0.0 218172 15788 ?Ss   15:59   0:00
postgres: michael michael [local] idle waiting

Here is a backtrace:
#7  0x5616900d0462 in LockAcquireExtended (locktag=0x7ffdd6bd5390,
lockmode=8, sessionLock=false, dontWait=false,
reportMemoryError=true, locallockp=0x0)
at lock.c:1050
#8  0x5616900cf9ab in LockAcquire (locktag=0x7ffdd6bd5390,
lockmode=8, sessionLock=false, dontWait=false) at lock.c:713
#9  0x5616900ced07 in LockDatabaseObject (classid=2615,
objid=16385, objsubid=0, lockmode=8) at lmgr.c:934
#10 0x56168fd8cace in AcquireDeletionLock (object=0x7ffdd6bd5414,
flags=0) at dependency.c:1389
#11 0x56168fd8b398 in performDeletion (object=0x7ffdd6bd5414,
behavior=DROP_CASCADE, flags=29) at dependency.c:325
#12 0x56168fda103a in RemoveTempRelations (tempNamespaceId=16385)
at namespace.c:4142
#13 0x56168fda106d in RemoveTempRelationsCallback (code=0, arg=0)
at namespace.c:4161

If you really intend to drop any trace of the objects at PREPARE
phase, that does not seem completely impossible to me, still you would
also need handling for the case where the temp table created also
creates the temporary schema for the session.
--
Michael


signature.asc
Description: PGP signature

Re: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

[Tatsuro Yamada]

Hi Michael,

On 2019/01/28 15:31, Michael Paquier wrote:

On Mon, Jan 28, 2019 at 02:18:25PM +0900, Tatsuro Yamada wrote:

I modified the patch to handle the both:
   - quoted index names
   - complete partial numbers


Committed, which should close the loop for this thread.  If you have
suggestions for the documentation, maybe it would be better to start
another thread.  I am not sure if that's worth worrying about, but
others may have input to offer on the matter.


Thanks!

I'll send a documentation patch on another thread to hear any opinions.

Regards,
Tatsuro Yamada

Re: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

[Michael Paquier]

On Mon, Jan 28, 2019 at 02:18:25PM +0900, Tatsuro Yamada wrote:
> I modified the patch to handle the both:
>   - quoted index names
>   - complete partial numbers

Committed, which should close the loop for this thread.  If you have
suggestions for the documentation, maybe it would be better to start
another thread.  I am not sure if that's worth worrying about, but
others may have input to offer on the matter.
--
Michael


signature.asc
Description: PGP signature

Re: pgsql: Avoid creation of the free space map for small heap relations.

[John Naylor]

On Mon, Jan 28, 2019 at 6:58 AM Amit Kapila  wrote:
>
> On Mon, Jan 28, 2019 at 11:10 AM Andrew Gierth
> you just need to create free space on a page that didn't have enough
> > before? It might be worth tweaking the fillfactor rather than trying to
> > delete anything.
> >
>
> No, it also depends on vacuum to remove rows and then truncate the
> relation accordingly.

That particular test could be removed -- it's just verifying behavior
that's already been there for years and is a direct consquence of
normal truncation combined with the addressing scheme of the FSM
logic.

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

Re: speeding up planning with partitions

[Amit Langote]

Imai-san,

Thanks for testing.

On 2019/01/24 15:09, Imai, Yoshikazu wrote:
> [pgbench commands]
> pgbench -n -f update.sql -T 30 postgres
> 
> [update.sql(updating partkey case)]
> update rt set a = 1;
> 
> [update.sql(updating non-partkey case)]
> update rt set b = 1;
> 
> [results]
> updating partkey case:
> 
> part-num  master 0001 0002 0003 0004
> 18215.34  7924.99  7931.15  8407.40  8475.65 
> 27137.49  7026.45  7128.84  7583.08  7593.73 
> 45880.54  5896.47  6014.82  6405.33  6398.71 
> 84222.96  4446.40  4518.54  4802.43  4785.82 
> 16   2634.91  2891.51  2946.99  3085.81  3087.91 
> 32935.12  1125.28  1169.17  1199.44  1202.04 
> 64352.37   405.27   417.09   425.78   424.53 
> 128   236.26   310.01   307.70   315.29   312.81 
> 25665.3686.8487.6784.3989.27 
> 51218.3424.8423.5523.9123.91 
> 10244.83 6.93 6.51 6.45 6.49 
> 
> 
> updating non-partkey case:
> 
> part-num   master0001 0002 0003  0004
> 18862.58  8421.49  8575.35  9843.71  10065.30   
> 27715.05  7575.78  7654.28  8800.84   8720.60   
> 46249.95  6321.32  6470.26  7278.14   7280.10   
> 84514.82  4730.48  4823.37  5382.93   5341.10   
> 16   2815.21  3123.27  3162.51  3422.36   3393.94   
> 32968.45  1702.47  1722.38  1809.89   1799.88   
> 64364.17   420.48   432.87   440.20435.31   
> 128   119.94   148.77   150.47   152.18143.35   
> 25645.0946.3546.9348.30 45.85   
> 512 8.7410.5910.2310.27 10.13   
> 10242.28 2.60 2.56 2.57  2.51   
> 
> 
> Looking at the results, if we only apply 0001 or 0001 + 0002 and if number of 
> partition is few like 1 or 2, performance degrades compare to master(A 
> maximum reduction is about 5%, which is 8863->8421).
> In all other cases, performance improves compare to master.

Just to be clear, these are cases where pruning *doesn't* occur, though
we'll still need to at least figure out why the degradation occurs for
small number of partitions.

Thanks,
Amit

Re: pgsql: Avoid creation of the free space map for small heap relations.

[Tom Lane]

Amit Kapila  writes:
>> I don't know what the common thread is here, but you don't get to leave
>> the buildfarm broken this badly while you figure it out.

> Sure, but I am wondering why none of this ever shown in local tests,
> as we have done quite some testing related to pgbench as well.

Not sure, but I notice that two of the three critters I just pointed to
use force_parallel_mode.  Dromedary does not, but it's old and slow.
Kind of smells like a timing problem ...

regards, tom lane

Re: pgsql: Avoid creation of the free space map for small heap relations.

[Amit Kapila]

On Mon, Jan 28, 2019 at 11:10 AM Andrew Gierth
 wrote:
>
> > "Amit" == Amit Kapila  writes:
>
>  Amit> Yes, so this could be the cause of the problem. I think we need
>  Amit> to change the tests added by the patch such that they don't rely
>  Amit> on vacuum to remove dead-row versions? Do you or anybody else see
>  Amit> any better way to fix this?
>
> Do you just need to create free space on a page that didn't have enough
> before? It might be worth tweaking the fillfactor rather than trying to
> delete anything.
>

No, it also depends on vacuum to remove rows and then truncate the
relation accordingly.

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

Re: pgsql: Avoid creation of the free space map for small heap relations.

[Amit Kapila]

On Mon, Jan 28, 2019 at 11:18 AM Tom Lane  wrote:
>
> Amit Kapila  writes:
> > Yes, so this could be the cause of the problem.  I think we need to
> > change the tests added by the patch such that they don't rely on
> > vacuum to remove dead-row versions?  Do you or anybody else see any
> > better way to fix this?
>
> To be blunt, this patch needs to be reverted immediately.

Okay, I will do it.

>  The failures
> that are showing up are not just "the fsm test is not portable" problems.
> See for example
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mantid&dt=2019-01-28%2005%3A07%3A06
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2019-01-28%2003%3A07%3A39
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2019-01-28%2003%3A20%3A02
>
> I don't know what the common thread is here, but you don't get to leave
> the buildfarm broken this badly while you figure it out.
>

Sure, but I am wondering why none of this ever shown in local tests,
as we have done quite some testing related to pgbench as well.
Anyway, I think we need figure that out seprately.

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

Re: A few new options for vacuumdb

[Michael Paquier]

On Thu, Jan 24, 2019 at 12:49:28AM +, Bossart, Nathan wrote:
> Oh, wow.  Thanks for pointing this out.  I should have caught this.
> With 0002, we are basically just throwing out the column lists
> entirely as we obtain the qualified identifiers from the catalog
> query.  To fix this, I've added an optional CTE for tracking any
> provided column lists.  v5-0001 is your test patch for this case, and
> v5-0002 splits out the work for split_table_columns_spec().

I think that the query generation could be simplified by always using
the CTE if column lists are present or not, by associating NULL if no
column list is present, and by moving the regclass casting directly
into the CTE.

This way, for the following vacuumdb command with a set of tables
wanted:
vacuumdb --table aa --table 'bb(b)' --table 'cc(c)'

Then the query generated looks like that:
WITH column_lists (table_name, column_list) AS (
  VALUES ('aa'::pg_catalog.regclass, NULL),
 ('bb'::pg_catalog.regclass, '(b)'),
 ('cc'::pg_catalog.regclass, '(c)')
  )
SELECT c.relname, ns.nspname, column_lists.column_list
  FROM pg_catalog.pg_class c
JOIN pg_catalog.pg_namespace ns ON c.relnamespace
  OPERATOR(pg_catalog.=) ns.oid
JOIN column_lists ON
   column_lists.table_name
   OPERATOR(pg_catalog.=) c.oid
  WHERE c.relkind OPERATOR(pg_catalog.=) ANY (array['r', 'm'])
  ORDER BY c.relpages DESC;

So only the following parts are added:
- The CTE with a table and its column list.
- A join on pg_class.oid and column_lists.table_name.
The latest version of the patch is doing a double amount of work by
using a left join and an extra set of clauses in WHERE to fetch the
matching column list from the table name entry.

If no tables are listed, then we just finish with that:
SELECT c.relname, ns.nspname FROM pg_catalog.pg_class c
 JOIN pg_catalog.pg_namespace ns ON c.relnamespace
   OPERATOR(pg_catalog.=) ns.oid
 WHERE c.relkind OPERATOR(pg_catalog.=) ANY (array['r', 'm'])
   ORDER BY c.relpages DESC;
--
Michael


signature.asc
Description: PGP signature

Re: pgsql: Avoid creation of the free space map for small heap relations.

[Tom Lane]

Amit Kapila  writes:
> Yes, so this could be the cause of the problem.  I think we need to
> change the tests added by the patch such that they don't rely on
> vacuum to remove dead-row versions?  Do you or anybody else see any
> better way to fix this?

To be blunt, this patch needs to be reverted immediately.  The failures
that are showing up are not just "the fsm test is not portable" problems.
See for example

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mantid&dt=2019-01-28%2005%3A07%3A06

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2019-01-28%2003%3A07%3A39

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2019-01-28%2003%3A20%3A02

I don't know what the common thread is here, but you don't get to leave
the buildfarm broken this badly while you figure it out.

regards, tom lane

Re: pgsql: Avoid creation of the free space map for small heap relations.

[Andrew Gierth]

> "Amit" == Amit Kapila  writes:

 Amit> Yes, so this could be the cause of the problem. I think we need
 Amit> to change the tests added by the patch such that they don't rely
 Amit> on vacuum to remove dead-row versions? Do you or anybody else see
 Amit> any better way to fix this?

Do you just need to create free space on a page that didn't have enough
before? It might be worth tweaking the fillfactor rather than trying to
delete anything.

-- 
Andrew (irc:RhodiumToad)

Re: pgsql: Avoid creation of the free space map for small heap relations.

[Amit Kapila]

On Mon, Jan 28, 2019 at 10:25 AM Andrew Gierth
 wrote:
>
> > "Amit" == Amit Kapila  writes:
>
>  Amit> One possibility is that autovacuum has triggered to perform
>  Amit> truncation of some other relation (remove pages at the end) which
>  Amit> doesn't allow the FSM test to remove the rows/perform truncation
>  Amit> and thus let to the failure. Can there be anything else which can
>  Amit> start a transaction when a regression test is running? Still
>  Amit> thinking, but inputs are welcome.
>
> I've bumped into issues (cf. commit 64ae420b2) with regression tests of
> this kind caused by concurrent (auto-)ANALYZE (not VACUUM);
>

Yes, so this could be the cause of the problem.  I think we need to
change the tests added by the patch such that they don't rely on
vacuum to remove dead-row versions?  Do you or anybody else see any
better way to fix this?

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

Re: Emacs vs pg_indent's weird indentation for function declarations

[Tom Lane]

Thomas Munro  writes:
> For a while I've been baffled by that: the first arguments of later
> lines don't line up with that of the first line, but they're also not
> in a constant column (it varies from function to function), and it's
> also not caused by 8-space vs 4-space confusion.  It was only when I
> put those two things next to each other just now in this email that I
> finally spotted the logic it's using: if you remove "extern int " then
> the later lines line up with the first argument of the top line.  This
> works for other examples I looked at too.  Huh.

Yeah.  I suspect that the underlying cause is that pgindent doesn't
really distinguish function declarations from definitions, at least
not when it comes time to indent the lines after the first one.

> That's ... annoying.  I wish indent wouldn't do that, because it means
> that my declarations get moved around every time I write code.

If you can fix it, I'd vote for accepting the patch.  I don't personally
have the desire to dig into the indent code that much ...

regards, tom lane

Re: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

[Tatsuro Yamada]

Hi Peter,

On 2019/01/25 20:09, Peter Eisentraut wrote:

On 26/12/2018 07:07, Tatsuro Yamada wrote:

+#define Query_for_list_of_attribute_numbers \
+"SELECT attnum "\
+"  FROM pg_catalog.pg_attribute a, "\
+"   pg_catalog.pg_class c "\
+" WHERE c.oid = a.attrelid "\
+"   AND a.attnum > 0 "\
+"   AND NOT a.attisdropped "\
+"   /* %d %s */" \
+"   AND a.attrelid = (select oid from pg_catalog.pg_class where relname = '%s') 
"\
+"   AND pg_catalog.pg_table_is_visible(c.oid) "\
+"order by a.attnum asc "


This needs a bit of refinement.  You need to handle quoted index names
(see nearby Query_for_list_of_attributes), and you should also complete
partial numbers (e.g., if I type 1 then complete 10, 11, ... if
appropriate).



Thanks for the comments.
I modified the patch to handle the both:
  - quoted index names
  - complete partial numbers

e.g.
-
# create table hoge (a integer, b integer, c integer);
# create index ind_hoge on hoge(a, b, c, (c*1), (c*2), (c*3), (c*4), (c*5), 
(c*6), (c*7), (c*8), (c*9));
# create index "ind hoge2" on hoge(c, b, a, (c*1), (c*2), (c*3), (c*4), (c*5), 
(c*6), (c*7), (c*8), (c*9));

# alter index "ind hoge2" alter column
1   10  11  12  2   3   4   5   6   7   8   9
# alter index "ind hoge2" alter column 1
1   10  11  12

# alter index ind_hoge alter column
1   10  11  12  2   3   4   5   6   7   8   9
# alter index ind_hoge alter column 1
1   10  11  12
-

Please find attached file. :)

Regards,
Tatsuro Yamada
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 292b1f483a..faee44393c 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -583,6 +583,17 @@ static const SchemaQuery Query_for_list_of_statistics = {
 "OR '\"' || relname || '\"'='%s') "\
 "   AND pg_catalog.pg_table_is_visible(c.oid)"
 
+#define Query_for_list_of_attribute_numbers \
+"SELECT attnum "\
+"  FROM pg_catalog.pg_attribute a, pg_catalog.pg_class c "\
+" WHERE c.oid = a.attrelid "\
+"   AND a.attnum > 0 "\
+"   AND NOT a.attisdropped "\
+"   AND pg_catalog.substring(attnum::text,1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(relname)='%s' "\
+"OR '\"' || relname || '\"'='%s') "\
+"   AND pg_catalog.pg_table_is_visible(c.oid)"
+
 #define Query_for_list_of_attributes_with_schema \
 "SELECT pg_catalog.quote_ident(attname) "\
 "  FROM pg_catalog.pg_attribute a, pg_catalog.pg_class c, pg_catalog.pg_namespace n "\
@@ -1604,6 +1615,12 @@ psql_completion(const char *text, int start, int end)
 	/* ALTER INDEX  ALTER */
 	else if (Matches("ALTER", "INDEX", MatchAny, "ALTER"))
 		COMPLETE_WITH("COLUMN");
+	/* ALTER INDEX  ALTER COLUMN */
+	else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLUMN"))
+	{
+		completion_info_charp = prev3_wd;
+		COMPLETE_WITH_QUERY(Query_for_list_of_attribute_numbers);
+	}
 	/* ALTER INDEX  ALTER COLUMN  */
 	else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLUMN", MatchAny))
 		COMPLETE_WITH("SET STATISTICS");

Re: pgsql: Avoid creation of the free space map for small heap relations.

[Andrew Gierth]

> "Amit" == Amit Kapila  writes:

 Amit> One possibility is that autovacuum has triggered to perform
 Amit> truncation of some other relation (remove pages at the end) which
 Amit> doesn't allow the FSM test to remove the rows/perform truncation
 Amit> and thus let to the failure. Can there be anything else which can
 Amit> start a transaction when a regression test is running? Still
 Amit> thinking, but inputs are welcome.

I've bumped into issues (cf. commit 64ae420b2) with regression tests of
this kind caused by concurrent (auto-)ANALYZE (not VACUUM); VACUUM's
snapshot is ignored for testing for whether row versions can be removed,
but ANALYZE's is not.

-- 
Andrew (irc:RhodiumToad)

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Mon, Jan 28, 2019 at 10:03 AM John Naylor
 wrote:
>
> On Mon, Jan 28, 2019 at 4:53 AM Amit Kapila  wrote:
> > There are a few buildfarm failures due to this commit, see my email on
> > pgsql-committers.  If you have time, you can also once look into
> > those.
>
> I didn't see anything in common with the configs of the failed
> members. None have a non-default BLCKSZ that I can see.
>
> Looking at this typical example from woodlouse:
>
> == pgsql.build/src/test/regress/regression.diffs
> ==
> --- C:/buildfarm/buildenv/HEAD/pgsql.build/src/test/regress/expected/fsm.out
> 2019-01-28 04:43:09.031456700 +0100
> +++ C:/buildfarm/buildenv/HEAD/pgsql.build/src/test/regress/results/fsm.out
> 2019-01-28 05:06:20.351100400 +0100
> @@ -26,7 +26,7 @@
>  pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
>   heap_size | fsm_size
>  ---+--
> - 24576 |0
> + 32768 |0
>  (1 row)
>
> ***It seems like the relation extended when the new records should
> have gone into block 0.
>
>  -- Extend table with enough blocks to exceed the FSM threshold
> @@ -56,7 +56,7 @@
>  SELECT pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
>   fsm_size
>  --
> -16384
> +24576
>  (1 row)
>
> ***And here it seems vacuum didn't truncate the FSM. I wonder if the
> heap didn't get truncated either.
>

Yeah, it seems to me that vacuum is not able to truncate the relation,
see my latest reply on another thread [1].

[1] - 
https://www.postgresql.org/message-id/CAA4eK1JntHd7X6dLJVPGYV917HejjhbMKXn9m_RnnCE162LbLA%40mail.gmail.com

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

RE: Timeout parameters

[Nagaura, Ryohei]

Hi,

Sorry for my late.

On Tue, Dec 25, 2018 at 7:40 PM, Fabien COELHO wrote:
> I still do not understand the use-case specifics: for me, aborting the
> connection, or a softer cancelling the statement, will result in the server
> stopping the statement, so the server does NOT "continue the job", 
The server continue the job when the connection is aborted.
You can confirm by the following procedure.
0. connect to the server normally.
1. query the following statement
 =# update tbl set clmn = (select to_number(pg_sleep(10)||'10','20'));
2. kill client-side psql process before the result returns.
3. reconnect the server and "select" query on tbl.

On Thu, Jan 10, 2019 at 3:14 AM, ayaho...@ibagroup.eu wrote:
> Takayuki Tsunakawa, could you provide wider explanation of socket_timeout?
> I'm little bit misunderstanding in which cases this parameter is/can be
> used.
The communication between a client and the server is normal.
The server is so busy that the server can return ack packet and can't work 
statement_timeout.
In this case, the client may wait for very long time.
This parameter is effective for such clients.

> If TCP_USER_TIMEOUT is not supported by PostgreSQL, it means that TCP
> connection are partly controlled by the operation system (kernel). In this
> case pqWaitTimed() should be used on the application layer for connection
> control in data transmission phase.
In the current postgres, PQgetResult() called by sync command "PQexec()" uses 
pqWait().
If the user wishes to sync communication, how do you specify the waiting time 
limit?
It makes sense to implement in pqWait() that can wait clients indefinitely, I 
think.

> As for me It better to specify the description as follows:
Thank you for your comment.
I adopted your documentation in the current patch.

On Wed, Dec 26, 2018 at 8:25 PM, Tsunakawa, Takayuki wrote:
> To wrap up, the relevant parameters work like this:
> 
> * TCP keepalive and TCP user (retransmission) timeout: for network problems
> * statement_timeout: for long-running queries
> * socket_timeout (or send/recv_timeout): for saturated servers
Thank you for your summary.


Best regards,
-
Ryohei Nagaura



TCP_backend_v4.patch
Description: TCP_backend_v4.patch


TCP_interface_v4.patch
Description: TCP_interface_v4.patch

Re: WIP: Avoid creation of the free space map for small tables

[John Naylor]

On Mon, Jan 28, 2019 at 4:53 AM Amit Kapila  wrote:
> There are a few buildfarm failures due to this commit, see my email on
> pgsql-committers.  If you have time, you can also once look into
> those.

I didn't see anything in common with the configs of the failed
members. None have a non-default BLCKSZ that I can see.

Looking at this typical example from woodlouse:

== pgsql.build/src/test/regress/regression.diffs
==
--- C:/buildfarm/buildenv/HEAD/pgsql.build/src/test/regress/expected/fsm.out
2019-01-28 04:43:09.031456700 +0100
+++ C:/buildfarm/buildenv/HEAD/pgsql.build/src/test/regress/results/fsm.out
2019-01-28 05:06:20.351100400 +0100
@@ -26,7 +26,7 @@
 pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
  heap_size | fsm_size
 ---+--
- 24576 |0
+ 32768 |0
 (1 row)

***It seems like the relation extended when the new records should
have gone into block 0.

 -- Extend table with enough blocks to exceed the FSM threshold
@@ -56,7 +56,7 @@
 SELECT pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
  fsm_size
 --
-16384
+24576
 (1 row)

***And here it seems vacuum didn't truncate the FSM. I wonder if the
heap didn't get truncated either.

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

Re: Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

At Fri, 25 Jan 2019 07:26:46 +, "Tsunakawa, Takayuki" 
 wrote in 
<0A3221C70F24FB45833433255569204D1FB70E6B@G01JPEXMBYT05>
> From: Robert Haas [mailto:robertmh...@gmail.com]
> > On Thu, Jan 24, 2019 at 10:02 AM Tom Lane  wrote:
> > > I will argue hard that we should not do it at all, ever.
> > >
> > > There is already a mechanism for broadcasting global GUC changes:
> > > apply them to postgresql.conf (or use ALTER SYSTEM) and SIGHUP.
> > > I do not think we need something that can remotely change a GUC's
> > > value in just one session.  The potential for bugs, misuse, and
> > > just plain confusion is enormous, and the advantage seems minimal.
> > 
> > I think there might be some merit in being able to activate debugging
> > or tracing facilities for a particular session remotely, but designing
> > something that will do that sort of thing well seems like a very
> > complex problem that certainly should not be sandwiched into another
> > patch that is mostly about something else.  And if we ever get such a
> > thing I suspect it should be entirely separate from the GUC system.

It means that we have a lesser copy of the GUC system but can be
set remotely, then some features explicitly register their own
knob on the new system, with the name that I suspenct it should
be the same to the related GUC (for users' convenient).

> +1 for a separate patch for remote session configuration.

It sounds reasnable for me. As I said there should be some such
variables.

> ALTER SYSTEM + SIGHUP targeted at a particular backend would do
> if the DBA can log into the database server (so, it can't be
> used for DBaaS.)  It would be useful to have
> pg_reload_conf(pid).

I don't think it is reasonable. ALTER SYSTEM alters a *system*
configuration which is assumed to be the same on all sessions and
other processes. All sessions start the syscache tracking if
another ALTER SYSTEM for another variable then pg_reload_conf()
come after doing the above. I think the change should persist no
longer than the session-lifetime.

I think that a consensus on backend-targetted remote tuning is
made here:)

A. Let GUC variables settable by a remote session.

  A-1. Variables are changed at a busy time (my first patch).
  (transaction-awareness of GUC makes this complex)

  A-2. Variables are changed when the session is idle (or outside
   a transaction).

B. Override some variables via values laid on shared memory. (my
   second or the last patch).

Very specific to a target feature. I think it consumes a bit
too large memory.

C. Provide session-specific GUC variable (that overides the global one)

   - Add new configuration file "postgresql.conf." and
 pg_reload_conf() let the session with the PID loads it as if
 it is the last included file. All such files are removed at
 startup or at the end of the coressponding session.

   - Add a new syntax like this:
 ALTER SESSION WITH (pid=)
SET configuration_parameter {TO | =} {value | 'value' | DEFAULT}
RESET configuration_parameter
RESET ALL

   - Target variables are marked with GUC_REMOTE.

I'll consider the last choice and will come up with a patch.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: speeding up planning with partitions

[David Rowley]

On Sat, 12 Jan 2019 at 02:00, Amit Langote
 wrote:
>
> On 2019/01/09 9:09, David Rowley wrote:
> > postgres=# update parent set c = c where a = 333;
> > server closed the connection unexpectedly
> > This probably means the server terminated abnormally
> > before or while processing the request.
> >
> > I didn't look into what's causing the crash.
>
> I tried your example, but it didn't crash for me:
>
> explain update parent set c = c where a = 333;
>  QUERY PLAN
> 
>  Update on parent  (cost=0.00..0.00 rows=0 width=0)
>->  Result  (cost=0.00..0.00 rows=0 width=54)
>  One-Time Filter: false
> (3 rows)

I had a closer look. The crash is due to
set_inherit_target_rel_sizes() forgetting to set has_live_children to
false. This results in the relation not properly being set to a dummy
rel and the code then making a modify table node without any subnodes.
That crashes due to getTargetResultRelInfo() returning NULL due to
rootResultRelInfo and resultRelInfo both being NULL.

The attached fixes it. If you were not seeing the crash then
has_live_children must have been zero/false by chance during your
test.

A simple case of:

create table listp (a int, b int) partition by list(a);
create table listp1 partition of listp for values in(1);
update listp set b = b + 1 where a = 42;

was crashing for me.

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


v17_fixup.diff
Description: Binary data

Re: pgsql: Avoid creation of the free space map for small heap relations.

[Amit Kapila]

On Mon, Jan 28, 2019 at 8:49 AM Amit Kapila  wrote:
>
> On Mon, Jan 28, 2019 at 8:17 AM Amit Kapila  wrote:
> >
> > Avoid creation of the free space map for small heap relations.
> >
>
> It seems there is some failure due to this on build farm machines. I
> will investigate!
>

The failure is as below:

@@ -26,7 +26,7 @@
 pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
  heap_size | fsm_size
 ---+--
- 24576 |0
+ 32768 |0
 (1 row)

 -- Extend table with enough blocks to exceed the FSM threshold
@@ -56,7 +56,7 @@
 SELECT pg_relation_size('fsm_check_size', 'fsm') AS fsm_size;
  fsm_size
 --
-16384
+24576
 (1 row)

So here in the above tests, we are deleting some rows, performing
vacuum and then checking the size of heap and or vacuum.  It seems to
me sometimes the vacuum is *not* able to remove the rows and truncate
the relation/FSM as per tests expectation.  One possible theory is
that there is some parallel transaction running which prevents a
vacuum from doing so.  We have tried to avoid that by not allowing to
run the FSM test in parallel with other tests, but I think still
something seems to have run in parallel to the FSM test.  One
possibility is that autovacuum has triggered to perform truncation of
some other relation (remove pages at the end) which doesn't allow the
FSM test to remove the rows/perform truncation and thus let to the
failure.  Can there be anything else which can start a transaction
when a regression test is running?  Still thinking, but inputs are
welcome.

If my theory is correct, then in the newly added tests by this patch,
we can't rely on the vacuum to truncate the relation pages at the end
and hence can't rely on heap/FSM size.

Thoughts?

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

Re: [Sender Address Forgery]Re: using expression syntax for partition bounds

[Amit Langote]

Hi Peter,

On 2019/01/26 17:25, Peter Eisentraut wrote:
> On 25/01/2019 16:19, Tom Lane wrote:
>> Peter Eisentraut  writes:
>>> committed
>>
>> Some of the buildfarm members are having sort-ordering problems
>> with this.  Looks like you could work around it with different
>> partition names (don't assume the relative sort order of
>> letters and digits).
> 
> Fixed by light renaming.

Thank you for committing and taking care of BF failures.

Regards,
Amit

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Mon, Jan 28, 2019 at 9:16 AM John Naylor  wrote:
>
> On Mon, Jan 28, 2019 at 3:53 AM Amit Kapila  wrote:
> > On Thu, Jan 24, 2019 at 9:14 AM Amit Kapila  wrote:
> > > Sure, apart from this I have run pgindent on the patches and make some
> > > changes accordingly.  Latest patches attached (only second patch has
> > > some changes).  I will take one more pass on Monday morning (28th Jan)
> > > and will commit unless you or others see any problem.
> >
> > Pushed these two patches.
>
> Thank you for your input and detailed review! Thank you Mithun for testing!
>

There are a few buildfarm failures due to this commit, see my email on
pgsql-committers.  If you have time, you can also once look into
those.



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

Re: Protect syscache from bloating with negative cache entries

[Kyotaro HORIGUCHI]

At Fri, 25 Jan 2019 08:14:19 +, "Tsunakawa, Takayuki" 
 wrote in 
<0A3221C70F24FB45833433255569204D1FB70EFB@G01JPEXMBYT05>
> Hi Horiguchi-san, Bruce,
> 
> From: Bruce Momjian [mailto:br...@momjian.us]
> > I suggest you go with just syscache_prune_min_age, get that into PG 12,
> > and we can then reevaluate what we need.  If you want to hard-code a
> > minimum cache size where no pruning will happen, maybe based on the system
> > catalogs or typical load, that is fine.
> 
> Please forgive me if I say something silly (I might have got lost.)
> 
> Are you suggesting to make the cache size limit system-defined and 
> uncontrollable by the user?  I think it's necessary for the DBA to be able to 
> control the cache memory amount.  Otherwise, if many concurrent connections 
> access many partitions within a not-so-long duration, then the cache eviction 
> can't catch up and ends up in OOM.  How about the following questions I asked 
> in my previous mail?

cache_memory_target does the opposit of limiting memory usage. It
keeps some amount of syscahe entries unpruned. It is intended for
sessions on where cache-effective queries runs intermittently.
syscache_prune_min_age also doesn't directly limit the size. It
just eventually prevents infinite memory consumption.

The knobs are not no-brainer at all and don't need tuning in most
cases.

> --
> This is a pure question.  How can we answer these questions from users?
> 
> * What value can I set to cache_memory_target when I can use 10 GB for the 
> caches and max_connections = 100?
> * How much RAM do I need to have for the caches when I set 
> cache_memory_target = 1M?
> 
> The user tends to estimate memory to avoid OOM.
> --

You don't have a direct control on syscache memory usage. When
you find a queriy slowed by the default cache expiration, you can
set cache_memory_taret to keep them for intermittent execution of
a query, or you can increase syscache_prune_min_age to allow
cache live for a longer time.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: WIP: Avoid creation of the free space map for small tables

[John Naylor]

On Mon, Jan 28, 2019 at 3:53 AM Amit Kapila  wrote:
> On Thu, Jan 24, 2019 at 9:14 AM Amit Kapila  wrote:
> > Sure, apart from this I have run pgindent on the patches and make some
> > changes accordingly.  Latest patches attached (only second patch has
> > some changes).  I will take one more pass on Monday morning (28th Jan)
> > and will commit unless you or others see any problem.
>
> Pushed these two patches.

Thank you for your input and detailed review! Thank you Mithun for testing!

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

Re: problems with foreign keys on partitioned tables

[Amit Langote]

On 2019/01/25 2:18, Alvaro Herrera wrote:
> On 2019-Jan-24, Amit Langote wrote:
> 
>> A few hunks of the originally proposed patch are attached here as 0001,
>> especially the part which fixes ATAddForeignKeyConstraint to pass the
>> correct value of connoninherit to CreateConstraintEntry (which should be
>> false for partitioned tables).  With that change, many tests start failing
>> because of the above bug.  That patch also adds a test case like the one
>> above, but it fails along with others due to the bug.  Patch 0002 is the
>> aforementioned simpler fix to make the errors (existing and the newly
>> added) go away.
> 
> Cool, thanks.  I made a bunch of fixes -- I added an elog(ERROR) check
> to ensure a constraint whose parent is being set does not already have a
> parent; that seemed in line with the new asserts that check the
> coninhcount.  I also moved those asserts, changing the spirit of what
> they checked.  Also: I wasn't sure about stopping recursion for legacy
> inheritance in ATExecDropConstraint() for non-check constraints, so I
> changed that to occur in partitioned only.  Also, stylistic fixes.

Thanks for the fixes and committing.

> I was mildly surprised to realize that the my_fkey constraint on
> fk_part_1_1 is gone after dropping fkey on its parent, since it was
> declared locally when that table was created.  However, it makes perfect
> sense in retrospect, since we made it dependent on its parent.  I'm not
> terribly happy about this, but I don't quite see a way to make it better
> that doesn't require much more code than is warranted.

Fwiw, CHECK constraints behave that way too.  OTOH, detaching a partition
preserves all the constraints, even the ones that were never locally
defined on the partition.

>> These patches apply unchanged to the PG 11 branch.
> 
> Yeah, only if you tried to compile, it would have complained about
> table_close() ;-)

Oops, sorry.  I was really in a hurry that day as the dinnertime had passed.

Thanks,
Amit

Re: WIP: Avoid creation of the free space map for small tables

[Amit Kapila]

On Thu, Jan 24, 2019 at 9:14 AM Amit Kapila  wrote:
>
> Sure, apart from this I have run pgindent on the patches and make some
> changes accordingly.  Latest patches attached (only second patch has
> some changes).  I will take one more pass on Monday morning (28th Jan)
> and will commit unless you or others see any problem.
>

Pushed these two patches.

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

Re: Use an enum for RELKIND_*?

[Kyotaro HORIGUCHI]

At Thu, 24 Jan 2019 09:37:41 -0500, Tom Lane  wrote in 
<15760.1548340...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  writes:
> > I might misunderstand something, but my compiler (gcc 7.3.1)
> > won't be quiet about omitted value even with default:.
> > ...
> 
> I would call that a compiler bug, TBH.  The code is 100% correct,
> if you intended to allow the default case to handle some enum
> values, which is perfectly reasonable coding.

Yeah, the code is correct. I had switch-enum in my mind.

We can use #pragma (or _Pragma) to apply an option to a specific
region.


#pragma GCC diagnostic push
#pragma GCC diagnostic warning "-Wswitch-enum"
switch ((type) something)
{
#pragma GCC diagnostic pop


but I don't find a usable form of syntax sugar to wrap this.  The
best I can think of is...


#define STRICT_SWITCH(type, value) { \
_Pragma ("GCC diagnostic push")\
_Pragma ("GCC diagnostic warning \"-Wswitch-enum\"")\
switch((type) (value))

#define END_STRICT_SWITCH() \
_Pragma ("GCC diagnostic pop") }


(The brace causes syntax error when END_ is omitted, but the
error messages is not so developer friendly...)


STRICT_SWITCH(type, var)
{
case xxx:
...
default:
  error(ERROR, (errmsg ("unexpected value %d" (int)var)));
}
END_STRICT_SWITCH();



> > Isn't it enough that at least one platform correctly warns that?
> 
> No, especially not if it's only a warning.  Many developers would
> not see it initially, and the buildfarm likely wouldn't complain
> either.

I agree that it would be bothersome for people who are working on
such platforms.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

RE: Log a sample of transactions

[Kuroda, Hayato]

Dear Adrien,

>> * xact_is_sampled is left at the end of a transaction.
>> Should the parameter be set to false at the lowest layer of the transaction 
>> system?
>> I understand it is unnecessary for the functionality, but it have more 
>> symmetry.
>
> Yes, it is not necessary. I wonder what is more important : keep some 
>kind of symmetry or avoid unnecessary code (which can be source of mistake)?

>> 
>> * check_log_duration should be used only when postgres check the duration.
>> But I'm not sure a new function such as check_is_sampled is needed because A 
>> processing in new function will be as almost same as check_log_duration.

> I agree, I asked myself the same question and I preferred to keep code 
simple.

I think your point is also correct.
You should inquire superior reviewers or committers because I cannot judge 
which one is better.


BTW, I give you a suggestion about a test.
This parameter enables users to log statements randomly, hence adding some 
tests is very difficult.
Perhaps Only three cases are available:

* When log_transaction_sample_rate is set to 1, all statements are logged.
* When the parameter is set to 0, they are never logged.
* When the parameter change to 0 inside the transaction, logging is immediately 
stopped.


Best Regards,
Hayato Kuroda
Fujitsu LIMITED

RE: speeding up planning with partitions

[Imai, Yoshikazu]

On Thu, Jan 24, 2019 at 6:10 AM, Imai, Yoshikazu wrote:
> updating partkey case:
> 
> part-num  master 0001 0002 0003 0004
> 18215.34  7924.99  7931.15  8407.40  8475.65
> 27137.49  7026.45  7128.84  7583.08  7593.73
> 45880.54  5896.47  6014.82  6405.33  6398.71
> 84222.96  4446.40  4518.54  4802.43  4785.82
> 16   2634.91  2891.51  2946.99  3085.81  3087.91
> 32935.12  1125.28  1169.17  1199.44  1202.04
> 64352.37   405.27   417.09   425.78   424.53
> 128   236.26   310.01   307.70   315.29   312.81
> 25665.3686.8487.6784.3989.27
> 51218.3424.8423.5523.9123.91
> 10244.83 6.93 6.51 6.45 6.49

I also tested with non-partitioned table case.

updating partkey case:

part-num  master 0001 0002 0003 0004
010956.7  10370.5  10472.6  10571.0  10581.5
18215.34  7924.99  7931.15  8407.40  8475.65 
...
10244.83 6.93 6.51 6.45 6.49


In my performance results, it seems update performance degrades in 
non-partitioned case with v17-patch applied.
But it seems this degrades did not happen at v2-patch.

On Thu, Aug 30, 2018 at 1:45 AM, Amit, Langote wrote:
> UPDATE:
> 
> nparts  master00010002   0003
> ==  ==   
> 0 285628932862   2816

Does this degradation only occur in my tests? Or if this result is correct, 
what may causes the degradation?

--
Yoshikazu Imai

[ANN] pg2arrow

[Kohei KaiGai]

Hello,

I made a utility program to dump PostgreSQL database in Apache Arrow format.

Apache Arrow is a kind of data format for columnar-based structured
data; actively
developed by Spark and comprehensive communities.
It is suitable data representation for static and read-only but large
number of rows.
Many of data analytics tools support Apache Arrow as a common data
exchange format.
See, https://arrow.apache.org/

* pg2arrow
https://github.com/heterodb/pg2arrow

usage:
$ ./pg2arrow -h localhost postgres -c 'SELECT * FROM hogehoge LIMIT
1' -o /tmp/hogehoge.arrow
--> fetch results of the query, then write out "/tmp/hogehoge"
$ ./pg2arrow --dump /tmp/hogehoge
--> shows schema definition of the "/tmp/hogehoge"

$ python
>>> import pyarrow as pa
>>> X = pa.RecordBatchFileReader("/tmp/hogehoge").read_all()
>>> X.schema
id: int32
a: int64
b: double
c: struct
  child 0, x: int32
  child 1, y: double
  child 2, z: decimal(30, 11)
  child 3, memo: string
d: string
e: double
ymd: date32[day]

--> read the Apache Arrow file using PyArrow, then shows its schema definition.


It is also a groundwork for my current development - arrow_fdw; which
allows to scan
on the configured Apache Arrow file(s) as like regular PostgreSQL table.
I expect integration of the arrow_fdw support with SSD2GPU Direct SQL
of PG-Strom
can pull out maximum capability of the latest hardware (NVME and GPU).
Likely, it is an ideal configuration for log-data processing generated
by many sensors.

Please check it.
Comments, ideas, bug-reports, and other feedbacks are welcome.

As an aside, NVIDIA announced their RAPIDS framework; to exchange data frames
on GPU among multiple ML/Analytics solutions. It also uses Apache
Arrow as a common
format for data exchange, and this is also our groundwork for them.
https://www.nvidia.com/en-us/deep-learning-ai/solutions/data-science/

Thanks,
-- 
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei 

Re: Delay locking partitions during query execution

[David Rowley]

On Tue, 4 Dec 2018 at 00:42, David Rowley  wrote:
> Over here and along similar lines to the above, but this time I'd like
> to take this even further and change things so we don't lock *any*
> partitions during AcquireExecutorLocks() and instead just lock them
> when we first access them with ExecGetRangeTableRelation().  This
> improves the situation when many partitions get run-time pruned, as
> we'll never bother locking those at all since we'll never call
> ExecGetRangeTableRelation() on them. We'll of course still lock the
> partitioned table so that plan invalidation works correctly.

I started looking at this patch again and I do see a problem with it.
Let me explain:

Currently during LockRelationOid() when we obtain a lock on a relation
we'll check for rel cache invalidation messages. This means that
during AcquireExecutorLocks(), as called from CheckCachedPlan(), we
could miss invalidation messages since we're no longer necessarily
locking the partitions.

I think this only creates problems for partition's reloptions being
changed and cached plans possibly being not properly invalidated when
that happens, but I might be wrong about that, but if I'm not, there
still also might be more important reasons to ensure we invalidate the
plan properly in the future.

The following shows the problem:

create table listp (a int) partition by list(a);
create table listp1 partition of listp for values in(1);
create table listp2 partition of listp for values in(2);
insert into listp select x from generate_series(1,2)x,
generate_series(1,10);
analyze listp;

-- session 1;
begin;
prepare q1 as select count(*) from listp;
explain (costs off,analyze, timing off, summary off) execute q1;
QUERY PLAN
--
 Finalize Aggregate (actual rows=1 loops=1)
   ->  Gather (actual rows=3 loops=1)
 Workers Planned: 2
 Workers Launched: 2
 ->  Partial Aggregate (actual rows=1 loops=3)
   ->  Parallel Append (actual rows=7 loops=3)
 ->  Parallel Seq Scan on listp2 (actual rows=3 loops=3)
 ->  Parallel Seq Scan on listp1 (actual
rows=10 loops=1)
(8 rows)
-- session 2
alter table listp1 set (parallel_workers=0);

-- session 1

explain (costs off, analyze, timing off, summary off) execute q1; --
same as previous plan, i.e. still does a parallel scan on listp1.

One way around this would be to always perform an invalidation on the
partition's parent when performing a relcache invalidation on the
partition.  We could perhaps invalidate all the way up to the top
level partitioned table, that way we could just obtain a lock on the
target partitioned table during AcquireExecutorLocks(). I'm currently
only setting the delaylock flag to false for leaf partitions only.

It's not particularly great to think of invalidating the partitioned
table's relcache entry for this, but it's also not so great that we
lock all partitions when runtime pruning might prune away the
partition anyway.  I don't see a way that we can have both, but I'm
happy to hear people's thoughts about this.

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

RE: pg_upgrade: Pass -j down to vacuumdb

[Jamison, Kirk]

Hi Jesper,

On Friday, January 25, 2019, Jesper Pedersen  
> Thanks for your feedback !
>
> As per Peter's comments I have changed the patch (v2) to not pass down the -j 
> option to vacuumdb.
>
> Only an update to the documentation and console output is made in order to 
> make it more clear.

Yeah, I understood from the references that the parallel option use
for vacuumdb and pg_upgrade are different. I was also getting
clarification whether pg_upgrade gets to pass it down to vacuumdb.
Based from other Michael's and Peter's replies, I now understand it.

There were also helpful comments from the developers above,
pointing it to the right direction.
In addition to Peter's comment, quoting "...if you want to do this
as fast as possible, don't use this script.  That comment could be
enhanced to suggest the use of the -j option.", so I think you 
should also update the following script in create_script_for_cluster_analyze():
 fprintf(script, "echo %sIf you would like default statistics as quickly as 
possible, cancel%s\n",
 ECHO_QUOTE, ECHO_QUOTE);

There were also advice from Tom and Alvaro that you can incorporate
further in your next patch.

>Alvaro Herrera  writes:
>> So let's have it write with a $VACUUMDB_OPTS variable, which is by 
>> default defined as empty but with a comment suggesting that maybe the 
>> user wants to add the -j option.  This way, if they have to edit it, 
>> they only have to edit the VACUUMDB_OPTS line instead of each of the 
>> two vacuumdb lines.
>
Tom Lane wrote:
>Even better, allow the script to absorb a value from the environment, so that 
>it needn't be edited at all.

Regards,
Kirk Jamison

Re: log bind parameter values on error

[Alexey Bashtanov]

Hi Peter,


With your patch, with log_statement=all and log_parameters=on, you get
the same, but with log_statement=all and log_parameters=off you get

LOG:  execute : SELECT abalance FROM pgbench_accounts WHERE aid
= $1;
DETAIL:  parameters: $1 = UNKNOWN TYPE

Thanks for spotting this, I've fixed it, see the new patch attached.

This also raises the question of the new parameter name.  Parameters are
already logged.  So the name should perhaps be more like
log_parameters_on_error.

Done

Some API notes on your patch:  I think you can change
get_portal_bind_parameters() to take a ParamListInfo, since you're not
doing anything with the Portal other than grab the parameters.  And that
would allow you to keep the signature of errdetail_params() unchanged.

Done

I did some performance tests using the commands shown above and didn't
find any problems.  Obviously the default pgbench stuff isn't very
parameter-intensive.  Do you have tests with more and larger parameter
values?



I've done some tests, but they are not very reproducible:
the difference between runs is more than the difference between master 
vs feature branch

and log_parameters_on_error on vs off.

I was using a small java app, it tested the speed using only a single 
connection.

See its code and the results attached.

I'm sorry for the delay, feel free to move it to next commitfest if needed.

Best,
  Alex

import java.sql.ResultSet;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;

import com.zaxxer.hikari.HikariDataSource;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;


public class JDBCTest {
  public static void main(String[] argv) throws Exception {
Class.forName("org.postgresql.Driver");

HikariDataSource dataSource = new HikariDataSource();

dataSource.setPoolName("foo");
dataSource.setJdbcUrl("jdbc:postgresql://127.0.0.1:5432/alexey?prepareThreshold=1");
dataSource.setUsername("alexey");
dataSource.setPassword("");
dataSource.setMinimumIdle(0);
dataSource.setMaximumPoolSize(1);
dataSource.setIdleTimeout(1 * 1000L);
dataSource.setMaxLifetime(1 * 1000L);
JdbcTemplate jt = new JdbcTemplate(dataSource);

int[][] paramsets = new int[][]{
{10, 0, 0},
{1000, 1, 1},
{1000, 1000, 1000},
{1000, 100, 1},
{1000, 10, 10},
{1000, 1, 100},
{10, 1, 1}
};

for(int[] paramset: paramsets) {

int nruns = paramset[0];
int nparams = paramset[1];
int paramLength = paramset[2];

String sql = "select coalesce('' "
+ String.join("", Collections.nCopies(nparams, ", ?"))
+ ")";
String p = String.join("", Collections.nCopies(paramLength, "z"));
Object[] pp = new Object[nparams];
Arrays.fill(pp, p);

RowMapper rm = (ResultSet resultSet, int i) -> resultSet.getString(1);

long startTime = System.nanoTime();

for (int i = 0; i < nruns; i++) {
List result = jt.query(sql, pp, rm);
}

long endTime = System.nanoTime();
long duration = (endTime - startTime);
System.out.println(String.format("NPARAMS=%d PARAM_LENGTH=%d NRUNS=%d TIME=%fs",
nparams, paramLength, nruns, duration / 100. / 1000));
}
System.exit(0);
  }
}
log_min_duration_statement=-1:

feature branch, log_parameters_on_error=off:
NPARAMS=0 PARAM_LENGTH=0 NRUNS=10 TIME=5.455950s
NPARAMS=1 PARAM_LENGTH=1 NRUNS=1000 TIME=8.708911s
NPARAMS=1000 PARAM_LENGTH=1000 NRUNS=1000 TIME=9.266428s
NPARAMS=100 PARAM_LENGTH=1 NRUNS=1000 TIME=5.949109s
NPARAMS=10 PARAM_LENGTH=10 NRUNS=1000 TIME=5.086381s
NPARAMS=1 PARAM_LENGTH=100 NRUNS=1000 TIME=5.301780s
NPARAMS=1 PARAM_LENGTH=1 NRUNS=10 TIME=6.763883s

feature branch, log_parameters_on_error=on:
NPARAMS=0 PARAM_LENGTH=0 NRUNS=10 TIME=5.238674s
NPARAMS=1 PARAM_LENGTH=1 NRUNS=1000 TIME=8.927818s
NPARAMS=1000 PARAM_LENGTH=1000 NRUNS=1000 TIME=7.960569s
NPARAMS=100 PARAM_LENGTH=1 NRUNS=1000 TIME=6.398343s
NPARAMS=10 PARAM_LENGTH=10 NRUNS=1000 TIME=5.293962s
NPARAMS=1 PARAM_LENGTH=100 NRUNS=1000 TIME=5.519607s
NPARAMS=1 PARAM_LENGTH=1 NRUNS=10 TIME=7.236014s

master:
NPARAMS=0 PARAM_LENGTH=0 NRUNS=10 TIME=5.600735s
NPARAMS=1 PARAM_LENGTH=1 NRUNS=1000 TIME=11.478406s
NPARAMS=1000 PARAM_LENGTH=1000 NRUNS=1000 TIME=9.701054s
NPARAMS=100 PARAM_LENGTH=1 NRUNS=1000 TIME=6.352961s
NPARAMS=10 PARAM_LENGTH=10 NRUNS=1000 TIME=5.153303s
NPARAMS=1 PARAM_LE

Re: "SELECT ... FROM DUAL" is not quite as silly as it appears

[Stefan Keller]

Dear all,

I'm following this list since years - especially PostGIS related - and
you and PG are just awesome!
Pls. let me chime in as a university teacher, therefore used to
explain every year the same things :-).
My 2 cents here are:

Pls. try to give DUAL a better name, since it's IMHO neither
self-explaining nor correct.
Taken from [1], citing the originator:
<<
The name, DUAL, seemed apt for the process of creating a pair of
rows from just one.[1]
The original DUAL table had two rows in it (hence its name), but
subsequently it only had one row.
<<

My first guess is to name the dummy table with with no columns and one
row "DUMMY" - but I actually want to leave the fun to you to name it.

:Stefan

[1] https://en.wikipedia.org/wiki/DUAL_table

Am So., 27. Jan. 2019 um 21:53 Uhr schrieb Mark Dilger
:
>
>
>
> > On Jan 27, 2019, at 12:04 PM, Mark Dilger  wrote:
> >
> >
> >
> >> On Jan 25, 2019, at 5:09 PM, Tom Lane  wrote:
> >>
> >> David Rowley  writes:
> >>> As far as I can see the patch is ready to go, but I'll defer to Mark,
> >>> who's also listed on the reviewer list for this patch.
> >>
> >> Mark, are you planning to do further review on this patch?
> >> I'd like to move it along, since (IMO anyway) we need it in
> >> before progress can be made on
> >> https://commitfest.postgresql.org/21/1664/
> >>
> >>  regards, tom lane
> >
> >
> > These two observations are not based on any deep understanding of
> > your patch, but just some cursory review:
> >
> > The swtich in src/backend/parser/analyze.c circa line 2819 should
> > probably have an explicit case for RTE_RESULT rather than just a
> > comment and allowing the default to log "unrecognized RTE type",
> > since it's not really unrecognized, just unexpected.  But I'm not
> > too exercised about that, and won't argue if you want to leave it
> > as is.
> >
> > Similarly, in src/backend/commands/explain.c, should there be a
> > case for T_Result in ExplainTargetRel's switch circa line 2974?
> > I'm not sure given your design whether this could ever be relevant,
> > but I noticed that T_Result / RTE_RELATION isn't handled here.
> >
> >
> > Applying your patch and running the regression tests is failing
> > left and right, so I'm working to pull a fresh copy from git and
> > build again -- probably just something wrong in my working directory.
>
> Ok, version 6 of the patch applies cleanly, compiles, and passes
> all tests for me on my platform (mac os x).  You can address the two
> minor observations above, or not, at your option.
>
> mark
>
>

Re: FETCH FIRST clause PERCENT option

[Tomas Vondra]

On 1/24/19 10:57 AM, Surafel Temesgen wrote:
> 
> 
> On Wed, Jan 9, 2019 at 8:18 PM Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
>  
> 
> 
> See the attached patch, which recomputes the count regularly. I don't
> claim the patch is committable or that it has no other bugs, but
> hopefully it shows what I meant.
> 
> 
> 
> I got only one issue it is not work well with cursor
> 
> postgres=# START TRANSACTION;
> 
> START TRANSACTION
> 
> postgres=# create table t as select i from generate_series(1,1000) s(i);
> 
> SELECT 1000
> 
> postgres=# declare c cursor for select * from t fetch first 5 percent
> rows only;
> 
> DECLARE CURSOR
> 
> postgres=# fetch all in c;
> 
> ERROR: trying to store a minimal tuple into wrong type of slot
> 
> 
> I am looking at it .
> 

OK. Does that mean you agree the incremental approach is reasonable?

> meanwhile i fix row estimation and cost and make create_ordered_paths
> creation with no LIMIT consideration in PERCENTAGE case
> 

I haven't reviewed the costing code yet, but linear interpolation seems
reasonable in principle. I find the create_limit_path changes somewhat
sloppy and difficult to comprehend (particularly without comments).

For example, the fact that the code computes the total cost based on
count_rows, which may be tweaked later seems suspicious. I mean, the doe
now does this:

if (limitOption = PERCENTAGE)
{
... update count_rows
... compute total_cost
}

if (count_rows > pathnode->path.rows)
count_rows = pathnode->path.rows;

... compute total_cost for the EXACT_NUMBER case

pathnode->path.rows = count_rows;

But I need to do think about this a bit more.

FWIW I see there are no regression tests for the PERCENT option.


regards

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

Emacs vs pg_indent's weird indentation for function declarations

[Thomas Munro]

Hello,

Using either the .dir-locals.el settings or the "more complete"
src/tools/editors/emacs.samples, I have never convinced Emacs to
produce multi-line function declarations in .h files that satisfy
pg_indent.

For example, latch.c has the following definition:

int
WaitEventSetWait(WaitEventSet *set, long timeout,
 WaitEvent *occurred_events, int nevents,
 uint32 wait_event_info)

And now let's see the declaration in latch.h:

extern int WaitEventSetWait(WaitEventSet *set, long timeout,
 WaitEvent *occurred_events, int nevents,
 uint32 wait_event_info);

(I replaced all tabs with four space here; if you're reading in a
monospace font, you'll see that the first arguments off all lines
begin in the same column in the definition by not in the declaration.)

For a while I've been baffled by that: the first arguments of later
lines don't line up with that of the first line, but they're also not
in a constant column (it varies from function to function), and it's
also not caused by 8-space vs 4-space confusion.  It was only when I
put those two things next to each other just now in this email that I
finally spotted the logic it's using: if you remove "extern int " then
the later lines line up with the first argument of the top line.  This
works for other examples I looked at too.  Huh.

That's ... annoying.  I wish indent wouldn't do that, because it means
that my declarations get moved around every time I write code.  But if
it's not possible to change that for whatever technical or political
reason, I wonder if it's possible to teach Emacs to understand that
weird rule...

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

Re: WIP: Avoid creation of the free space map for small tables

[John Naylor]

On Sat, Jan 26, 2019 at 2:14 PM Amit Kapila  wrote:
>
> On Sat, Jan 26, 2019 at 5:05 AM John Naylor  
> wrote:
> >
> > So, in v19 we check pg_class.relpages and if it's
> > a heap and less than or equal the threshold we call stat on the 0th
> > segment to verify.
> >
>
> Okay, but the way logic is implemented appears clumsy to me.

> The function transfer_relfile has no clue about skipping of FSM stuff,
> but it contains comments about it.

Yeah, I wasn't entirely happy with how that turned out.

> I think there is some value in using the information from
> this function to skip fsm files, but the code doesn't appear to fit
> well, how about moving this check to new function
> new_cluster_needs_fsm()?

For v21, new_cluster_needs_fsm() has all responsibility for obtaining
the info it needs. I think this is much cleaner, but there is a small
bit of code duplication since it now has to form the file name. One
thing we could do is form the the base old/new file names in
transfer_single_new_db() and pass those to transfer_relfile(), which
will only add suffixes and segment numbers. We could then pass the
base old file name to new_cluster_needs_fsm() and use it as is. Not
sure if that's worthwhile, though.

> The order in which relkind and relpages is used in the above code is
> different from the order in which it is mentioned in the query, it
> won't matter, but keeping in order will make look code consistent.  I
> have made this and some more minor code adjustments in the attached
> patch.  If you like those, you can include them in the next version of
> your patch.

Okay, done.

-- 
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 2b70b65f43c05234b81e939b0ed49e34b97d5667 Mon Sep 17 00:00:00 2001
From: John Naylor 
Date: Sun, 27 Jan 2019 21:42:07 +0100
Subject: [PATCH v21 3/3] During pg_upgrade, conditionally skip transfer of
 FSMs.

If a heap on the old cluster has 4 pages or fewer, and the old cluster
was PG v11 or earlier, don't copy or link the FSM. This will shrink
space usage for installations with large numbers of small tables.
---
 src/bin/pg_upgrade/info.c| 16 +++--
 src/bin/pg_upgrade/pg_upgrade.h  |  6 
 src/bin/pg_upgrade/relfilenode.c | 58 ++--
 3 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index 2f925f086c..902bfc647e 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -200,6 +200,8 @@ create_rel_filename_map(const char *old_data, const char *new_data,
 
 	map->old_db_oid = old_db->db_oid;
 	map->new_db_oid = new_db->db_oid;
+	map->relpages = old_rel->relpages;
+	map->relkind = old_rel->relkind;
 
 	/*
 	 * old_relfilenode might differ from pg_class.oid (and hence
@@ -418,6 +420,7 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
 	char	   *nspname = NULL;
 	char	   *relname = NULL;
 	char	   *tablespace = NULL;
+	char	   *relkind = NULL;
 	int			i_spclocation,
 i_nspname,
 i_relname,
@@ -425,7 +428,9 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
 i_indtable,
 i_toastheap,
 i_relfilenode,
-i_reltablespace;
+i_reltablespace,
+i_relpages,
+i_relkind;
 	char		query[QUERY_ALLOC];
 	char	   *last_namespace = NULL,
 			   *last_tablespace = NULL;
@@ -494,7 +499,7 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
 	 */
 	snprintf(query + strlen(query), sizeof(query) - strlen(query),
 			 "SELECT all_rels.*, n.nspname, c.relname, "
-			 "  c.relfilenode, c.reltablespace, %s "
+			 "  c.relfilenode, c.reltablespace, c.relpages, c.relkind, %s "
 			 "FROM (SELECT * FROM regular_heap "
 			 "  UNION ALL "
 			 "  SELECT * FROM toast_heap "
@@ -525,6 +530,8 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
 	i_relname = PQfnumber(res, "relname");
 	i_relfilenode = PQfnumber(res, "relfilenode");
 	i_reltablespace = PQfnumber(res, "reltablespace");
+	i_relpages = PQfnumber(res, "relpages");
+	i_relkind = PQfnumber(res, "relkind");
 	i_spclocation = PQfnumber(res, "spclocation");
 
 	for (relnum = 0; relnum < ntups; relnum++)
@@ -556,6 +563,11 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
 		curr->relname = pg_strdup(relname);
 
 		curr->relfilenode = atooid(PQgetvalue(res, relnum, i_relfilenode));
+		curr->relpages = atoi(PQgetvalue(res, relnum, i_relpages));
+
+		relkind = PQgetvalue(res, relnum, i_relkind);
+		curr->relkind = relkind[0];
+
 		curr->tblsp_alloc = false;
 
 		/* Is the tablespace oid non-default? */
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 2f67eee22b..baeb8ff0f8 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -147,6 +147,8 @@ typedef struct
 	char	   *tablespace;		/* tablespace path; "" for cluster default */
 	bool		nsp_alloc;		/* should nspname be freed? */
 	bool		tblsp_alloc;	/* should tablespace be freed? */
+	int32

Re: "SELECT ... FROM DUAL" is not quite as silly as it appears

[Mark Dilger]

> On Jan 27, 2019, at 12:04 PM, Mark Dilger  wrote:
> 
> 
> 
>> On Jan 25, 2019, at 5:09 PM, Tom Lane  wrote:
>> 
>> David Rowley  writes:
>>> As far as I can see the patch is ready to go, but I'll defer to Mark,
>>> who's also listed on the reviewer list for this patch.
>> 
>> Mark, are you planning to do further review on this patch?
>> I'd like to move it along, since (IMO anyway) we need it in
>> before progress can be made on
>> https://commitfest.postgresql.org/21/1664/
>> 
>>  regards, tom lane
> 
> 
> These two observations are not based on any deep understanding of
> your patch, but just some cursory review:
> 
> The swtich in src/backend/parser/analyze.c circa line 2819 should
> probably have an explicit case for RTE_RESULT rather than just a
> comment and allowing the default to log "unrecognized RTE type",
> since it's not really unrecognized, just unexpected.  But I'm not
> too exercised about that, and won't argue if you want to leave it
> as is.
> 
> Similarly, in src/backend/commands/explain.c, should there be a
> case for T_Result in ExplainTargetRel's switch circa line 2974?
> I'm not sure given your design whether this could ever be relevant,
> but I noticed that T_Result / RTE_RELATION isn't handled here.
> 
> 
> Applying your patch and running the regression tests is failing
> left and right, so I'm working to pull a fresh copy from git and
> build again -- probably just something wrong in my working directory.

Ok, version 6 of the patch applies cleanly, compiles, and passes
all tests for me on my platform (mac os x).  You can address the two
minor observations above, or not, at your option.

mark

Re: "SELECT ... FROM DUAL" is not quite as silly as it appears

[Mark Dilger]

> On Jan 25, 2019, at 5:09 PM, Tom Lane  wrote:
> 
> David Rowley  writes:
>> As far as I can see the patch is ready to go, but I'll defer to Mark,
>> who's also listed on the reviewer list for this patch.
> 
> Mark, are you planning to do further review on this patch?
> I'd like to move it along, since (IMO anyway) we need it in
> before progress can be made on
> https://commitfest.postgresql.org/21/1664/
> 
>   regards, tom lane


These two observations are not based on any deep understanding of
your patch, but just some cursory review:

The swtich in src/backend/parser/analyze.c circa line 2819 should
probably have an explicit case for RTE_RESULT rather than just a
comment and allowing the default to log "unrecognized RTE type",
since it's not really unrecognized, just unexpected.  But I'm not
too exercised about that, and won't argue if you want to leave it
as is.

Similarly, in src/backend/commands/explain.c, should there be a
case for T_Result in ExplainTargetRel's switch circa line 2974?
I'm not sure given your design whether this could ever be relevant,
but I noticed that T_Result / RTE_RELATION isn't handled here.


Applying your patch and running the regression tests is failing
left and right, so I'm working to pull a fresh copy from git and
build again -- probably just something wrong in my working directory.

mark

Re: "SELECT ... FROM DUAL" is not quite as silly as it appears

[Mark Dilger]

> On Jan 25, 2019, at 5:09 PM, Tom Lane  wrote:
> 
> David Rowley  writes:
>> As far as I can see the patch is ready to go, but I'll defer to Mark,
>> who's also listed on the reviewer list for this patch.
> 
> Mark, are you planning to do further review on this patch?
> I'd like to move it along, since (IMO anyway) we need it in
> before progress can be made on
> https://commitfest.postgresql.org/21/1664/

Doing a quick review now.  Sorry I didn't see your messages earlier.

mark

Re: [Patch] Log10 and hyperbolic functions for SQL:2016 compliance

[Lætitia Avrot]

Hi,

Thanks for your time and advice, Tom!


> > [ adding_log10_and_hyperbolic_functions_v1.patch ]
>
> No objection to the feature, but
>
> - Why are you using the float4-width library functions (coshf etc)
> rather than the float8-width ones (cosh etc)?
>
> Well, I guess the only reason is that I wasn't paying attention enough...
I changed for the float8-width library functions.


> - I wonder whether these library functions exist everywhere.
> If they don't, what will we do about it ... throw an error?
>
> I see that SUSv2 requires cosh() and so on, so it's quite possible
> that these do exist everywhere we care about.  I'd be okay with
> throwing a patch onto the buildfarm to see, and adding an autoconf
> test only if the buildfarm is unhappy.  But we should be clear on
> what we're going to do about it if that happens.
>
> I think I was too confident because of the "it works on my laptop"
syndrome... I don't know how to answer to this point.


> > I added regression tests for the new functions, but I didn't for log10
> > function, assuming that if log function worked, log10 will work too.
>
> Not quite sure I believe that.
>
> Do you mean I should also add a regression test for the new log10 function
too ?


> Actually, what I'd counsel is that you *not* include tests of what
> these do with Inf and NaN.  There is no upside to doing so, and lots
> of downside if older platforms are squirrely in their behavior, which
> is hardly unlikely (cf opossum ...)
>

I changed the regression tests for hyperbolic functions, so it doesn't test
for Inf and NaN.

You'll find enclosed the new version of the patch.

Cheers,

Lætitia
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4930ec17f6..94e3fcd4b9 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -896,6 +896,19 @@
2
   
 
+  
+   
+
+ log10
+
+log10(dp or numeric)
+   
+   (same as input)
+   base 10 logarithm
+   log10(100.0)
+   2
+  
+
   
log(b numeric,
 x numeric)
@@ -1147,7 +1160,7 @@
   
 
   
-   Finally,  shows the
+shows the
available trigonometric functions.  All trigonometric functions
take arguments and return values of type double
precision.  Each of the trigonometric functions comes in
@@ -1311,8 +1324,65 @@

   
 
-  
+  
+   Finally,  shows the
+   available hyperbolic functions.
+  
 
+  
+Hyperbolic Functions
+
+
+ 
+  
+   Function
+   Return Type
+   Description
+   Example
+   Result
+  
+ 
+ 
+  
+   
+
+ sinh
+
+sinh(dp)
+   
+   dp
+   hyperbolic sine
+   sinh(0)
+   0
+  
+  
+   
+
+ cosh
+
+cosh(dp)
+   
+   dp
+   hyperbolic cosine
+   cosh(0)
+   1
+  
+  
+   
+
+ tanh
+
+tanh(dp)
+   
+   dp
+   hyperbolic tangent
+   tanh(0)
+   0
+  
+ 
+
+   
+  
 
   
String Functions and Operators
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 117ded8d1d..7712c3d5bd 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2375,6 +2375,54 @@ radians(PG_FUNCTION_ARGS)
 	PG_RETURN_FLOAT8(float8_mul(arg1, RADIANS_PER_DEGREE));
 }
 
+/* == HYPERBOLIC FUNCTIONS == */
+
+/*
+ *		dsinh			- returns the hyperbolic sine of arg1
+ */
+Datum
+dsinh(PG_FUNCTION_ARGS)
+{
+	float8		arg1 = PG_GETARG_FLOAT8(0);
+	float8		result;
+
+	result = sinh(arg1);
+
+	check_float8_val(result, isinf(arg1), true);
+	PG_RETURN_FLOAT8(result);
+}
+
+
+/*
+ *		dcosh			- returns the hyperbolic cosine of arg1
+ */
+Datum
+dcosh(PG_FUNCTION_ARGS)
+{
+	float8		arg1 = PG_GETARG_FLOAT8(0);
+	float8		result;
+
+	result = cosh(arg1);
+
+	check_float8_val(result, isinf(arg1), false);
+	PG_RETURN_FLOAT8(result);
+}
+
+/*
+ *		dtanh			- returns the hyperbolic tangent of arg1
+ */
+Datum
+dtanh(PG_FUNCTION_ARGS)
+{
+	float8		arg1 = PG_GETARG_FLOAT8(0);
+	float8		result;
+
+	errno = 0;
+	result = tanh(arg1);
+
+	check_float8_val(result, false, true);
+	PG_RETURN_FLOAT8(result);
+}
 
 /*
  *		drandom		- returns a random number
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 3ecc2e12c3..ca6b09a13d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -2617,6 +2617,9 @@
 { oid => '1340', descr => 'base 10 logarithm',
   proname => 'log', prorettype => 'float8', proargtypes => 'float8',
   prosrc => 'dlog10' },
+{ oid => '1364', descr => 'base 10 logarithm',
+  proname => 'log10', prorettype => 'float8', proargtypes => 'float8',
+  prosrc => 'dlog10' },
 { oid => '1341', descr => 'natural logarithm',
   proname => 'ln', prorettype => 'float8', proargtypes => 'float8',
   prosrc => 'dlog1' },
@@ -3283,6 +3286,17 @@
 { oid => '1610', descr => 'PI',
   pron

Re: libpq environment variables in the server

[Dmitry Igrishin]

пн, 21 янв. 2019 г. в 13:42, Peter Eisentraut
:
>
> When libpq is loaded in the server (libpqwalreceiver, dblink,
> postgres_fdw), it may use libpq environment variables set in the
> postmaster environment for connection parameter defaults.  I have
> noticed that this has some confusing effects in our test suites.  I
> wonder if this is a good idea in general.
>
> For example, the TAP test infrastructure sets PGAPPNAME to allow
> identifying clients in the server log.  But this environment variable is
> also inherited by temporary servers started with pg_ctl and is then in
> turn used by libpqwalreceiver as the application_name for connecting to
> remote servers where it then shows up in pg_stat_replication and is
> relevant for things like synchronous_standby_names.
>
> Also, the pg_rewind tests appear to rely implicitly on PGHOST and PGPORT
> being set in the server environment to find the other node via
> primary_conninfo.  That is easy to fix, but it shows that this kind of
> thing can creep in unintentionally.
>
> I was thinking that maybe we should clear all libpq environment
> variables in the server, or perhaps have a mode in libpq to ignore all
> environment variables.  Then again, maybe setting something like
> PGSSLMODE globally in the server could be useful, so just removing
> everything might not be the right answer.
As an author of the C++ client API (Pgfe) that currently wraps libpq I
would like
to ignore all these environment variables that affects libpq's behavior, because
libpq is an implementation detail and the Pgfe API hides it
completely. So +1 from
me for such a mode in libpq.

Re: Early WIP/PoC for inlining CTEs

[Andreas Karlsson]

On 1/26/19 11:55 PM, Tom Lane wrote:> Hearing no immediate pushback on 
that proposal, I went ahead and made

a version of the patch that does it like that, as attached.  I also took
a stab at documenting it fully.


Thanks! This version of the patch looks solid, including the 
documentation. The only remaining question I see is the one Andrew 
raised about if we should change the language to allow for future 
versions of PostgreSQL to add costing for when the same CTE is 
referenced multiple times.


Andreas

Re: Early WIP/PoC for inlining CTEs

[Andreas Karlsson]

On 1/27/19 4:21 PM, Tom Lane wrote:

Andrew Gierth  writes:

I'm not sure we should nail down the rule that the absence of NOT
MATERIALIZED will mean a multiply-referenced CTE is evaluated once. One
would hope that in the future the planner might be taught to inline or
not in that case depending on cost. I think it makes more sense to say
that we never inline if MATERIALIZED is specified, that we always inline
if NOT MATERIALIZED is specified, and that if neither is specified the
planner will choose (but perhaps note that currently it always chooses
only based on refcount).


I have no objection to documenting it like that; I just don't want us
to go off into the weeds trying to actually implement something smarter
for v12.


+1

Andreas

Re: Index Skip Scan

[Dmitry Dolgov]

> On Sat, Jan 26, 2019 at 6:45 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> Rebased version after rd_amroutine was renamed.

And one more to fix the documentation. Also I've noticed few TODOs in the patch
about the missing docs, and replaced them with a required explanation of the
feature.


0001-Index-skip-scan-v7.patch
Description: Binary data

Re: Early WIP/PoC for inlining CTEs

[Tom Lane]

Andrew Gierth  writes:
> I'm not sure we should nail down the rule that the absence of NOT
> MATERIALIZED will mean a multiply-referenced CTE is evaluated once. One
> would hope that in the future the planner might be taught to inline or
> not in that case depending on cost. I think it makes more sense to say
> that we never inline if MATERIALIZED is specified, that we always inline
> if NOT MATERIALIZED is specified, and that if neither is specified the
> planner will choose (but perhaps note that currently it always chooses
> only based on refcount).

I have no objection to documenting it like that; I just don't want us
to go off into the weeds trying to actually implement something smarter
for v12.

regards, tom lane

Re: Allowing extensions to supply operator-/function-specific info

[Tom Lane]

Simon Riggs  writes:
> On Sun, 20 Jan 2019 at 23:48, Tom Lane  wrote:
>> What I'm envisioning therefore is that we allow an auxiliary function ...

> Does this help with these cases?

> * Allow a set returning function to specify number of output rows, in cases
> where that is variable and dependent upon the input params?

Yes, within the usual limits of what the planner can know.  The 0004
patch I posted yesterday correctly estimates the number of rows for
constant-arguments cases of generate_series() and unnest(anyarray),
and it also understands unnest(array[x,y,z,...]) even when some of the
array[] elements aren't constants.  There's room to add knowledge about
other SRFs, but those are cases I can recall hearing complaints about.

> * Allow a normal term to match a functional index, e.g. WHERE x =
> 'abcdefgh' => WHERE substr(x, 1 , 5) = 'abcde' AND x = 'abcdefgh'

I'm a bit confused about what you think this example means.  I do
intend to work on letting extensions define rules for extracting
index clauses from function calls, because that's the requirement
that PostGIS is after in the thread that started this.  I don't
know whether that would satisfy your concern, because I'm not clear
on what your concern is.

> * Allow us to realise that ORDER BY f(x) => ORDER BY x so we can use
> ordered paths from indexes, or avoid sorts.

Hm.  That's not part of what I'm hoping to get done for v12, but you
could imagine a future extension to add a support request type that
allows deriving related pathkeys.  There would be a lot of work to do
to make that happen, but the aspect of it that requires adding
function-specific knowledge could usefully be packaged as a
support-function request.

regards, tom lane

Re: Opossum vs. float4 NaN

[Tom Lane]

Glyn Astill  writes:
> I guess the main question is; does anybody care about builds on a 20 year old 
> netbsd/mipsel dinosaur?  I noticed there are now mips64el and mips64eb 
> build-farm members.

I tend to think that variety in the buildfarm is intrinsically a good
thing.  I'd rather see you continue to run the thing, perhaps with a
newer netbsd version if that will resolve the problem.

regards, tom lane

Re: A few new options for vacuumdb

[Michael Paquier]

On Thu, Jan 24, 2019 at 12:49:28AM +, Bossart, Nathan wrote:
> Oh, wow.  Thanks for pointing this out.  I should have caught this.
> With 0002, we are basically just throwing out the column lists
> entirely as we obtain the qualified identifiers from the catalog
> query.  To fix this, I've added an optional CTE for tracking any
> provided column lists.  v5-0001 is your test patch for this case, and
> v5-0002 splits out the work for split_table_columns_spec().

Committed the test portion for now, still reviewing the rest..
--
Michael


signature.asc
Description: PGP signature

Re: Early WIP/PoC for inlining CTEs

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> I was interested to find, while writing the docs, that it's a real
 Tom> struggle to invent plausible reasons to write MATERIALIZED given
 Tom> the above specification. You pretty much have to have lied to the
 Tom> planner, eg by making a volatile function that's not marked
 Tom> volatile, before there's a real need for that. Am I missing
 Tom> something? If I'm not, then we're in a really good place
 Tom> backwards-compatibility-wise, because the new default behavior
 Tom> shouldn't break any cases where people weren't cheating.

The cases where the new default will break things are where people used
WITH to force a choice of join order or otherwise constrain the planner
in order to avoid a misplan.

I'm not sure we should nail down the rule that the absence of NOT
MATERIALIZED will mean a multiply-referenced CTE is evaluated once. One
would hope that in the future the planner might be taught to inline or
not in that case depending on cost. I think it makes more sense to say
that we never inline if MATERIALIZED is specified, that we always inline
if NOT MATERIALIZED is specified, and that if neither is specified the
planner will choose (but perhaps note that currently it always chooses
only based on refcount).

-- 
Andrew (irc:RhodiumToad)

Re: backslash-dot quoting in COPY CSV

[Michael Paquier]

On Thu, Jan 24, 2019 at 10:09:30PM -0500, Bruce Momjian wrote:
> This seems like a bug to me.  Looking at the code, psql issues the
> prompts for STDIN, but when it sees \. alone on a line, it has no idea
> you are in a quoted CSV string, so it thinks the copy is done and sends
> the result to the server.  I can't see an easy way to fix this.  I guess
> we could document it.

In src/bin/psql/copy.c, handleCopyIn():

/*
 * This code erroneously assumes '\.' on a line alone
 * inside a quoted CSV string terminates the \copy.
 * 
http://www.postgresql.org/message-id/e1tdnvq-0001ju...@wrigleys.postgresql.org
 */
if (strcmp(buf, "\\.\n") == 0 ||
strcmp(buf, "\\.\r\n") == 0)
{
copydone = true;
break;
}

This story pops up from time to time..
--
Michael


signature.asc
Description: PGP signature

Re: pg_basebackup, walreceiver and wal_sender_timeout

[Michael Paquier]

On Sat, Jan 26, 2019 at 01:45:46PM +0100, Magnus Hagander wrote:
> One workaround you could perhaps look at here is to run pg_basebackup
> with --no-sync. That way there will be no fsyncs issued while running. You
> will then of course have to take care of syncing all the files to disk
> after it's done, but a network filesystem might be happier in dealing with
> a large "batch-sync" like that rather than piece-by-piece sync.

Hm.  Aren't we actually wrong in letting the WAL receive method use
the value of do_sync depending on the command line arguments, with
true being the default for pg_basebackup?  In plain format, we flush
the full data directory anyway when the backup ends.  In tar format,
each individual tar file is flushed one-by-one after being received,
and we issue a final sync on the parent directory at the end.  So
what's missing is just to make sure that the fully generated
pg_wal.tar is synced once completed.  This would be way cheaper than
letting the stream process issue syncs for each segments, which does
not matter much in the event of a host crash because the base backup
may finish in an inconsistent state, and one should not use it.
--
Michael


signature.asc
Description: PGP signature

Re: pg_upgrade: Pass -j down to vacuumdb

[Michael Paquier]

On Fri, Jan 25, 2019 at 12:16:49PM -0500, Tom Lane wrote:
> Alvaro Herrera  writes:
>> So let's have it write with a $VACUUMDB_OPTS variable, which is by
>> default defined as empty but with a comment suggesting that maybe the
>> user wants to add the -j option.  This way, if they have to edit it,
>> they only have to edit the VACUUMDB_OPTS line instead of each of the two
>> vacuumdb lines.
> 
> Even better, allow the script to absorb a value from the environment,
> so that it needn't be edited at all.

+1.
--
Michael


signature.asc
Description: PGP signature

Re: relcache reference leak with pglogical replication to insert-only partitioned table?

[Michael Paquier]

On Sat, Jan 26, 2019 at 09:19:49PM -0600, Jeremy Finzel wrote:
> I understand it's not fully supported to replicate to a differently
> partitioned setup on a subscriber with either pglogical or the native
> logical replication, however I also know that INSERT triggers can be fired
> in replication mode.  I have an insert-only OLTP table that I want
> partitioned only on the subscriber system.  I have this setup using the
> "old style" partitioning as it is a 9.6 system.

Logical replication has been added to core in v10, and pglogical is
quite a different thing.  It may be better to contact the folks in
charge of it here:
https://github.com/2ndQuadrant/pglogical
--
Michael


signature.asc
Description: PGP signature

Re: Opossum vs. float4 NaN

[Glyn Astill]

  >> On Saturday, 26 January 2019, 16:00:24 GMT, Tom Lane  
wrote:  >>
>> I'm thinking we should regretfully retire opossum, unless there's
>> a software update available for it that fixes this bug.
I'm happy to update opossum to see if the issue goes away; likewise I'm just as 
happy to retire it completely.  It'll still come online occasionally 
regardless, so makes no difference. 

I guess the main question is; does anybody care about builds on a 20 year old 
netbsd/mipsel dinosaur?  I noticed there are now mips64el and mips64eb 
build-farm members.



  

Re: Allowing extensions to supply operator-/function-specific info

[Simon Riggs]

On Sun, 20 Jan 2019 at 23:48, Tom Lane  wrote:


> What I'm envisioning therefore is that we allow an auxiliary function to
> be attached to any operator or function that can provide functionality
> like this, and that we set things up so that the set of tasks that
> such functions can perform can be extended over time without SQL-level
> changes.  For example, we could say that the function takes a single
> Node* argument, and that the type of Node tells it what to do, and if it
> doesn't recognize the type of Node it should just return NULL indicating
> "use default handling".  We'd start out with two relevant Node types,
> one for the selectivity-estimation case and one for the extract-a-lossy-
> index-qual case, and we could add more over time.
>

Does this help with these cases?

* Allow a set returning function to specify number of output rows, in cases
where that is variable and dependent upon the input params?

* Allow a normal term to match a functional index, e.g. WHERE x =
'abcdefgh' => WHERE substr(x, 1 , 5) = 'abcde' AND x = 'abcdefgh'

* Allow us to realise that ORDER BY f(x) => ORDER BY x so we can use
ordered paths from indexes, or avoid sorts.

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

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