Re: [HACKERS] [PATCH] Lockable views
On Mon, 16 Oct 2017 10:07:48 +0900 (JST) Tatsuo Ishii <is...@sraoss.co.jp> wrote: > >> >> test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; > >> >> CREATE VIEW > >> >> test=# BEGIN; > >> >> BEGIN > >> >> test=# LOCK TABLE v3; > >> >> ERROR: cannot lock view "v3" > >> >> DETAIL: Views that return aggregate functions are not automatically > >> >> updatable. > >> > > >> > It would be nice if the message would be something like: > >> > > >> > DETAIL: Views that return aggregate functions are not lockable > > > > This uses messages from view_query_is_auto_updatable() of the rewrite > > system directly. > > Although we can modify the messages, I think it is not necessary for now > > since we can lock only automatically updatable views. > > You could add a flag to view_query_is_auto_updatable() to switch the > message between > > DETAIL: Views that return aggregate functions are not automatically > updatable. > > and > > DETAIL: Views that return aggregate functions are not lockable OK. I'll change view_query_is_auto_updatable() so. > > >> > I wonder if we should lock tables in a subquery as well. For example, > >> > > >> > create view v1 as select * from t1 where i in (select i from t2); > >> > > >> > In this case should we lock t2 as well? > >> > >> Current the patch ignores t2 in the case above. > >> > >> So we have options below: > >> > >> - Leave as it is (ignore tables appearing in a subquery) > >> > >> - Lock all tables including in a subquery > >> > >> - Check subquery in the view definition. If there are some tables > >> involved, emit an error and abort. > >> > >> The first one might be different from what users expect. There may be > >> a risk that the second one could cause deadlock. So it seems the third > >> one seems to be the safest IMO. > > > > Make sense. Even if the view is locked, when tables in a subquery is > > modified, the contents of view can change. To avoid it, we have to > > lock tables, or give up to lock such views. > > > > We can say the same thing for functions in a subquery. If the definition > > of the functions are changed, the result of the view can change. > > We cannot lock functions, but should we abtain row-level lock on pg_proc > > in such cases? (of cause, or give up to lock such views) > > I think we don't need to care about function definition changes used > in where clauses in views. None view queries using functions do not > care about the definition changes of functions while executing the > query. So why updatable views need to care them? I'm a bit confused. What is difference between tables and functions in a subquery with regard to view locking? I think also none view queries using a subquery do not care about the changes of tables in the subquery while executing the query. I might be misnderstanding the problem you mentioned. BTW, I found that if we have to handle subqueries in where clause, we would also have to care about subqueries in target list... The view defined as below is also updatable. =# create view v7 as select (select * from tbl2 limit 1) from tbl; > > > BTW, though you mentioned the risk of deadlocks, even when there > > are no subquery, deadlock can occur in the current patch. > > > > For example, lock a table T in Session1, and then lock a view V > > whose base relation is T in Session2. Session2 will wait for > > Session1 to release the lock on T. After this, when Session1 try to > > lock view V, the deadlock occurs and the query is canceled. > > You are right. Dealocks could occur in any case. > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Lockable views
On Thu, 12 Oct 2017 13:11:45 +0900 (JST) Tatsuo Ishii <is...@sraoss.co.jp> wrote: > >> test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; > >> CREATE VIEW > >> test=# BEGIN; > >> BEGIN > >> test=# LOCK TABLE v3; > >> ERROR: cannot lock view "v3" > >> DETAIL: Views that return aggregate functions are not automatically > >> updatable. > > > > It would be nice if the message would be something like: > > > > DETAIL: Views that return aggregate functions are not lockable This uses messages from view_query_is_auto_updatable() of the rewrite system directly. Although we can modify the messages, I think it is not necessary for now since we can lock only automatically updatable views. > > > >> test=# END; > >> ROLLBACK > >> > >> test=# CREATE FUNCTION fnc() RETURNS trigger AS $$ BEGIN RETURN NEW; END; > >> $$ LANGUAGE plpgsql; > >> CREATE FUNCTION > >> test=# CREATE TRIGGER trg INSTEAD OF INSERT ON v1 FOR EACH ROW EXECUTE > >> PROCEDURE fnc(); > >> CREATE TRIGGER > >> test=# BEGIN; > >> BEGIN > >> test=# LOCK TABLE v1; > >> ERROR: cannot lock view "v1" > >> DETAIL: views that have an INSTEAD OF trigger are not lockable > >> test=# END; > >> ROLLBACK > > > > I wonder if we should lock tables in a subquery as well. For example, > > > > create view v1 as select * from t1 where i in (select i from t2); > > > > In this case should we lock t2 as well? > > Current the patch ignores t2 in the case above. > > So we have options below: > > - Leave as it is (ignore tables appearing in a subquery) > > - Lock all tables including in a subquery > > - Check subquery in the view definition. If there are some tables > involved, emit an error and abort. > > The first one might be different from what users expect. There may be > a risk that the second one could cause deadlock. So it seems the third > one seems to be the safest IMO. Make sense. Even if the view is locked, when tables in a subquery is modified, the contents of view can change. To avoid it, we have to lock tables, or give up to lock such views. We can say the same thing for functions in a subquery. If the definition of the functions are changed, the result of the view can change. We cannot lock functions, but should we abtain row-level lock on pg_proc in such cases? (of cause, or give up to lock such views) BTW, though you mentioned the risk of deadlocks, even when there are no subquery, deadlock can occur in the current patch. For example, lock a table T in Session1, and then lock a view V whose base relation is T in Session2. Session2 will wait for Session1 to release the lock on T. After this, when Session1 try to lock view V, the deadlock occurs and the query is canceled. Is this unacceptable behavior? > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Lockable views
Hi, Attached is a patch to enable views to be locked. PostgreSQL has supported automatically updatable views since 9.3, so we can udpate simply defined views like regular tables. However, currently, table-level locks on views are not supported. We can not execute LOCK TABLE for views, while we can get row-level locks by FOR UPDATE/SHARE. In some situations that we need table-level locks on tables, we may also need table-level locks on automatically updatable views. Although we can lock base-relations manually, it would be useful if we can lock views without knowing the definition of the views. In the attached patch, only automatically-updatable views that do not have INSTEAD OF rules or INSTEAD OF triggers are lockable. It is assumed that those views definition have only one base-relation. When an auto-updatable view is locked, its base relation is also locked. If the base relation is a view again, base relations are processed recursively. For locking a view, the view owner have to have he priviledge to lock the base relation. * Example test=# CREATE TABLE tbl (i int); CREATE TABLE test=# CREATE VIEW v1 AS SELECT * FROM tbl; CREATE VIEW test=# BEGIN; BEGIN test=# LOCK TABLE v1; LOCK TABLE test=# SELECT relname, locktype, mode FROM pg_locks,pg_class c WHERE c.oid=relation AND relname NOT LIKE 'pg%'; relname | locktype |mode -+--+- tbl | relation | AccessExclusiveLock v1 | relation | AccessExclusiveLock (2 rows) test=# END; COMMIT test=# CREATE VIEW v2 AS SELECT * FROM v1; CREATE VIEW test=# BEGIN; BEGIN test=# LOCK TABLE v2; LOCK TABLE test=# SELECT relname, locktype, mode FROM pg_locks,pg_class c WHERE c.oid=relation AND relname NOT LIKE 'pg%'; relname | locktype |mode -+--+- v2 | relation | AccessExclusiveLock tbl | relation | AccessExclusiveLock v1 | relation | AccessExclusiveLock (3 rows) test=# END; COMMIT test=# CREATE VIEW v3 AS SELECT count(*) FROM v1; CREATE VIEW test=# BEGIN; BEGIN test=# LOCK TABLE v3; ERROR: cannot lock view "v3" DETAIL: Views that return aggregate functions are not automatically updatable. test=# END; ROLLBACK test=# CREATE FUNCTION fnc() RETURNS trigger AS $$ BEGIN RETURN NEW; END; $$ LANGUAGE plpgsql; CREATE FUNCTION test=# CREATE TRIGGER trg INSTEAD OF INSERT ON v1 FOR EACH ROW EXECUTE PROCEDURE fnc(); CREATE TRIGGER test=# BEGIN; BEGIN test=# LOCK TABLE v1; ERROR: cannot lock view "v1" DETAIL: views that have an INSTEAD OF trigger are not lockable test=# END; ROLLBACK Regards, -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml index b946eab..5a431b7 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -46,6 +46,13 @@ LOCK [ TABLE ] [ ONLY ] name [ * ] + Automatically updatable views (see ) + that do not have INSTEAD OF triggers or INSTEAD + rules are also lockable. When a view is locked, its base relations are + also locked recursively with the same lock mode. + + + When acquiring locks automatically for commands that reference tables, PostgreSQL always uses the least restrictive lock mode possible. LOCK TABLE diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 9fe9e02..37fd028 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -19,15 +19,20 @@ #include "commands/lockcmds.h" #include "miscadmin.h" #include "parser/parse_clause.h" +#include "parser/parsetree.h" #include "storage/lmgr.h" #include "utils/acl.h" #include "utils/lsyscache.h" #include "utils/syscache.h" +#include "rewrite/rewriteHandler.h" +#include "access/heapam.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); -static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid); static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, void *arg); +static void LockViewRecurse(Oid reloid, Oid root_reloid, LOCKMODE lockmode, bool nowait); +static void LockViewCheck(Relation view, Query *viewquery); /* * LOCK TABLE @@ -62,8 +67,10 @@ LockTableCommand(LockStmt *lockstmt) RangeVarCallbackForLockTable, (void *) >mode); - if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); + if (get_rel_relkind(reloid) == RELKIND_VIEW) + LockViewRecurse(reloid, reloid, lockstmt->mode, lockstmt->nowait); + else if (recurse) + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); } } @@ -86,15 +93,17 @@ RangeVarCallbackForLockTab
Re: [HACKERS] Optional message to user when terminating/cancelling backend
On Sun, 3 Sep 2017 22:47:10 +0200
Daniel Gustafsson <dan...@yesql.se> wrote:
I have reviewed your latest patch.
I can apply this to the master branch and build this successfully,
and the behavior is as expected.
However, here are some comments and suggestions.
> 135 + char *buffer = palloc0(MAX_CANCEL_MSG);
> 136 +
> 137 + GetCancelMessage(, MAX_CANCEL_MSG);
> 138 + ereport(ERROR,
> 139 + (errcode(ERRCODE_QUERY_CANCELED),
> 140 +errmsg("canceling statement due to user request:
> \"%s\"",
> 141 + buffer)));
The memory for buffer is allocated here, but I think we can do this
in GetCancelMessage. Since the size of allocated memory is fixed
to MAX_CANCEL_MSG, it isn't neccesary to pass this to the function.
In addition, how about returning the message as the return value?
That is, we can define GetCancelMessage as following;
char * GetCancelMessage(void)
> 180 + r = SetBackendCancelMessage(pid, msg);
> 181 +
> 182 + if (r != -1 && r != strlen(msg))
> 183 + ereport(NOTICE,
> 184 + (errmsg("message is too long and has been
> truncated")));
> 185 + }
We can this error handling in SetBackendCancelMessage. I think this is better
because the truncation of message is done in this function. In addition,
we should use pg_mbcliplen for this truncation as done in truncate_identifier.
Else, multibyte character boundary is broken, and noises can slip in log
messages.
> 235 - int r = pg_signal_backend(PG_GETARG_INT32(0), SIGTERM);
> 236 + int r = pg_signal_backend(pid, SIGTERM, msg);
This line includes an unnecessary indentation change.
> 411 + * returns the length of message actually created. If the returned length
"the length of message" might be "the length of the message"
> 413 + * If the backend wasn't found and no message was set, -1 is returned.
> If two
This comment is incorrect since this function returns 0 when no message was set.
> 440 + strlcpy(slot->message, message, sizeof(slot->message));
> 441 + slot->len = strlen(slot->message);
> 442 + message_len = slot->len;
> 443 + SpinLockRelease(>mutex);
> 444 +
> 445 + return message_len;
This can return slot->len directly and the variable message_len is
unnecessary. However, if we handle the "too long message" error
in this function as suggested above, this does not have to
return anything.
Regards,
--
Yugo Nagata <nag...@sraoss.co.jp>
> > On 02 Sep 2017, at 02:21, Thomas Munro <thomas.mu...@enterprisedb.com>
> > wrote:
> >
> > On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson <dan...@yesql.se> wrote:
> >> Good point. I’ve attached a new version which issues a NOTICE on
> >> truncation
> >> and also addresses all other comments so far in this thread. Thanks a lot
> >> for
> >> the early patch reviews!"
> >
> > FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:
> >
> > make[3]: Entering directory
> > `/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
> > cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
> > 772
> > 972
> > make[3]: *** [postgres.bki] Error 1
>
> Thanks, I hadn’t spotted that yet. Attached is an updated patch using new
> OIDs
> to make it compile again.
>
> cheers ./daniel
>
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
ved by Filter: 6
>-> Seq Scan on h3 (cost=0.00..41.88 rows=26 width=4) (actual
> time=0.005..0.007 rows=1 loops=1)
> Filter: (id = ANY ('{1,5}'::integer[]))
> Rows Removed by Filter: 3
> Planning time: 0.720 ms
> Execution time: 0.074 ms
> (9 rows)
>
> postgres=# explain analyze select * from h where id = 1 or id = 5;;
> QUERY PLAN
>
>
> Append (cost=0.00..96.50 rows=50 width=4) (actual time=0.017..0.078 rows=2
> loops=1)
>-> Seq Scan on h1 (cost=0.00..48.25 rows=25 width=4) (actual
> time=0.015..0.019 rows=1 loops=1)
> Filter: ((id = 1) OR (id = 5))
> Rows Removed by Filter: 6
>-> Seq Scan on h3 (cost=0.00..48.25 rows=25 width=4) (actual
> time=0.005..0.010 rows=1 loops=1)
> Filter: ((id = 1) OR (id = 5))
> Rows Removed by Filter: 3
> Planning time: 0.396 ms
> Execution time: 0.139 ms
> (9 rows)
>
> Can not detach / attach / drop partition table.
>
> Best regards,
> young
>
>
> yonj1e.github.io
> yang...@highgo.com
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A suspicious code in pgoutput_startup().
On Tue, 15 Aug 2017 16:23:35 -0400
Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote:
> On 7/27/17 20:52, Yugo Nagata wrote:
> > 175 /* Check if we support requested protocol */
> > 176 if (data->protocol_version != LOGICALREP_PROTO_VERSION_NUM)
> > 177 ereport(ERROR,
> > 178 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > 179 errmsg("client sent proto_version=%d but we only
> > support protocol %d or lower",
> > 180 data->protocol_version,
> > LOGICALREP_PROTO_VERSION_NUM)));
> >
> > Although the if condition is not-equal, the error message says
> > "we only support protocol %d or lower". Is this intentional?
> > Or should this be fixed as below?
> >
> > 176 if (data->protocol_version > LOGICALREP_PROTO_VERSION_NUM)
> >
> > Attached is a simple patch in case of fixing.
>
> Fixed, thanks.
Thanks, too!
>
> --
> Peter Eisentraut http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A little improvementof ApplyLauncherMain loop code
On Tue, 15 Aug 2017 15:17:06 -0400 Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > On 8/1/17 02:28, Yugo Nagata wrote: > > When reading the logical replication code, I found that the following > > part could be improved a bit. In the foreach, LWLockAcquire and > > logicalrep_worker_find are called for each loop, but they are needed > > only when sub->enabled is true. > > Fixed, thanks! Thanks, too! > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Notice message of ALTER SUBSCRIPTION ... RERESH PUBLICATIION
On Mon, 7 Aug 2017 09:46:56 -0400 Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > On 7/27/17 20:51, Yugo Nagata wrote: > > When we run ALTER SUBSCRIPTION ... REFRESH PUBLICATION and there is > > an unkown table at local, it says; > > > > NOTICE: added subscription for table public.tbl > > > > I feel this a bit misleading because it says a subscription is added > > but actually new subscription object is not created. Of cause, I can > > understand the intention this message says, but I feel that > > it is better to use other expression. For example, how about saying > > > > NOTICE: table public.tbl is added to subscription sub1 > > > > as proposed in the attached patch. This also fixes the message > > when a table is removed from a subscription. > > Fixed, thanks. (Note that per another discussion, these are now DEBUG > messages.) Thanks, too. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Incorrect comment of XLByteToSeg() and XLByteToPrevSeg()
On Tue, 01 Aug 2017 08:11:23 +0900 (JST) Tatsuo Ishii <is...@sraoss.co.jp> wrote: > > Thanks for the patch. Looks good to me. I will commit/push into all > > supported branches if there's no objection. > > Done. Thanks! > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] A little improvementof ApplyLauncherMain loop code
Hi,
When reading the logical replication code, I found that the following
part could be improved a bit. In the foreach, LWLockAcquire and
logicalrep_worker_find are called for each loop, but they are needed
only when sub->enabled is true.
846 /* Start the missing workers for enabled subscriptions. */
847 foreach(lc, sublist)
848 {
849 Subscription *sub = (Subscription *) lfirst(lc);
850 LogicalRepWorker *w;
851
852 LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
853 w = logicalrep_worker_find(sub->oid, InvalidOid, false);
854 LWLockRelease(LogicalRepWorkerLock);
855
856 if (sub->enabled && w == NULL)
857 {
858 last_start_time = now;
859 wait_time = wal_retrieve_retry_interval;
860
861 logicalrep_worker_launch(sub->dbid, sub->oid,
sub->name,
862 sub->owner, InvalidOid);
863 }
864 }
We can fix this to call them only when there are needed, but I'm not sure
whether these call are expensive enough to fix. Is it worth to fix?
A patch attached.
Regards,
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index d165d51..4816a5b 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -849,11 +849,14 @@ ApplyLauncherMain(Datum main_arg)
Subscription *sub = (Subscription *) lfirst(lc);
LogicalRepWorker *w;
+if (!sub->enabled)
+ continue;
+
LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
w = logicalrep_worker_find(sub->oid, InvalidOid, false);
LWLockRelease(LogicalRepWorkerLock);
-if (sub->enabled && w == NULL)
+if (w == NULL)
{
last_start_time = now;
wait_time = wal_retrieve_retry_interval;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] A suspicious code in pgoutput_startup().
Hi,
I found a suspicious code in pgoutput_startup().
175 /* Check if we support requested protocol */
176 if (data->protocol_version != LOGICALREP_PROTO_VERSION_NUM)
177 ereport(ERROR,
178 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
179 errmsg("client sent proto_version=%d but we only
support protocol %d or lower",
180 data->protocol_version,
LOGICALREP_PROTO_VERSION_NUM)));
Although the if condition is not-equal, the error message says
"we only support protocol %d or lower". Is this intentional?
Or should this be fixed as below?
176 if (data->protocol_version > LOGICALREP_PROTO_VERSION_NUM)
Attached is a simple patch in case of fixing.
Regards,
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 4a56288..370b74f 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -173,7 +173,7 @@ pgoutput_startup(LogicalDecodingContext *ctx, OutputPluginOptions *opt,
>publication_names);
/* Check if we support requested protocol */
- if (data->protocol_version != LOGICALREP_PROTO_VERSION_NUM)
+ if (data->protocol_version > LOGICALREP_PROTO_VERSION_NUM)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("client sent proto_version=%d but we only support protocol %d or lower",
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Notice message of ALTER SUBSCRIPTION ... RERESH PUBLICATIION
Hi,
When we run ALTER SUBSCRIPTION ... REFRESH PUBLICATION and there is
an unkown table at local, it says;
NOTICE: added subscription for table public.tbl
I feel this a bit misleading because it says a subscription is added
but actually new subscription object is not created. Of cause, I can
understand the intention this message says, but I feel that
it is better to use other expression. For example, how about saying
NOTICE: table public.tbl is added to subscription sub1
as proposed in the attached patch. This also fixes the message
when a table is removed from a subscription.
Regards,
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 6dc3f6e..5a7e6d5 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -573,9 +573,10 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
copy_data ? SUBREL_STATE_INIT : SUBREL_STATE_READY,
InvalidXLogRecPtr, false);
ereport(NOTICE,
- (errmsg("added subscription for table %s.%s",
+ (errmsg("table %s.%s is added to subscription %s",
quote_identifier(rv->schemaname),
- quote_identifier(rv->relname;
+ quote_identifier(rv->relname),
+ quote_identifier(sub->name;
}
}
@@ -601,9 +602,10 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
namespace = get_namespace_name(get_rel_namespace(relid));
ereport(NOTICE,
- (errmsg("removed subscription for table %s.%s",
+ (errmsg("table %s.%s is removed from subscription %s",
quote_identifier(namespace),
- quote_identifier(get_rel_name(relid);
+ quote_identifier(get_rel_name(relid)),
+ quote_identifier(sub->name;
}
}
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Missing comment for max_logical_replication_workers in postgresql.conf.sample
On Thu, 27 Jul 2017 14:38:29 +0900 Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Thu, Jul 27, 2017 at 10:14 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > Hi, > > > > I found that postgresql.conf.sample is missing a comment > > to note that changing max_logical_replication_workers requires > > restart of the server. > > > > Other such parameters has the comments, so I think the new > > parameter also needs this. Attached is a simple patch to fix > > this. > > > > Good point. Similarly, dynamic_shared_memory_type and event_source are > required restarting to change but are not mentioned in > postgresql.conf.sample. Should we add a comment as well? I think so. The updated patch is attached. > > Regards, > > -- > Masahiko Sawada > NIPPON TELEGRAPH AND TELEPHONE CORPORATION > NTT Open Source Software Center -- Yugo Nagata <nag...@sraoss.co.jp> postgresql.conf.sample.patch.v2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Missing comment for max_logical_replication_workers in postgresql.conf.sample
Hi, I found that postgresql.conf.sample is missing a comment to note that changing max_logical_replication_workers requires restart of the server. Other such parameters has the comments, so I think the new parameter also needs this. Attached is a simple patch to fix this. Regards, -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 2b1ebb7..d624ad3 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -277,6 +277,7 @@ # These settings are ignored on a publisher. #max_logical_replication_workers = 4 # taken from max_worker_processes + # (change requires restart) #max_sync_workers_per_subscription = 2 # taken from max_logical_replication_workers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] A hook for session start
On Fri, 21 Jul 2017 10:31:57 -0300 Fabrízio de Royes Mello <fabriziome...@gmail.com> wrote: > On Fri, Jul 21, 2017 at 9:35 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > > > On Fri, 21 Jul 2017 09:53:19 +0800 > > Craig Ringer <cr...@2ndquadrant.com> wrote: > > > > > On 21 July 2017 at 08:42, Robert Haas <robertmh...@gmail.com> wrote: > > > > > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello > > > > <fabriziome...@gmail.com> wrote: > > > > > I'm not sure your real needs but doesn't it material for improve > Event > > > > > Triggers??? > > > > > > > > I've thought about that, too. One problem is what to do if the user > > > > hits ^C while the event trigger procedure is running. If you respond > > > > to that by killing the event trigger and letting the user issue > > > > commands, then the event trigger can't be used for security or > > > > auditing purposes because the user might prevent it from doing > > > > whatever it's intended to do with a well-timed interrupt. If you > > > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger > > > > can lock users out of the database. Maybe that's OK. We could say > > > > "well, if you lock yourself out of the database with your logon > > > > trigger, you get to shut down the database and restart in single user > > > > mode to recover". > > > > > > > > A hook, as proposed here, is a lot simpler and lacks these concerns. > > > > Installing code in C into the database is intrinsically risky > > > > anywhere, and not any moreso here than elsewhere. But it's also less > > > > accessible to the average user. > > > > <http://www.postgresql.org/mailpref/pgsql-hackers> > > > > > > > > > I'd favour the c hook personally. It's a lot more flexible, and can be > used > > > by an extension to implement trigger-like behaviour if anyone wants it, > > > including the extension's choice of error handling decisions. > > > > > > It's also a lot simpler and less intrusive for core. Which is nice > where we > > > don't have something that we don't have anything compelling destined for > > > core that needs it. (I want to add a bunch of hooks in the logical > > > replication code in pg11 for similar reasons, and so features like DDL > > > replication can be prototyped as extensions more practically). > > > > > I agree with you both... > > > > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve > the > > > same job as a session-start hook, albeit at slightly higher overhead? > You > > > can just test to see if your initial tasks have run yet. > > > > Thank you for your suggestion. Certainly, we can do the similar job of a > > session-start hook using these existing hooks, although these hooks are > > triggered when the first query is executed not when the session is > started. > > Now I come to think that an additional hook is not need. > > > > As Nagata said hooks proposed by Craing will happens only when the first > query is called so I don't know how it works for session start... are we > missing something? Yes, ExecutorStart_hook + ProcessUtility_hook is not strictly same as session_start hook. If a query is issued a long time since the session start, the timing the hook happens is largely deviated. It is no problem if we only want do something once at the session start, but it might be problem if we want to record the timestamp of the session start, for example. > > If we're going to add this hook what about add a session end hook also? If someone want the session-start hook, he might want this too. > > Regards, > > -- > Fabrízio de Royes Mello > Consultoria/Coaching PostgreSQL > >> Timbira: http://www.timbira.com.br > >> Blog: http://fabriziomello.github.io > >> Linkedin: http://br.linkedin.com/in/fabriziomello > >> Twitter: http://twitter.com/fabriziomello > >> Github: http://github.com/fabriziomello -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make sure all statistics is sent after a few DML are performed
On Fri, 21 Jul 2017 04:58:47 -0700 Andres Freund <and...@anarazel.de> wrote: > Hi, > > (please don't top-reply on this list) > > On 2017-07-19 14:04:39 +0900, Yugo Nagata wrote: > > On Tue, 18 Jul 2017 10:10:49 -0400 > > Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Thank you for your comments. I understand the problem of my proposal > > patch. > > Does that mean you're trying to rewrite it in the way that was > suggested: Not yet, but I'll try to do it. > > > > > Another, > > > > pretty half-baked, approach would be to add a procsignal triggering idle > > > > backends to send stats, and send that to all idle backends when querying > > > > stats. We could even publish the number of outstanding stats updates in > > > > PGXACT or such, without any locking, and send it only to those that have > > > > outstanding ones. > > > > > > If somebody wanted to do the work, that'd be a viable answer IMO. You'd > > > really want to not wake backends that have nothing more to send, but > > > I agree that it'd be possible to advertise that in shared memory. > > or are you planning to just let the issue leave be? > > - Andres -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] A hook for session start
On Fri, 21 Jul 2017 09:53:19 +0800 Craig Ringer <cr...@2ndquadrant.com> wrote: > On 21 July 2017 at 08:42, Robert Haas <robertmh...@gmail.com> wrote: > > > On Thu, Jul 20, 2017 at 8:27 AM, Fabrízio de Royes Mello > > <fabriziome...@gmail.com> wrote: > > > I'm not sure your real needs but doesn't it material for improve Event > > > Triggers??? > > > > I've thought about that, too. One problem is what to do if the user > > hits ^C while the event trigger procedure is running. If you respond > > to that by killing the event trigger and letting the user issue > > commands, then the event trigger can't be used for security or > > auditing purposes because the user might prevent it from doing > > whatever it's intended to do with a well-timed interrupt. If you > > ignore ^C or make it turn into FATAL, then a poorly-crafted trigger > > can lock users out of the database. Maybe that's OK. We could say > > "well, if you lock yourself out of the database with your logon > > trigger, you get to shut down the database and restart in single user > > mode to recover". > > > > A hook, as proposed here, is a lot simpler and lacks these concerns. > > Installing code in C into the database is intrinsically risky > > anywhere, and not any moreso here than elsewhere. But it's also less > > accessible to the average user. > > <http://www.postgresql.org/mailpref/pgsql-hackers> > > > I'd favour the c hook personally. It's a lot more flexible, and can be used > by an extension to implement trigger-like behaviour if anyone wants it, > including the extension's choice of error handling decisions. > > It's also a lot simpler and less intrusive for core. Which is nice where we > don't have something that we don't have anything compelling destined for > core that needs it. (I want to add a bunch of hooks in the logical > replication code in pg11 for similar reasons, and so features like DDL > replication can be prototyped as extensions more practically). > > That said, isn't ExecutorStart_hook + ProcessUtility_hook able to serve the > same job as a session-start hook, albeit at slightly higher overhead? You > can just test to see if your initial tasks have run yet. Thank you for your suggestion. Certainly, we can do the similar job of a session-start hook using these existing hooks, although these hooks are triggered when the first query is executed not when the session is started. Now I come to think that an additional hook is not need. Thanks, > > -- > Craig Ringer http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] xlogfilename
On Fri, 21 Jul 2017 10:11:05 +0800 Craig Ringer <cr...@2ndquadrant.com> wrote: > On 20 July 2017 at 21:33, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > > On Thu, 20 Jul 2017 11:02:25 +0200 > > Michael Paquier <michael.paqu...@gmail.com> wrote: > > > > > On Thu, Jul 20, 2017 at 10:58 AM, DEV_OPS <dev...@ww-it.cn> wrote: > > > > I think you may reference to function: pg_xlogfile_name in > > > > src/backend/access/transam/xlogfuncs.c, it use XLogFileName defined > > in > > > > src/include/access/xlog_internal.h > > > > > > > > #define XLogFileName(fname, tli, logSegNo) \ > > > > snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli, \ > > > > (uint32) ((logSegNo) / XLogSegmentsPerXLogId), \ > > > > (uint32) ((logSegNo) % XLogSegmentsPerXLogId)) > > > > > > > > > > > > hope it's helpful for you > > > > > > The first 8 characters are the timeline number in hexadecimal format. > > > The next 8 characters indicate a segment number, which gets > > > incremented every 256 segments in hexa format. The last 8 characters > > > indicate the current segment number in hexa format. > > > > As far as I understand, XLOG is a logical big file of 256 * 16 MB, > > and this is split to multiple physical files of 16MB which are called > > WAL segments. The second 8 characters indicate the id of the logical > > xlog file, and the last 8 characters indicate the sequencial number of > > the segment in this xlog. > > <http://www.postgresql.org/mailpref/pgsql-hackers> > > > > You missed the timeline ID, which is the first 8 digits. Yes, I missed this. Thanks. > > > -- > Craig Ringer http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Incorrect comment of XLByteToSeg() and XLByteToPrevSeg()
Hi, I found a type in the comment for XLByteToSeg() and XLByteToPrevSeg(). This says "Compute ID and segment from an XLogRecPtr", but these macros compute only segment numbers. I think "Compute a segment number from an XLogRecPtr" is correct. The definition of these macros were modified by the following commit, but the comment were not. commit dfda6ebaec6763090fb78b458a979b558c50b39b Author: Heikki Linnakangas <heikki.linnakan...@iki.fi> Date: Sun Jun 24 18:06:38 2012 +0300 Regards, -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index a661ec0..7453dcb 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -96,7 +96,7 @@ typedef XLogLongPageHeaderData *XLogLongPageHeader; (dest) = (segno) * XLOG_SEG_SIZE + (offset) /* - * Compute ID and segment from an XLogRecPtr. + * Compute a segment number from an XLogRecPtr. * * For XLByteToSeg, do the computation at face value. For XLByteToPrevSeg, * a boundary byte is taken to be in the previous segment. This is suitable -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] xlogfilename
On Thu, 20 Jul 2017 11:02:25 +0200 Michael Paquier <michael.paqu...@gmail.com> wrote: > On Thu, Jul 20, 2017 at 10:58 AM, DEV_OPS <dev...@ww-it.cn> wrote: > > I think you may reference to function: pg_xlogfile_name in > > src/backend/access/transam/xlogfuncs.c, it use XLogFileName defined in > > src/include/access/xlog_internal.h > > > > #define XLogFileName(fname, tli, logSegNo) \ > > snprintf(fname, MAXFNAMELEN, "%08X%08X%08X", tli, \ > > (uint32) ((logSegNo) / XLogSegmentsPerXLogId), \ > > (uint32) ((logSegNo) % XLogSegmentsPerXLogId)) > > > > > > hope it's helpful for you > > The first 8 characters are the timeline number in hexadecimal format. > The next 8 characters indicate a segment number, which gets > incremented every 256 segments in hexa format. The last 8 characters > indicate the current segment number in hexa format. As far as I understand, XLOG is a logical big file of 256 * 16 MB, and this is split to multiple physical files of 16MB which are called WAL segments. The second 8 characters indicate the id of the logical xlog file, and the last 8 characters indicate the sequencial number of the segment in this xlog. Regards, > -- > Michael > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] A hook for session start
Hi,
Currently, PostgreSQL doen't have a hook triggered at session
start. Although we already have ClientAuthentication_hook,
this is triggered during authentication, so we can not
access the database.
If we have a hook triggerd only once at session start, we may
do something useful on the session for certain database or user.
For example, one of our clients wanted such feature. He wanted
to handle encription for specific users, though I don't know
the detail.
The attached patch (session_start_hook.patch) implements such
hook.
Another patch, session_start_sample.patch, is a very simple
example of this hook that changes work_mem values for sessions
of a specific database.
I would appreciate hearing your opinion on this hook.
Regards,
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b8d860e..7a1fa3b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -160,6 +160,9 @@ static bool RecoveryConflictPending = false;
static bool RecoveryConflictRetryable = true;
static ProcSignalReason RecoveryConflictReason;
+/* Hook for plugins to get control at start of session */
+session_start_hook_type session_start_hook = NULL;
+
/*
* decls for routines only used in this file
*
@@ -3808,6 +3811,9 @@ PostgresMain(int argc, char *argv[],
if (!IsUnderPostmaster)
PgStartTime = GetCurrentTimestamp();
+ if (session_start_hook)
+ (*session_start_hook) (dbname, username);
+
/*
* POSTGRES main processing loop begins here
*
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index f8c535c..d349592 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -35,6 +35,11 @@ extern PGDLLIMPORT const char *debug_query_string;
extern int max_stack_depth;
extern int PostAuthDelay;
+/* Hook for plugins to get control at start of session */
+typedef void (*session_start_hook_type) (const char *dbname,
+ const char *username);
+extern PGDLLIMPORT session_start_hook_type session_start_hook;
+
/* GUC-configurable parameters */
typedef enum
diff --git a/contrib/session_start/Makefile b/contrib/session_start/Makefile
new file mode 100644
index 000..f94355b
--- /dev/null
+++ b/contrib/session_start/Makefile
@@ -0,0 +1,15 @@
+# contrib/session_start/Makefile
+
+MODULES = session_start
+PGFILEDESC = "session_start - sample for session start hook"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/session_start
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/session_start/session_start.c b/contrib/session_start/session_start.c
new file mode 100644
index 000..1792879
--- /dev/null
+++ b/contrib/session_start/session_start.c
@@ -0,0 +1,53 @@
+/* -
+ *
+ * session_start.c
+ *
+ * Copyright (c) 2010-2017, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/session_start/session_start.c
+ *
+ * -
+ */
+#include "postgres.h"
+
+#include "executor/spi.h"
+#include "tcop/tcopprot.h"
+
+PG_MODULE_MAGIC;
+
+void _PG_init(void);
+
+/* Original Hook */
+static session_start_hook_type original_session_start_hook = NULL;
+
+/* sample hook function */
+static void
+sample_session_start_hook(const char *dbname, const char *username)
+{
+ CommandDest back;
+
+ if (original_session_start_hook)
+ original_session_start_hook(dbname, username);
+
+ if (!strcmp(dbname, "test"))
+ {
+ StartTransactionCommand();
+ SPI_connect();
+ SPI_exec("set work_mem to 10240", 1);
+ SPI_finish();
+ CommitTransactionCommand();
+ }
+}
+
+/*
+ * Module Load Callback
+ */
+void
+_PG_init(void)
+{
+ /* Install Hooks */
+
+ original_session_start_hook = session_start_hook;
+ session_start_hook = sample_session_start_hook;
+}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Make sure all statistics is sent after a few DML are performed
On Tue, 18 Jul 2017 10:10:49 -0400 Tom Lane <t...@sss.pgh.pa.us> wrote: Thank you for your comments. I understand the problem of my proposal patch. > Andres Freund <and...@anarazel.de> writes: > > On 2017-07-18 09:42:31 -0400, Tom Lane wrote: > >> I wonder if a better answer wouldn't be to reduce PGSTAT_STAT_INTERVAL. > > > Not sure if that really does that much to solve the concern. > > Well, it reduces the amount of data churn that a statement shorter than > PGSTAT_STAT_INTERVAL could cause. > > > Another, > > pretty half-baked, approach would be to add a procsignal triggering idle > > backends to send stats, and send that to all idle backends when querying > > stats. We could even publish the number of outstanding stats updates in > > PGXACT or such, without any locking, and send it only to those that have > > outstanding ones. > > If somebody wanted to do the work, that'd be a viable answer IMO. You'd > really want to not wake backends that have nothing more to send, but > I agree that it'd be possible to advertise that in shared memory. > > regards, tom lane -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Make sure all statistics is sent after a few DML are performed
Hi,
After a DML is perfomed, the statistics is sent to pgstat by
pgsent_report_sent().
However, the mininum time between stas file update is PGSTAT_STAT_INTERVAL, so
if
a few DMLs are performed with short interval, some statistics could not be sent
until the backend is shutdown.
This is not a problem in usual cases, but in case that a session is remained in
idle for a long time, for example when using connection pooling, statistics of
a huge change of a table is not sent for a long time, and as a result, starting
autovacuum might be delayed.
An idea to resolve this is call pgsent_report_sent() again with a delay
after the last DML to make sure to send the statistics. The attached patch
implements this.
Any comments would be appreciated.
Regards,
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index b8d860e..928d479 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3012,6 +3012,12 @@ ProcessInterrupts(void)
}
+ if (PgStatReportStatTimeoutPending)
+ {
+ pgstat_report_stat(false);
+ PgStatReportStatTimeoutPending = false;
+ }
+
if (ParallelMessagePending)
HandleParallelMessages();
}
@@ -4010,6 +4016,14 @@ PostgresMain(int argc, char *argv[],
ProcessCompletedNotifies();
pgstat_report_stat(false);
+/* Call pgstat_report_stat() after PGSTAT_REPORT_STAT_DELAY
+ * again because if DMLs are performed with interval shorter
+ * than PGSTAT_STAT_INTERVAL then some statistics could not be
+ * sent until the backend is shutdown.
+ */
+enable_timeout_after(PGSTAT_REPORT_STAT_TIMEOUT,
+ PGSTAT_REPORT_STAT_DELAY);
+
set_ps_display("idle", false);
pgstat_report_activity(STATE_IDLE, NULL);
}
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 7c09498..8c29ebd 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -31,6 +31,7 @@ volatile bool QueryCancelPending = false;
volatile bool ProcDiePending = false;
volatile bool ClientConnectionLost = false;
volatile bool IdleInTransactionSessionTimeoutPending = false;
+volatile bool PgStatReportStatTimeoutPending = false;
volatile sig_atomic_t ConfigReloadPending = false;
volatile uint32 InterruptHoldoffCount = 0;
volatile uint32 QueryCancelHoldoffCount = 0;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index eb6960d..1df30bc 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -71,6 +71,7 @@ static void ShutdownPostgres(int code, Datum arg);
static void StatementTimeoutHandler(void);
static void LockTimeoutHandler(void);
static void IdleInTransactionSessionTimeoutHandler(void);
+static void PgStatReportStatTimeoutHandler(void);
static bool ThereIsAtLeastOneRole(void);
static void process_startup_options(Port *port, bool am_superuser);
static void process_settings(Oid databaseid, Oid roleid);
@@ -599,6 +600,8 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
RegisterTimeout(LOCK_TIMEOUT, LockTimeoutHandler);
RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
IdleInTransactionSessionTimeoutHandler);
+ RegisterTimeout(PGSTAT_REPORT_STAT_TIMEOUT,
+ PgStatReportStatTimeoutHandler);
}
/*
@@ -1196,6 +1199,14 @@ IdleInTransactionSessionTimeoutHandler(void)
SetLatch(MyLatch);
}
+static void
+PgStatReportStatTimeoutHandler(void)
+{
+ PgStatReportStatTimeoutPending = true;
+ InterruptPending = true;
+ SetLatch(MyLatch);
+}
+
/*
* Returns true if at least one role is defined in this database cluster.
*/
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index dad98de..4bc6f0f 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -81,6 +81,7 @@ extern PGDLLIMPORT volatile bool InterruptPending;
extern PGDLLIMPORT volatile bool QueryCancelPending;
extern PGDLLIMPORT volatile bool ProcDiePending;
extern PGDLLIMPORT volatile bool IdleInTransactionSessionTimeoutPending;
+extern PGDLLIMPORT volatile bool PgStatReportStatTimeoutPending;
extern PGDLLIMPORT volatile sig_atomic_t ConfigReloadPending;
extern volatile bool ClientConnectionLost;
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 6bffe63..a06703f 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -33,6 +33,11 @@
/* Default directory to store temporary statistics data in */
#define PG_STAT_TMP_DIR "pg_stat_tmp"
+/* How long to wait before calling pgstat_stat_report after
+ * the last DML is performed; in milliseconds.
+ */
+#define PGSTAT_REPORT_STAT_DELAY 500
+
/* Values for track_functions GUC variable --- order is significant! */
typedef enum TrackFunctionsLevel
{
diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h
index 5a2efc0..7eccd2d 100644
--- a/src/include/utils/timeout.h
+++ b/src/include/utils/timeout.h
@@ -31,6 +31,7 @@ typedef enum T
Re: [HACKERS] Fix doc of DROP SUBSCRIPTION
On Fri, 30 Jun 2017 20:17:39 +0900
Masahiko Sawada <sawada.m...@gmail.com> wrote:
> On Fri, Jun 30, 2017 at 7:01 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> > Hi,
> >
> > The documentation says that a subscription that has a replication slot
> > cannot be dropped in a transaction block, but it is not allowed even
> > outside of a transaction block.
>
> Hmm, I think we can drop a subscription outside of a transaction block
> even if the subscription associates with a replication.
Sorry, I was wrong and missing something... I confirmaed it.
The documentation is right. Sorry for the noise.
>
> =# table pg_subscription;
> subdbid | subname | subowner | subenabled |subconninfo
> | subslotname | subsynccommit | subpublications
> -+--+--++---+-+---+-
>13164 | hoge_sub | 10 | t | dbname=postgres
> port=5550 | hoge_sub| off | {one_pub}
> (1 row)
>
> =# drop subscription hoge_sub ;
> DROP SUBSCRIPTION
>
> Regards,
>
> --
> Masahiko Sawada
> NIPPON TELEGRAPH AND TELEPHONE CORPORATION
> NTT Open Source Software Center
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix doc of DROP SUBSCRIPTION
Hi, The documentation says that a subscription that has a replication slot cannot be dropped in a transaction block, but it is not allowed even outside of a transaction block. Attached is a patch to fix it. Regards, -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml index f535c00..bb48578 100644 --- a/doc/src/sgml/ref/drop_subscription.sgml +++ b/doc/src/sgml/ref/drop_subscription.sgml @@ -38,10 +38,9 @@ DROP SUBSCRIPTION [ IF EXISTS ] name - DROP SUBSCRIPTION cannot be executed inside a - transaction block if the subscription is associated with a replication - slot. (You can use ALTER SUBSCRIPTION to unset the - slot.) + DROP SUBSCRIPTION cannot be executed if the subscription + is associated with a replication slot. (You can use ALTER + SUBSCRIPTION to unset the slot.) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_reload_backend to signal SIGHUP to a specific backend
On Wed, 28 Jun 2017 13:59:51 +0200
Remi Colinet <remi.coli...@gmail.com> wrote:
> Hi Yugo,
>
> The patch looks fine. Installed and tested.
Thank you for your interest.
>
> But a similar function already exists for the Postmaster process.
>
> Datum
> pg_reload_conf(PG_FUNCTION_ARGS)
> {
> if (kill(PostmasterPid, SIGHUP))
> {
> ereport(WARNING,
> (errmsg("failed to send signal to
> postmaster: %m")));
> PG_RETURN_BOOL(false);
> }
>
> PG_RETURN_BOOL(true);
> }
>
> I would suggest to merge your patch with above function which is close to
> yours.
> Btw, your patch looks better that above function code.
>
> You may for instance retain pg_reload_conf() without argument for the same
> purpose as currently (Postmaster signaling).
> And provide a pid argument to signal a given backend.
Actually, I proposed that version patch in the previous thread[1], but there
as a suggestion to use other function name, so I attached fixed version
in this thread.
[1]
https://www.postgresql.org/message-id/20170624000156.74ca9485.nagata%40sraoss.co.jp
>
> Regards
> Remi
>
>
> 2017-06-28 12:17 GMT+02:00 Yugo Nagata <nag...@sraoss.co.jp>:
>
> > Hi,
> >
> > Attached is a patch of pg_reload_backend that is a function signaling
> > SIGHUP to a specific backend. The original idea is from Michael Paquier[1].
> > The documatation isn't included in this patch yet.
> >
> > We can change some parameters of other backend using the function
> > as bellow.
> >
> > postgres=# alter system set log_min_messages to debug2;
> > ALTER SYSTEM
> > postgres=# select pg_reload_backend(18375);
> > pg_reload_backend
> > ---
> > t
> > (1 row)
> >
> > There are some usecases. For example:
> >
> > - changing log configuration in other backends temporally.
> > - changing cost parameters for planner in other backends.
> > - changing autovacuum parameters of an already running autovacuum worker.
> >
> >
> > Hoever, this function need the superuser previlige to execute as well as
> > pg_reload_conf(). Although we can grant the execution to public, it is
> > useless for normal users bacause only superuser can change postgresql.conf.
> >
> > To allow normal users to change parameters in ohter backends, instead
> > we might want another feature, for example, a function like set_config()
> > than can change other backend's parameters using shared memory and SIGUSR1.
> >
> > Any comments would be appreciated.
> >
> > Regards,
> >
> > [1]
> > https://www.postgresql.org/message-id/CAB7nPqT4y8-
> > QoGKEugk99_tFuEOtAshcs5kxOeZ_0w27UtdGyA%40mail.gmail.com
> >
> > --
> > Yugo Nagata <nag...@sraoss.co.jp>
> >
> >
> > --
> > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> > To make changes to your subscription:
> > http://www.postgresql.org/mailpref/pgsql-hackers
> >
> >
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_reload_backend to signal SIGHUP to a specific backend
On Wed, 28 Jun 2017 13:35:12 +0200 Dmitry Dolgov <9erthali...@gmail.com> wrote: > > On 28 June 2017 at 12:17, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > > > Hi, > > > > Attached is a patch of pg_reload_backend that is a function signaling > > SIGHUP to a specific backend. The original idea is from Michael > Paquier[1]. > > The documatation isn't included in this patch yet. > > I have few questions. I'm curious, why this function returns something > different from bool when I'm passing null as an argument? This is because this function is defined as strict, that is pg_proc.proisstrict is true, as well as pg_reload_conf, pg_terminate_backend, pg_cancel_bacnend, and so on. > > =# select pg_reload_backend(27961); > WARNING: PID 27961 is not a PostgreSQL server process > WARNING: failed to send signal to backend: 27961 > pg_reload_backend > --- > f > (1 row) > > =# select pg_reload_backend(27962); > pg_reload_backend > --- > t > (1 row) > > =# select pg_reload_backend(null); > pg_reload_backend > --- > > (1 row) > > Also for some reason I can't grant an execute permission on this function, > am I doing something wrong? > > =# grant execute on function pg_reload_backend() to test_user; > ERROR: function pg_reload_backend() does not exist > =# grant execute on function pg_reload_conf() to test_user; > GRANT Arg type is needed. =# grant execute on function pg_reload_backend(int) to test_user; -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] pg_reload_backend to signal SIGHUP to a specific backend
Hi,
Attached is a patch of pg_reload_backend that is a function signaling
SIGHUP to a specific backend. The original idea is from Michael Paquier[1].
The documatation isn't included in this patch yet.
We can change some parameters of other backend using the function
as bellow.
postgres=# alter system set log_min_messages to debug2;
ALTER SYSTEM
postgres=# select pg_reload_backend(18375);
pg_reload_backend
---
t
(1 row)
There are some usecases. For example:
- changing log configuration in other backends temporally.
- changing cost parameters for planner in other backends.
- changing autovacuum parameters of an already running autovacuum worker.
Hoever, this function need the superuser previlige to execute as well as
pg_reload_conf(). Although we can grant the execution to public, it is
useless for normal users bacause only superuser can change postgresql.conf.
To allow normal users to change parameters in ohter backends, instead
we might want another feature, for example, a function like set_config()
than can change other backend's parameters using shared memory and SIGUSR1.
Any comments would be appreciated.
Regards,
[1]
https://www.postgresql.org/message-id/CAB7nPqT4y8-QoGKEugk99_tFuEOtAshcs5kxOeZ_0w27UtdGyA%40mail.gmail.com
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 0fdad0c..3a9a020 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1128,6 +1128,7 @@ REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause() FROM public;
REVOKE EXECUTE ON FUNCTION pg_wal_replay_resume() FROM public;
REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public;
REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_reload_backend(int) FROM public;
REVOKE EXECUTE ON FUNCTION pg_current_logfile() FROM public;
REVOKE EXECUTE ON FUNCTION pg_current_logfile(text) FROM public;
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 62341b8..83b6c45 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -339,6 +339,36 @@ pg_reload_conf(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(true);
}
+/*
+ * Signal to reload the database configuration for a specified backend
+ *
+ * Permission checking for this function is managed through the normal
+ * GRANT system.
+ */
+Datum
+pg_reload_backend(PG_FUNCTION_ARGS)
+{
+ int pid = PG_GETARG_INT32(0);
+ int r = pg_signal_backend(pid, SIGHUP);
+
+ if (r == SIGNAL_BACKEND_NOSUPERUSER)
+ ereport(WARNING,
+(errmsg("must be a superuser to reload superuser process")));
+
+ if (r == SIGNAL_BACKEND_NOPERMISSION)
+ ereport(WARNING,
+ (errmsg("must be a member of the role whose process is being reloaded or member of pg_signal_backend")));
+
+ if (r)
+ {
+ ereport(WARNING,
+(errmsg("failed to send signal to backend: %d", pid)));
+ PG_RETURN_BOOL(false);
+ }
+
+ PG_RETURN_BOOL(true);
+}
+
/*
* Rotate log file
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 1191b4a..a05e78a 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3255,6 +3255,8 @@ DESCR("true if wal replay is paused");
DATA(insert OID = 2621 ( pg_reload_conf PGNSP PGUID 12 1 0 0 0 f f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_reload_conf _null_ _null_ _null_ ));
DESCR("reload configuration files");
+DATA(insert OID = 3409 ( pg_reload_backend PGNSP PGUID 12 1 0 0 0 f f f f t f v s 1 0 16 "23" _null_ _null_ _null_ _null_ _null_ pg_reload_backend _null_ _null_ _null_ ));
+DESCR("reload configuration files");
DATA(insert OID = 2622 ( pg_rotate_logfile PGNSP PGUID 12 1 0 0 0 f f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_rotate_logfile _null_ _null_ _null_ ));
DESCR("rotate log file");
DATA(insert OID = 3800 ( pg_current_logfilePGNSP PGUID 12 1 0 0 0 f f f f f f v s 0 0 25 "" _null_ _null_ _null_ _null_ _null_ pg_current_logfile _null_ _null_ _null_ ));
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend can terminate background workers and autovacuum launchers
On Fri, 23 Jun 2017 19:43:35 -0400 Stephen Frost <sfr...@snowman.net> wrote: > Alvaro, all, > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > > Yugo Nagata wrote: > > > > > I tried to make it. Patch attached. > > > > > > It is easy and simple. Although I haven't tried for autovacuum worker, > > > I confirmed I can change other process' parameters without affecting > > > others. > > > Do you want this in PG? > > > > One advantage of this gadget is that you can signal backends that you > > own without being superuser, so +1 for the general idea. (Please do > > create a fresh thread so that this can be appropriately linked to in > > commitfest app, though.) > > Well, that wouldn't quite work with the proposed patch because the > proposed patch REVOKE's execute from public... Yes. It is intentional to revoke execute from public because we need to change configuration before signaling other backend and it needs superuser privilege. > I'm trying to figure out how it's actually useful to be able to signal a > backend that you own about a config change that you *aren't* able to > make without being a superuser.. Now, if you could tell the other > backend to use a new value for some USERSET GUC, then *that* would be > useful and interesting. > > I haven't got any particularly great ideas for how to make that happen > though. > > > You need a much bigger patch for the autovacuum worker. The use case I > > had in mind is that you have a maintenance window and can run fast > > vacuuming during it, but if the table is large and vacuum can't finish > > within that window then you want vacuum to slow down, without stopping > > it completely. But implementing this requires juggling the > > stdVacuumCostDelay/Limit values during the reload, which are currently > > read at start of vacuuming only; the working values are overwritten from > > those during a rebalance. > > Being able to change those values during a vacuuming run would certainly > be useful, I've had cases where I would have liked to have been able to > do just exactly that. That's largely independent of this though. > > Thanks! > > Stephen -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend can terminate background workers and autovacuum launchers
On Sat, 24 Jun 2017 08:09:52 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:
> On Sat, Jun 24, 2017 at 5:07 AM, Alvaro Herrera
> <alvhe...@2ndquadrant.com> wrote:
> > Yugo Nagata wrote:
> >
> >> I tried to make it. Patch attached.
> >>
> >> It is easy and simple. Although I haven't tried for autovacuum worker,
> >> I confirmed I can change other process' parameters without affecting
> >> others.
> >> Do you want this in PG?
>
> Just browsing the patch...
>
> +if (r == SIGNAL_BACKEND_NOSUPERUSER)
> +ereport(WARNING,
> +(errmsg("must be a superuser to terminate superuser
> process")));
> +
> +if (r == SIGNAL_BACKEND_NOPERMISSION)
> +ereport(WARNING,
> + (errmsg("must be a member of the role whose process
> is being terminated or member of pg_signal_backend")));
> Both messages are incorrect. This is not trying to terminate a process.
I'll fix this.
>
> +Datum
> +pg_reload_conf_pid(PG_FUNCTION_ARGS)
> I think that the naming is closer to pg_reload_backend.
>
> Documentation is needed as well.
Agree with pg_reload_backend and I'll write the documetation.
>
> > One advantage of this gadget is that you can signal backends that you
> > own without being superuser, so +1 for the general idea. (Please do
> > create a fresh thread so that this can be appropriately linked to in
> > commitfest app, though.)
>
> That would be nice.
Sure. I'll create a new thread and attach the new patch to it.
>
> > You need a much bigger patch for the autovacuum worker. The use case I
> > had in mind is that you have a maintenance window and can run fast
> > vacuuming during it, but if the table is large and vacuum can't finish
> > within that window then you want vacuum to slow down, without stopping
> > it completely. But implementing this requires juggling the
> > stdVacuumCostDelay/Limit values during the reload, which are currently
> > read at start of vacuuming only; the working values are overwritten from
> > those during a rebalance.
>
> Yeah, that's independent from the patch above.
Agree. It would be another feature from pg_reload_backend and
I think it would be implmented as another patch.
> --
> Michael
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Same expression more than once in partition key
On Fri, 23 Jun 2017 09:54:17 -0400 Tom Lane <t...@sss.pgh.pa.us> wrote: > Yugo Nagata <nag...@sraoss.co.jp> writes: > > When we create a range partitioned table, we cannot use > > a column more than once in the partition key. > > > postgres=# create table t (i int) partition by range(i,i); > > ERROR: column "i" appears more than once in partition key > > AFAIK, that's just a simple test for obvious mistakes; it's not a > property that's essential for correctness. Thanks. This is what I want to know. > > > However, I can use same expression more than once in partition key. > > ... and so, I can't get excited about extending it to expressions. > Where would you stop --- for example, are i and (i) equal? What > about (i) and (case when true then i else j end)? In principle, > the system could recognize both those identities, but it seems > silly to expend code on it just for nagware purposes. > Agreed. > regards, tom lane -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend can terminate background workers and autovacuum launchers
On Thu, 22 Jun 2017 14:08:30 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:
> On Thu, Jun 22, 2017 at 1:52 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> > On Thu, 22 Jun 2017 12:05:19 +0900
> > Michael Paquier <michael.paqu...@gmail.com> wrote:
> >> signal-able). Different thought, but I'd love to see a SQL function
> >> that allows triggering SIGHUP on a specific process, like an
> >> autovacuum worker to change its cost parameters. There is
> >> pg_reload_conf() to do so but that's system-wide.
> >
> > For example, is that like this?
> >
> > =# alter system set autovacuum_vacuum_cost_delay to 10;
> > =# select pg_reload_conf();
> > =# alter system reset autovacuum_vacuum_cost_delay;
>
> No need to reset the parameter afterwards as this makes it sensible to
> updates afterwards, but you have the idea. Note that this is rather
> recent, as autovacuum listens to SIGHUP only since a75fb9b3.
I tried to make it. Patch attached.
It is easy and simple. Although I haven't tried for autovacuum worker,
I confirmed I can change other process' parameters without affecting others.
Do you want this in PG?
[session A (PID:18375)]
=# show autovacuum_vacuum_cost_delay;
autovacuum_vacuum_cost_delay
--
20ms
(1 row)
[session B]
postgres=# alter system set autovacuum_vacuum_cost_delay to 10;
ALTER SYSTEM
postgres=# select pg_reload_conf(18375);
pg_reload_conf
t
(1 row)
postgres=# show autovacuum_vacuum_cost_delay;
autovacuum_vacuum_cost_delay
--
20ms
(1 row)
[session A again]
postgres=# show autovacuum_vacuum_cost_delay;
autovacuum_vacuum_cost_delay
--
10ms
(1 row)
> --
> Michael
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 0fdad0c..d79faf7 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1128,6 +1128,7 @@ REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause() FROM public;
REVOKE EXECUTE ON FUNCTION pg_wal_replay_resume() FROM public;
REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public;
REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public;
+REVOKE EXECUTE ON FUNCTION pg_reload_conf(int) FROM public;
REVOKE EXECUTE ON FUNCTION pg_current_logfile() FROM public;
REVOKE EXECUTE ON FUNCTION pg_current_logfile(text) FROM public;
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 62341b8..cc173ba 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -339,6 +339,36 @@ pg_reload_conf(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(true);
}
+/*
+ * Signal to reload the database configuration for a specified backend
+ *
+ * Permission checking for this function is managed through the normal
+ * GRANT system.
+ */
+Datum
+pg_reload_conf_pid(PG_FUNCTION_ARGS)
+{
+ int pid = PG_GETARG_INT32(0);
+ int r = pg_signal_backend(pid, SIGHUP);
+
+ if (r == SIGNAL_BACKEND_NOSUPERUSER)
+ ereport(WARNING,
+(errmsg("must be a superuser to terminate superuser process")));
+
+ if (r == SIGNAL_BACKEND_NOPERMISSION)
+ ereport(WARNING,
+ (errmsg("must be a member of the role whose process is being terminated or member of pg_signal_backend")));
+
+ if (r)
+ {
+ ereport(WARNING,
+(errmsg("failed to send signal to backend: %d", pid)));
+ PG_RETURN_BOOL(false);
+ }
+
+ PG_RETURN_BOOL(true);
+}
+
/*
* Rotate log file
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 1191b4a..7258f15 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3255,6 +3255,8 @@ DESCR("true if wal replay is paused");
DATA(insert OID = 2621 ( pg_reload_conf PGNSP PGUID 12 1 0 0 0 f f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_reload_conf _null_ _null_ _null_ ));
DESCR("reload configuration files");
+DATA(insert OID = 3409 ( pg_reload_conf PGNSP PGUID 12 1 0 0 0 f f f f t f v s 1 0 16 "23" _null_ _null_ _null_ _null_ _null_ pg_reload_conf_pid _null_ _null_ _null_ ));
+DESCR("reload configuration files");
DATA(insert OID = 2622 ( pg_rotate_logfile PGNSP PGUID 12 1 0 0 0 f f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_rotate_logfile _null_ _null_ _null_ ));
DESCR("rotate log file");
DATA(insert OID = 3800 ( pg_current_logfilePGNSP PGUID 12 1 0 0 0 f f f f f f v s 0 0 25 "" _null_ _null_ _null_ _null_ _null_ pg_current_logfile _null_ _null_ _null_ ));
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Same expression more than once in partition key
On Fri, 23 Jun 2017 15:57:54 +0900
Yugo Nagata <nag...@sraoss.co.jp> wrote:
> Hi,
>
> When we create a range partitioned table, we cannot use
> a column more than once in the partition key.
>
> postgres=# create table t (i int) partition by range(i,i);
> ERROR: column "i" appears more than once in partition key
>
> However, I can use same expression more than once in partition key.
>
> postgres=# create table t (i int) partition by range((i),(i));
> CREATE TABLE
>
> Although this can be blocked by the attached parch, for example,
> the following is still possible.
I forgot to attach it.
>
> postgres=# create table t (i int) partition by range((i),i);
> CREATE TABLE
>
> Can these definition be a problem in the internal of partitioning?
> If not, we might not need to check column that apears more than once
> in the partition key.
>
> Regards,
>
>
>
> --
> Yugo Nagata <nag...@sraoss.co.jp>
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7d9c769..dc4540b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13171,6 +13171,19 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
PartitionElem *pelem = castNode(PartitionElem, lfirst(l));
ListCell *lc;
+ if (pelem->expr)
+ {
+ /* Copy, to avoid scribbling on the input */
+ pelem = copyObject(pelem);
+
+ /* Now do parse transformation of the expression */
+ pelem->expr = transformExpr(pstate, pelem->expr,
+ EXPR_KIND_PARTITION_EXPRESSION);
+
+ /* we have to fix its collations too */
+ assign_expr_collations(pstate, pelem->expr);
+ }
+
/* Check for PARTITION BY ... (foo, foo) */
foreach(lc, newspec->partParams)
{
@@ -13183,19 +13196,11 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
errmsg("column \"%s\" appears more than once in partition key",
pelem->name),
parser_errposition(pstate, pelem->location)));
- }
-
- if (pelem->expr)
- {
- /* Copy, to avoid scribbling on the input */
- pelem = copyObject(pelem);
-
- /* Now do parse transformation of the expression */
- pelem->expr = transformExpr(pstate, pelem->expr,
- EXPR_KIND_PARTITION_EXPRESSION);
-
- /* we have to fix its collations too */
- assign_expr_collations(pstate, pelem->expr);
+ else if (pelem->expr && pparam->expr && equal(pelem->expr, pparam->expr))
+ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_COLUMN),
+ errmsg("same expression appears more than once in partition key"),
+ parser_errposition(pstate, pelem->location)));
}
newspec->partParams = lappend(newspec->partParams, pelem);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Same expression more than once in partition key
Hi, When we create a range partitioned table, we cannot use a column more than once in the partition key. postgres=# create table t (i int) partition by range(i,i); ERROR: column "i" appears more than once in partition key However, I can use same expression more than once in partition key. postgres=# create table t (i int) partition by range((i),(i)); CREATE TABLE Although this can be blocked by the attached parch, for example, the following is still possible. postgres=# create table t (i int) partition by range((i),i); CREATE TABLE Can these definition be a problem in the internal of partitioning? If not, we might not need to check column that apears more than once in the partition key. Regards, -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
On Fri, 23 Jun 2017 13:41:15 +0900
Yugo Nagata <nag...@sraoss.co.jp> wrote:
> On Tue, 6 Jun 2017 13:03:58 +0530
> amul sul <sula...@gmail.com> wrote:
>
>
> > Updated patch attached.
>
> I looked into the latest patch (v13) and have some comments
> althogh they might be trivial.
One more comment:
+ if (spec->remainder < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+errmsg("remainder for hash partition must be a
non-negative integer")));
The value of remainder is defined as Iconst in gram.y, so it never be negative.
Hence, I think this check is not necessary or Assert is enough.
>
> First, I couldn't apply this patch to the latest HEAD due to
> a documentation fix and pgintend updates. It needes rebase.
>
> $ git apply /tmp/0002-hash-partitioning_another_design-v13.patch
> error: patch failed: doc/src/sgml/ref/create_table.sgml:87
> error: doc/src/sgml/ref/create_table.sgml: patch does not apply
> error: patch failed: src/backend/catalog/partition.c:76
> error: src/backend/catalog/partition.c: patch does not apply
> error: patch failed: src/backend/commands/tablecmds.c:13371
> error: src/backend/commands/tablecmds.c: patch does not apply
>
>
>
> + Hash Partitioning
> +
> +
> +
> + The table is partitioned by specifying modulus and remainder for
> each
> + partition. Each partition holds rows for which the hash value of
> + partition keys when divided by specified modulus produces specified
> + remainder. For more clarification on modulus and remainder please
> refer
> + .
> +
> +
> +
> +
> +
> Range Partitioning
>
> I think this section should be inserted after List Partitioning section
> because
> the order of the descriptions is Range, List, then Hash in other places of
> the documentation. At least,
>
>
> -partition bounds. Currently supported
> -partitioning methods include range and list, where each partition is
> -assigned a range of keys and a list of keys, respectively.
> +partition bounds. The currently supported
> +partitioning methods are list, range, and hash.
>
>
> Also in this hunk. I think "The currently supported partitioning methods are
> range, list, and hash." is better. We don't need to change the order of
> the original description.
>
>
>
>
> -Declarative partitioning only supports list and range partitioning,
> -whereas table inheritance allows data to be divided in a manner of
> -the user's choosing. (Note, however, that if constraint exclusion is
> -unable to prune partitions effectively, query performance will be
> very
> -poor.)
> +Declarative partitioning only supports hash, list and range
> +partitioning, whereas table inheritance allows data to be divided in
> a
> +manner of the user's choosing. (Note, however, that if constraint
> +exclusion is unable to prune partitions effectively, query
> performance
> +will be very poor.)
>
> Similarly, I think "Declarative partitioning only supports range, list and
> hash
> partitioning," is better.
>
>
> +
> +
> + Create a hash partitioned table:
> +
> +CREATE TABLE orders (
> +order_id bigint not null,
> +cust_id bigint not null,
> +status text
> +) PARTITION BY HASH (order_id);
> +
> +
>
> This paragraph should be inserted between "Create a list partitioned table:"
> paragraph and "Ceate partition of a range partitioned table:" paragraph
> as well as range and list.
>
>
> *strategy = PARTITION_STRATEGY_LIST;
> else if (pg_strcasecmp(partspec->strategy, "range") == 0)
> *strategy = PARTITION_STRATEGY_RANGE;
> + else if (pg_strcasecmp(partspec->strategy, "hash") == 0)
> + *strategy = PARTITION_STRATEGY_HASH;
> else
> ereport(ERROR,
>
> In the most of codes, the order is hash, range, then list, but only
> in transformPartitionSpec(), the order is list, range, then hash,
> as above. Maybe it is better to be uniform.
>
>
> + {
> + if (strategy == PARTITION_STRATEGY_HASH)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_UNDEFINED_OBJECT),
> +
Re: [HACKERS] Autovacuum launcher occurs error when cancelled by SIGINT
On Thu, 22 Jun 2017 13:55:26 -0400 Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > Yugo Nagata wrote: > > Hi, > > > > As I report in another thread[1], I found the autovacuum launcher occurs > > the following error in PG 10 when this received SIGINT. I can repuroduce > > this by pg_cancel_backend or `kill -2 `. > > Thanks for the report, BTW! Thank you for fixing it! > > -- > Álvaro Herrerahttps://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
atttype,
-
"btree",
-
BTREE_AM_OID);
+
am_oid == HASH_AM_OID ? "hash" : "btree",
+
am_oid);
How about writing this part as following to reduce code redundancy?
+ Oid am_oid;
+ char *am_name;
+ if (strategy == PARTITION_STRATEGY_HASH)
+ {
+ am_oid = HASH_AM_OID;
+ am_name = pstrdup("hash");
+ }
+ else
+ {
+ am_oid = BTREE_AM_OID;
+ am_name = pstrdup("btree");
+ }
+
if (!pelem->opclass)
{
- partopclass[attn] = GetDefaultOpClass(atttype,
BTREE_AM_OID);
+ partopclass[attn] = GetDefaultOpClass(atttype, am_oid);
if (!OidIsValid(partopclass[attn]))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("data type %s has no default btree
operator class",
- format_type_be(atttype)),
-errhint("You must specify a
btree operator class or define a default btree operator class for the data
type.")));
+errmsg("data type %s has no
default %s operator class",
+
format_type_be(atttype), am_name),
+errhint("You must specify a %s
operator class or define a default %s operator class for the data type.",
+am_name,
am_name)));
+
}
else
partopclass[attn] = ResolveOpClass(pelem->opclass,
atttype,
-
"btree",
-
BTREE_AM_OID);
+
am_name,
+
am_oid);
There is meaningless indentation change.
@@ -2021,7 +2370,8 @@ get_partition_for_tuple(PartitionDispatch *pd,
/* bsearch in partdesc->boundinfo */
cur_offset = partition_bound_bsearch(key,
partdesc->boundinfo,
-values, false, );
+ values, false, );
+
/*
* Offset returned is such that the bound at offset is
Fixing the comment of pg_get_partkeydef() is missing.
* pg_get_partkeydef
*
* Returns the partition key specification, ie, the following:
*
* PARTITION BY { RANGE | LIST } (column opt_collation opt_opclass [, ...])
*/
Datum
pg_get_partkeydef(PG_FUNCTION_ARGS)
{
Regards,
>
> Regards,
> Amul Sul
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Incorrect documentation about pg_stat_activity
On Thu, 22 Jun 2017 14:14:53 +0530 Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: > On Wed, Jun 21, 2017 at 7:48 PM, Simon Riggs <si...@2ndquadrant.com> wrote: > > On 21 June 2017 at 16:15, Yugo Nagata <nag...@sraoss.co.jp> wrote: > >> On Wed, 21 Jun 2017 19:08:35 +0530 > >> Kuntal Ghosh <kuntalghosh.2...@gmail.com> wrote: > >> > >>> On Wed, Jun 21, 2017 at 6:05 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > >>> > > >>> > Attached is a patch for the documentation fix. > >>> > > >>> Please attach the patch as well. :-) > >> > >> I'm sorry, I forgot it. I attahed this. > > > > Thanks, will apply. > > The patch is adjusting documentation for the observed behaviour. But I > don't see a justification as to why the observed behaviour is correct > one? Was there a commit which allowed startup process to be part of > pg_stat_activity but forgot to update the documentation or the current > behaviour is unintentional and probably needs to be fixed? I confirmed the pg_stat_activity documentation and this says "startup" can be backend_type column's value, so I believe the behaviour is intentional. https://www.postgresql.org/docs/10/static/monitoring-stats.html#pg-stat-activity-view > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optional message to user when terminating/cancelling backend
On Thu, 22 Jun 2017 09:24:54 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:
> On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson <dan...@yesql.se> wrote:
> > The message is truncated in SetBackendCancelMessage() for safety, but
> > pg_{cancel|terminate}_backend() could throw an error on too long message, or
> > warning truncation, to the caller as well. Personally I think a warning is
> > the
> > appropriate response, but I don’t really have a strong opinion.
>
> And a NOTICE? That's what happens for relation name truncation. You
> are right that having a check in SetBackendCancelMessage() makes the
> most sense as bgworkers could just call the low level API. Isn't the
> concept actually closer to just a backend message? This slot could be
> used for other purposes than cancellation.
+1 for NOTICE. The message truncation seems to be a kind of helpful
information rather than a likely problem as long as pg_terminated_backend
exits successfully.
https://www.postgresql.org/docs/10/static/runtime-config-logging.html#runtime-config-severity-levels
> --
> Michael
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Logical replication launcher never been restarted when terminated
On Wed, 21 Jun 2017 15:17:20 -0400 Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > On 6/21/17 13:03, Yugo Nagata wrote: > > As I report in another thread[1], when the logical replication launcher > > is terminated by SIGTERM, it never been restarted and we need to restart > > the server to enable logical replication again. > > > > This is because the logical replication launcher exits with exitstatus 0, > > so if it exits with status 1 it is restarted by the postmaster normally. > > Attached is a simple patch to fix it in this way. > > Fixed, thanks! Thanks! > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Autovacuum launcher occurs error when cancelled by SIGINT
On Thu, 22 Jun 2017 13:12:48 +0900 Michael Paquier <michael.paqu...@gmail.com> wrote: > On Wed, Jun 21, 2017 at 9:15 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > This errors continue until this process is terminated or the server is > > restarted. > > > > When SIGINT is issued, the process exits from the main loop and returns > > to sigsetjmp, and calls dsa_attach() before entering into the loop again, > > this causes the error. > > > > We can fix it by calling dsa_attach() before sigsetjmp. Attached is the > > patch. > > Your fix looks like a bad idea to me. If the shared memory area does > not exist after an exception occurred the process should be able to > re-attach to the shared memory area if it exists or create a new one > if that's not the case. That should not be a one-time execution. Thank you for your comment. I overlooked it and now I understand it. > -- > Michael -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend can terminate background workers and autovacuum launchers
On Thu, 22 Jun 2017 12:05:19 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:
> On Thu, Jun 22, 2017 at 11:52 AM, Andres Freund <and...@anarazel.de> wrote:
> > On 2017-06-22 11:49:47 +0900, Yugo Nagata wrote:
> >> I agree that we can kill theses processes by the OS command. However,
> >> It seems to me that pg_{cancel,terminate}_backend don't need to be able to
> >> kill processes except for client backends because we can do same thing by
> >> the OS command if necessary, and acutually these functions cannot kill
> >> most other processes, for example, background writer. Are the autovacuum
> >> launcher and background worker special for these functions?
> >
> > I strongly disagree with this - I think it's quite useful to be able to
> > kill things via SQL that can hold lock on database objects. I'm not
> > seeing which problem would be solved by prohibiting this?
>
> +1. Controlling them as of now is useful, particularly now that all
> processes show wait events, even auxiliary ones (though not
I got it. I agree that the current behaviour and it isn't worth changint it.
In my understand, processes that the functions can kill (client backends,
background workers, autovacuum processes) are ones that can hold lock
on database objects.
> signal-able). Different thought, but I'd love to see a SQL function
> that allows triggering SIGHUP on a specific process, like an
> autovacuum worker to change its cost parameters. There is
> pg_reload_conf() to do so but that's system-wide.
For example, is that like this?
=# alter system set autovacuum_vacuum_cost_delay to 10;
=# select pg_reload_conf();
=# alter system reset autovacuum_vacuum_cost_delay;
> --
> Michael
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend can terminate background workers and autovacuum launchers
On Wed, 21 Jun 2017 11:04:34 -0400
Robert Haas <robertmh...@gmail.com> wrote:
> On Wed, Jun 21, 2017 at 7:56 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> > I have found that we can cancel/terminate autovacuum launchers and
> > background worker processes by pg_cancel/terminate_backend function.
> > I'm wondering this behavior is not expected and if not I want to fix it.
>
> I think it is expected. Even if we blocked it, those processes have
> to cope gracefully with SIGTERM, because anyone with access to the OS
> user can kill them that way by hand.
I agree that we can kill theses processes by the OS command. However,
It seems to me that pg_{cancel,terminate}_backend don't need to be able to
kill processes except for client backends because we can do same thing by
the OS command if necessary, and acutually these functions cannot kill
most other processes, for example, background writer. Are the autovacuum
launcher and background worker special for these functions?
>
> > However, we can terminate background workers by pg_terminate_backend.
> > In the following example, I terminated the logical replication launcher,
> > and this process did not appear again[1].
> >
> > postgres=# select pg_terminate_backend(30902);
> > pg_terminate_backend
> > --
> > t
> > (1 row)
>
> That seems to be a bug in logical replication.
>
> > Similarly, we can terminate autovacuum launcher by pg_terminate_backend,
> > but a new process is restarted by postmaster in this case.[2]
> >
> > postgres=# select pg_terminate_backend(30900);
> > pg_terminate_backend
> > --
> > t
> > (1 row)
>
> That is as I would expect.
>
> > [2]
> > On the other hand, when we use pg_cancel_backend for autovacuum launcher,
> > it causes the following error. I'll report the detail in another thread.
> >
> > ERROR: can't attach the same segment more than once
>
> I think that's a bug.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Logical replication launcher never been restarted when terminated
Hi,
As I report in another thread[1], when the logical replication launcher
is terminated by SIGTERM, it never been restarted and we need to restart
the server to enable logical replication again.
This is because the logical replication launcher exits with exitstatus 0,
so if it exits with status 1 it is restarted by the postmaster normally.
Attached is a simple patch to fix it in this way.
However, I'm not sure this is the best way. For example, in this way,
we get the following log when the process is terminated, which we don't
get when it exits with status 0.
LOG: worker process: logical replication launcher (PID 11526) exited with
exit code 1
If we don't want to get this message, we need more fixes in
CleanupBackgroundWorker()
or around it.
[1]
https://www.postgresql.org/message-id/20170621205657.61d90605.nagata%40sraoss.co.jp
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index f99dd0a..a833dc5 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2855,7 +2855,7 @@ ProcessInterrupts(void)
(errmsg("logical replication launcher shutting down")));
/* The logical replication launcher can be stopped at any time. */
- proc_exit(0);
+ proc_exit(1);
}
else if (RecoveryConflictPending && RecoveryConflictRetryable)
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Incorrect documentation about pg_stat_activity
On Wed, 21 Jun 2017 16:18:50 +0200 Simon Riggs <si...@2ndquadrant.com> wrote: > On 21 June 2017 at 16:15, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > On Wed, 21 Jun 2017 19:08:35 +0530 > > Kuntal Ghosh <kuntalghosh.2...@gmail.com> wrote: > > > >> On Wed, Jun 21, 2017 at 6:05 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > >> > > >> > Attached is a patch for the documentation fix. > >> > > >> Please attach the patch as well. :-) > > > > I'm sorry, I forgot it. I attahed this. > > Thanks, will apply. Thanks, too. > > -- > Simon Riggs http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optional message to user when terminating/cancelling backend
On Wed, 21 Jun 2017 12:06:33 +0900 Michael Paquier <michael.paqu...@gmail.com> wrote: > On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <dan...@yesql.se> wrote: > > The message is stored in a new shmem area which is checked when the session > > is > > aborted. To keep things simple a small buffer is kept per backend for the > > message. If deemed too costly, keeping a central buffer from which slabs > > are > > allocated can be done (but seemed rather complicated for little gain > > compared > > to the quite moderate memory spend.) > > I think that you are right to take the approach with a per-backend > slot. This will avoid complications related to entry removals and > locking issues. There would be scaling issues as well if things get > very signaled for a lot of backends. > > +#define MAX_CANCEL_MSG 128 > That looks enough. > > + LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE); > + > + strlcpy(slot->message, message, sizeof(slot->message)); > + slot->len = strlen(message); > Why not using one spin lock per slot and save it in BackendCancelShmemStruct? +1 I found an example that a spin lock is used during strlcpy in WalReceiverMain(). > > + pid_t pid = PG_GETARG_INT32(0); > + char *msg = text_to_cstring(PG_GETARG_TEXT_PP(1)); > + > + PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg)); > It would be more solid to add some error handling for messages that > are too long, or at least truncate the message if too long. I agree that error handling for too long messages is needed. Although long messages are truncated in SetBackendCancelMessage(), it is better to inform users that the message they can read was truncated one. Or, maybe we should prohibit too long message is passed in pg_teminate_backend() > -- > Michael > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Incorrect documentation about pg_stat_activity
On Wed, 21 Jun 2017 19:08:35 +0530 Kuntal Ghosh <kuntalghosh.2...@gmail.com> wrote: > On Wed, Jun 21, 2017 at 6:05 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > > > Attached is a patch for the documentation fix. > > > Please attach the patch as well. :-) I'm sorry, I forgot it. I attahed this. > > > -- > Thanks & Regards, > Kuntal Ghosh > EnterpriseDB: http://www.enterprisedb.com > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml index 72eb073..1188f07 100644 --- a/doc/src/sgml/high-availability.sgml +++ b/doc/src/sgml/high-availability.sgml @@ -2203,12 +2203,12 @@ LOG: database system is ready to accept read only connections pg_cancel_backend() and pg_terminate_backend() will work on user backends, but not the Startup process, which performs -recovery. pg_stat_activity does not show an -entry for the Startup process, nor do recovering transactions show -as active. As a result, pg_prepared_xacts -is always empty during recovery. If you wish to resolve in-doubt -prepared transactions, view pg_prepared_xacts on the -primary and issue commands to resolve transactions there. +recovery. pg_stat_activity does not show +recovering transactions as active. As a result, +pg_prepared_xacts is always empty during +recovery. If you wish to resolve in-doubt prepared transactions, view +pg_prepared_xacts on the primary and issue commands to +resolve transactions there. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optional message to user when terminating/cancelling backend
Hi,
Here are some comments for the patch.
+Datum
+pg_cancel_backend(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL));
+}
+Datum
+pg_cancel_backend_msg(PG_FUNCTION_ARGS)
+{
+ pid_t pid = PG_GETARG_INT32(0);
+ char *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+ PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
+}
It would be better to insert a blank line between these functions.
+/*
+ * Sets a cancellation message for the backend with the specified pid and
+ * returns the length of message actually created. If the returned length
+ * is equal to the length of the message parameter, truncation may have
+ * occurred. If the backend wasn't found and no message was set, -1 is
+ * returned.
+ */
It seems to me that this comment is incorrect.
"If the returned length is not equal to the length of the message parameter,"
is right, isn't it?
In addition, the last statement would be
"If the backend wasn't found, -1 is returned. Otherwize, if no message was set,
0 is returned."
+ strlcpy(slot->message, message, sizeof(slot->message));
+ slot->len = strlen(message);
+
+ LWLockRelease(BackendCancelLock);
+ return slot->len;
If SetBackendCancelMessage() has to return the length of message actually
created,
slot->len should be strlen(slot->message) instead of strlen(message).
In the current code, when the return value and slot->len is always set
to the length of the passed message parameter.
Regards,
On Mon, 19 Jun 2017 20:24:43 +0200
Daniel Gustafsson <dan...@yesql.se> wrote:
> When terminating, or cancelling, a backend it’s currently not possible to let
> the signalled session know *why* it was dropped. This has nagged me in the
> past and now it happened to come up again, so I took a stab at this. The
> attached patch implements the ability to pass an optional text message to the
> signalled session which is included in the error message:
>
> SELECT pg_terminate_backend( [, message]);
> SELECT pg_cancel_backend( [, message]);
>
> Right now the message is simply appended on the error message, not sure if
> errdetail or errhint would be better? Calling:
>
> select pg_terminate_backend(, 'server rebooting');
>
> ..leads to:
>
> FATAL: terminating connection due to administrator command: "server
> rebooting"
>
> Omitting the message invokes the command just like today.
>
> The message is stored in a new shmem area which is checked when the session is
> aborted. To keep things simple a small buffer is kept per backend for the
> message. If deemed too costly, keeping a central buffer from which slabs are
> allocated can be done (but seemed rather complicated for little gain compared
> to the quite moderate memory spend.)
>
> cheers ./daniel
>
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Incorrect documentation about pg_stat_activity
Hi, In the documentation[1], there is the following description: "pg_stat_activity does not show an entry for the Startup process" However, the current pg_stat_activity show startup process's entry. postgres=# select pid, backend_type from pg_stat_activity ; pid | backend_type ---+--- 27314 | client backend 27103 | startup 27113 | background writer 27112 | checkpointer 27115 | walreceiver (5 rows) Attached is a patch for the documentation fix. Regards, [1] 26.5.3. Administrator's Overview https://www.postgresql.org/docs/10/static/hot-standby.html#hot-standby-admin -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Autovacuum launcher occurs error when cancelled by SIGINT
Hi,
As I report in another thread[1], I found the autovacuum launcher occurs
the following error in PG 10 when this received SIGINT. I can repuroduce
this by pg_cancel_backend or `kill -2 `.
2017-06-21 13:56:07.010 JST [32483] ERROR: canceling statement due to user
request
2017-06-21 13:56:08.022 JST [32483] ERROR: can't attach the same segment more
than once
2017-06-21 13:56:09.034 JST [32483] ERROR: can't attach the same segment more
than once
2017-06-21 13:56:10.045 JST [32483] ERROR: can't attach the same segment more
than once
...
This errors continue until this process is terminated or the server is
restarted.
When SIGINT is issued, the process exits from the main loop and returns
to sigsetjmp, and calls dsa_attach() before entering into the loop again,
this causes the error.
We can fix it by calling dsa_attach() before sigsetjmp. Attached is the patch.
Regards,
[1]
https://www.postgresql.org/message-id/20170621205657.61d90605.nagata%40sraoss.co.jp
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 89dd3b3..8a41a98 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -502,6 +502,28 @@ AutoVacLauncherMain(int argc, char *argv[])
MemoryContextSwitchTo(AutovacMemCxt);
/*
+ * Set up our DSA so that backends can install work-item requests. It may
+ * already exist as created by a previous launcher.
+ */
+ if (!AutoVacuumShmem->av_dsa_handle)
+ {
+ LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
+ AutoVacuumDSA = dsa_create(AutovacuumLock->tranche);
+ /* make sure it doesn't go away even if we do */
+ dsa_pin(AutoVacuumDSA);
+ dsa_pin_mapping(AutoVacuumDSA);
+ AutoVacuumShmem->av_dsa_handle = dsa_get_handle(AutoVacuumDSA);
+ /* delay array allocation until first request */
+ AutoVacuumShmem->av_workitems = InvalidDsaPointer;
+ LWLockRelease(AutovacuumLock);
+ }
+ else
+ {
+ AutoVacuumDSA = dsa_attach(AutoVacuumShmem->av_dsa_handle);
+ dsa_pin_mapping(AutoVacuumDSA);
+ }
+
+ /*
* If an exception is encountered, processing resumes here.
*
* This code is a stripped down version of PostgresMain error recovery.
@@ -610,28 +632,6 @@ AutoVacLauncherMain(int argc, char *argv[])
*/
rebuild_database_list(InvalidOid);
- /*
- * Set up our DSA so that backends can install work-item requests. It may
- * already exist as created by a previous launcher.
- */
- if (!AutoVacuumShmem->av_dsa_handle)
- {
- LWLockAcquire(AutovacuumLock, LW_EXCLUSIVE);
- AutoVacuumDSA = dsa_create(AutovacuumLock->tranche);
- /* make sure it doesn't go away even if we do */
- dsa_pin(AutoVacuumDSA);
- dsa_pin_mapping(AutoVacuumDSA);
- AutoVacuumShmem->av_dsa_handle = dsa_get_handle(AutoVacuumDSA);
- /* delay array allocation until first request */
- AutoVacuumShmem->av_workitems = InvalidDsaPointer;
- LWLockRelease(AutovacuumLock);
- }
- else
- {
- AutoVacuumDSA = dsa_attach(AutoVacuumShmem->av_dsa_handle);
- dsa_pin_mapping(AutoVacuumDSA);
- }
-
/* loop until shutdown request */
while (!got_SIGTERM)
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_terminate_backend can terminate background workers and autovacuum launchers
Hi,
I have found that we can cancel/terminate autovacuum launchers and
background worker processes by pg_cancel/terminate_backend function.
I'm wondering this behavior is not expected and if not I want to fix it.
The current pg_stat_activity shows background workers and autovacuum
lancher as below. It made me come up with the question.
postgres=# select pid, wait_event, backend_type from pg_stat_activity ;
pid | wait_event |backend_type
---+-+-
30902 | LogicalLauncherMain | background worker
30900 | AutoVacuumMain | autovacuum launcher
30923 | | client backend
30898 | BgWriterMain| background writer
30897 | CheckpointerMain| checkpointer
30899 | WalWriterMain | walwriter
(6 rows)
We cannot use pg_terminate/cancel_backend for most processes
except client backends. For example, when I tried to terminate
the background writer, I got a warning and failed.
postgres=# select pg_terminate_backend(30899);
WARNING: PID 30899 is not a PostgreSQL server process
pg_terminate_backend
--
f
(1 row)
However, we can terminate background workers by pg_terminate_backend.
In the following example, I terminated the logical replication launcher,
and this process did not appear again[1].
postgres=# select pg_terminate_backend(30902);
pg_terminate_backend
--
t
(1 row)
postgres=# select pid, wait_event, backend_type from pg_stat_activity ;
pid |wait_event |backend_type
---+---+-
30900 | AutoVacuumMain| autovacuum launcher
30923 | | client backend
30898 | BgWriterHibernate | background writer
30897 | CheckpointerMain | checkpointer
30899 | WalWriterMain | walwriter
(5 rows)
Similarly, we can terminate autovacuum launcher by pg_terminate_backend,
but a new process is restarted by postmaster in this case.[2]
postgres=# select pg_terminate_backend(30900);
pg_terminate_backend
--
t
(1 row)
postgres=# select pid, wait_event, backend_type from pg_stat_activity ;
pid |wait_event |backend_type
---+---+-
32483 | AutoVacuumMain| autovacuum launcher
30923 | | client backend
30898 | BgWriterHibernate | background writer
30897 | CheckpointerMain | checkpointer
30899 | WalWriterMain | walwriter
(5 rows)
My question is whether the behavior of pg_terminate/cancel_backend is
expected. If these functions should succeed only for client backends,
we need to fix the behavior. Attached is a patch to fix it in that case.
In my patch, process type is checked in pg_signal_backend(), and if it is
background worker or autovacuum launcher then throw a warning and fail.
BackendPidGetProc() returns valid PGPROC for proccesses that are initialized
by PostgresInit(), and, in my understand, all such proccess are client
backends, background workers, and autovacuum launcher. So, if this is
neither background woker nor autovacuum launcher, this should be
a normal client backend. For this check, I added a new field,
isAutoVacuumLauncher, to PGPROC.
Any comments would be appreciated.
-
[1]
AFAIK, we have to restart the server to enable logical replication after this.
I'm not sure this is expected, but I found the following comment in
ProcessInterrupts(). Does "can be stopped at any time" mean that we can
drop this process completely?
2852 else if (IsLogicalLauncher())
2853 {
2854 ereport(DEBUG1,
2855 (errmsg("logical replication launcher shutting
down")));
2856
2857 /* The logical replication launcher can be stopped at any
time. */
2858 proc_exit(0);
2859 }
When the logical replication launcher receive SIGTERM, this exits with
exitstatus 0,
so this is not restarted by the postmaster.
[2]
On the other hand, when we use pg_cancel_backend for autovacuum launcher,
it causes the following error. I'll report the detail in another thread.
ERROR: can't attach the same segment more than once
-
Regards,
--
Yugo Nagata <nag...@sraoss.co.jp>
pg_terminate_backend.pach
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] improve release-note for pg_current_logfile()
Hi, When I am reading the PG 10 release-notes, I found the following item. > Add function pg_current_logfile() to read syslog's current stderr and csvlog > output file names (Gilles Darold) This confused me because "syslog" is one of method for logging as well as stderr and csvlog. I guess it is intended syslogger, but I think that "logging collector" is more convenient for users because this is used in the pg_current_logfile() documentation. pg_current_logfile returns, as text, the path of the log file(s) currently in use by the logging collector. Attached is a patch to fix it. Regards, -- Yugo Nagata <nag...@sraoss.co.jp> release_note_pg_current_logfile.pach Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] type of release note of PG10
Hi, I found a typo in the PG10 release note and attached is a patch to fix it. Regards, -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/doc/src/sgml/release-10.sgml b/doc/src/sgml/release-10.sgml index b10086bd..f3e4a70 100644 --- a/doc/src/sgml/release-10.sgml +++ b/doc/src/sgml/release-10.sgml @@ -525,7 +525,7 @@ -Specifically, a new CREATE +Specifically, a new CREATE INDEX option allows auto-summarizion of the previous BRIN page range when a new page range is created. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] documentation typo in ALTER TABLE example
Hi, Attached is a simple patch to fix a documentation typo in the ALTER TABLE example. Regards, -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 56ea830..4c61c44 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -1398,7 +1398,7 @@ ALTER TABLE cities Detach a partition from partitioned table: -ALTER TABLE cities +ALTER TABLE measurement DETACH PARTITION measurement_y2015m12; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] CREATE TRIGGER document typo
Hi, Attached is a patch fixing simple typos in the CREATE TRIGGER document. -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml index 24195b3..c5f7c75 100644 --- a/doc/src/sgml/ref/create_trigger.sgml +++ b/doc/src/sgml/ref/create_trigger.sgml @@ -531,8 +531,8 @@ CREATE TRIGGER view_insert CREATE TRIGGER transfer_insert AFTER INSERT ON transfer -FOR EACH STATEMENT REFERENCING NEW TABLE AS inserted +FOR EACH STATEMENT EXECUTE PROCEDURE check_transfer_balances_to_zero(); @@ -543,8 +543,8 @@ CREATE TRIGGER transfer_insert CREATE TRIGGER paired_items_update AFTER UPDATE ON paired_items -FOR EACH ROW REFERENCING NEW TABLE AS newtab OLD TABLE AS oldtab +FOR EACH ROW EXECUTE PROCEDURE check_matching_pairs(); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
On Fri, 14 Apr 2017 09:05:14 -0400 Robert Haas <robertmh...@gmail.com> wrote: > On Fri, Apr 14, 2017 at 4:23 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > On Thu, 13 Apr 2017 16:40:29 -0400 > > Robert Haas <robertmh...@gmail.com> wrote: > >> On Fri, Mar 17, 2017 at 7:57 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > >> > I also understanded that my design has a problem during pg_dump and > >> > pg_upgrade, and that some information to identify the partition > >> > is required not depending the command order. However, I feel that > >> > Amul's design is a bit complicated with the rule to specify modulus. > >> > > >> > I think we can use simpler syntax, for example, as below. > >> > > >> > CREATE TABLE h1 PARTITION OF h FOR (0); > >> > CREATE TABLE h2 PARTITION OF h FOR (1); > >> > CREATE TABLE h3 PARTITION OF h FOR (2); > >> > >> I don't see how that can possibly work. Until you see all the table > >> partitions, you don't know what the partitioning constraint for any > >> given partition should be, which seems to me to be a fatal problem. > > > > If a partition has an id, the partitioning constraint can be written as > > > > hash_func(hash_key) % N = id > > > > wehre N is the number of paritions. Doesn't it work? > > Only if you know the number of partitions. But with your syntax, > after seeing only the first of the CREATE TABLE .. PARTITION OF > commands, what should the partition constraint be? It depends on how > many more such commands appear later in the dump file, which you do > not know at that point. I thought that the partition constraint could be decided every time a new partition is created or attached, and that it woule be needed to relocate records automatically when the partition configuration changes. However, I have come to think that the automatic relocation might not be needed at this point. > > >> I agree that Amul's syntax - really, I proposed it to him - is not the > >> simplest, but I think all the details needed to reconstruct the > >> partitioning constraint need to be explicit. Otherwise, I'm pretty > >> sure things we're going to have lots of problems that we can't really > >> solve cleanly. We can later invent convenience syntax that makes > >> common configurations easier to set up, but we should invent the > >> syntax that spells out all the details first. > > > > I have a question about Amul's syntax. After we create partitions > > as followings, > > > > create table foo (a integer, b text) partition by hash (a); > > create table foo1 partition of foo with (modulus 2, remainder 0); > > create table foo2 partition of foo with (modulus 2, remainder 1); > > > > we cannot create any additional partitions for the partition. > > > > Then, after inserting records into foo1 and foo2, how we can > > increase the number of partitions? > > You can detach foo1, create two new partitions with modulus 4 and > remainders 0 and 2, and move the data over from the old partition. > > I realize that's not as automated as you might like, but it's no worse > than what is currently required for list and range partitioning when > you split a partition. Someday we might build in tools to do that > kind of data migration automatically, but right now we have none. Thanks. I understood it. The automatic data migration feature would be better to be implemented separately. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
On Thu, 13 Apr 2017 16:40:29 -0400 Robert Haas <robertmh...@gmail.com> wrote: > On Fri, Mar 17, 2017 at 7:57 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > I also understanded that my design has a problem during pg_dump and > > pg_upgrade, and that some information to identify the partition > > is required not depending the command order. However, I feel that > > Amul's design is a bit complicated with the rule to specify modulus. > > > > I think we can use simpler syntax, for example, as below. > > > > CREATE TABLE h1 PARTITION OF h FOR (0); > > CREATE TABLE h2 PARTITION OF h FOR (1); > > CREATE TABLE h3 PARTITION OF h FOR (2); > > I don't see how that can possibly work. Until you see all the table > partitions, you don't know what the partitioning constraint for any > given partition should be, which seems to me to be a fatal problem. If a partition has an id, the partitioning constraint can be written as hash_func(hash_key) % N = id wehre N is the number of paritions. Doesn't it work? > I agree that Amul's syntax - really, I proposed it to him - is not the > simplest, but I think all the details needed to reconstruct the > partitioning constraint need to be explicit. Otherwise, I'm pretty > sure things we're going to have lots of problems that we can't really > solve cleanly. We can later invent convenience syntax that makes > common configurations easier to set up, but we should invent the > syntax that spells out all the details first. I have a question about Amul's syntax. After we create partitions as followings, create table foo (a integer, b text) partition by hash (a); create table foo1 partition of foo with (modulus 2, remainder 0); create table foo2 partition of foo with (modulus 2, remainder 1); we cannot create any additional partitions for the partition. Then, after inserting records into foo1 and foo2, how we can increase the number of partitions? > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
On Tue, 14 Mar 2017 10:08:14 -0400 David Steele <da...@pgmasters.net> wrote: > Please post an explanation for the delay and a schedule for the new > patch. If no patch or explanation is posted by 2017-03-17 AoE I will > mark this submission "Returned with Feedback". I am sorry for my late response. I had not a enough time because I had a business trip and was busy for other works. I agree that fixing the number of partitions is bad and a way to increase or decrease partitions should be provided. I also think using linear hashing would be good as Amul is mentioning, but I have not implemented it in my patch yet. I also understanded that my design has a problem during pg_dump and pg_upgrade, and that some information to identify the partition is required not depending the command order. However, I feel that Amul's design is a bit complicated with the rule to specify modulus. I think we can use simpler syntax, for example, as below. CREATE TABLE h1 PARTITION OF h FOR (0); CREATE TABLE h2 PARTITION OF h FOR (1); CREATE TABLE h3 PARTITION OF h FOR (2); If user want to user any complicated partitioning rule, it can be defined by specifying a user-defined hash function at creating partitioned table. If the hash function is omitted, we will be able to use default hash operator class as well as in Amul's patch. Attached is the updated patch taking the comments from Aleksander and Rushabh. HASH keyword and unnecessary spaces are removed, and some comments are added. Thanks, -- Yugo Nagata <nag...@sraoss.co.jp> hash_partition.patch.v2 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Report the number of skipped frozen pages by manual VACUUM
On Fri, 10 Mar 2017 20:08:42 +0900
Masahiko Sawada <sawada.m...@gmail.com> wrote:
> On Fri, Mar 10, 2017 at 3:58 PM, Jim Nasby <jim.na...@openscg.com> wrote:
> > On 3/6/17 8:34 PM, Masahiko Sawada wrote:
> >>
> >> I don't think it can say "1 frozen pages" because the number of
> >> skipped pages according to visibility map is always more than 32
> >> (SKIP_PAGES_THRESHOLD).
> >
> >
> > That's just an artifact of how the VM currently works. I'm not a fan of
> > cross dependencies like that unless there's a pretty good reason.
>
> Thank you for the comment.
> Agreed. Attached fixed version patch.
It seems that it says "frozen pages" if pinskipped_pages is not zero,
rather than about frozenskipped_pages.
How about writing as below?
appendStringInfo(, ngettext("Skipped %u page due to buffer pins, "
"Skipped %u pages due to buffer pins, ",
vacrelstats->pinskipped_pages),
vacrelstats->pinskipped_pages);
appendStringInfo(, ngettext("%u frozen page.\n", "%u frozen pages.\n",
vacrelstats->frozenskipped_pages),
vacrelstats->frozenskipped_pages);
>
> > BTW, I think there's already a function that handles the pluralization for
> > you. IIRC it's one of the things you can add to an ereport() call.
>
> What is the function name?
>
> Regards,
>
> --
> Masahiko Sawada
> NIPPON TELEGRAPH AND TELEPHONE CORPORATION
> NTT Open Source Software Center
--
Yugo Nagata <nag...@sraoss.co.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Report the number of skipped frozen pages by manual VACUUM
Hi, I think this is good since the information is useful and it is a little change. One thing I'm bothered is that even when the number of frozen page is one, it will say "1 frozen pages". In other messages, when the number of page is one, the word "page" rather than "pages" is used by using ngettext(). In addition, the document (doc/src/sgml/ref/vacuum.sgml) need a modification to use the new messages in its example. BTW, this patch can't be applied after the following commit. commit 9eb344faf54a849898d9be012ddfa8204cfeb57c Author: Simon Riggs <si...@2ndquadrant.com> Date: Fri Mar 3 19:18:25 2017 +0530 Allow vacuums to report oldestxmin Regards, Yugo Nagata On Tue, 28 Feb 2017 16:40:28 +0900 Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Fri, Feb 24, 2017 at 1:30 AM, Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > Hi all, > > > > The autovacuum reports the number of skipped frozen pages to the > > VACUUM output. But these information is not appeared by manual VACUUM. > > This information is useful for the user to check efficiency of VACUUM. > > > > Attached patch add this information to VACUUM output. > > > > * Example > > =# VACUUM VERBOSE test > > INFO: vacuuming "public.test" > > INFO: "test": found 0 removable, 56 nonremovable row versions in 1 > > out of 45 pages > > DETAIL: 0 dead row versions cannot be removed yet. > > There were 0 unused item pointers. > > Skipped 0 pages due to buffer pins, 44 frozen pages. > > 0 pages are entirely empty. > > CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s. > > VACUUM > > > > I'll register it to next CF. > > > > Added this patch to CF 2017-03. > > Regards, > > -- > Masahiko Sawada > NIPPON TELEGRAPH AND TELEPHONE CORPORATION > NTT Open Source Software Center > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
On Thu, 2 Mar 2017 18:33:42 +0530 amul sul <sula...@gmail.com> wrote: Thank you for the patch. This is very interesting. I'm going to look into your code and write a feedback later. > On Wed, Mar 1, 2017 at 3:50 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > > [] > > > > I Agree that it is unavoidable partitions number in modulo hashing, > > > but we can do in other hashing technique. Have you had thought about > > > Linear hashing[1] or Consistent hashing[2]? This will allow us to > > > add/drop > > > partition with minimal row moment. > > > > Thank you for your information of hash technique. I'll see them > > and try to allowing the number of partitions to be changed. > > > > > Thanks for showing interest, I was also talking about this with Robert Haas > and > hacking on this, here is what we came up with this. > > If we want to introduce hash partitioning without syntax contort and minimal > movement while changing hash partitions (ADD-DROP/ATTACH-DETACH operation), > at start I thought we could pick up linear hashing, because of in both the > hashing we might need to move approx tot_num_of_tuple/tot_num_of_partitions > tuples at adding new partition and no row moment required at dropping > partitioning. > > With further thinking and talking through the idea of using linear hashing > with my team, we realized that has some problems specially during pg_dump > and pg_upgrade. Both a regular pg_dump and the binary-upgrade version of > pg_dump which is used by pg_restore need to maintain the identity of the > partitions. We can't rely on things like OID order which may be unstable, or > name order which might not match the order in which partitions were added. > So > somehow the partition position would need to be specified explicitly. > > So later we came up with some syntax like this (just fyi, this doesn't add > any new keywords): > > create table foo (a integer, b text) partition by hash (a); > create table foo1 partition of foo with (modulus 4, remainder 0); > create table foo2 partition of foo with (modulus 8, remainder 1); -- > legal, modulus doesn't need to match > create table foo3 partition of foo with (modulus 8, remainder 4); -- > illegal, overlaps foo1 > > Here we need to enforce a rule that every modulus must be a factor of the > next > larger modulus. So, for example, if you have a bunch of partitions that all > have > modulus 5, you can add a new partition with modulus 10 or a new partition > with > modulus 15, but you cannot add both a partition with modulus 10 and a > partition > with modulus 15, because 10 is not a factor of 15. However, you could > simultaneously use modulus 4, modulus 8, modulus 16, and modulus 32 if you > wished, because each modulus is a factor of the next larger one. You could > also use modulus 10, modulus 20, and modulus 60. But you could not use > modulus > 10, modulus 15, and modulus 60, because while both of the smaller module are > factors of 60, it is not true that each is a factor of the next. > > Other advantages with this rule are: > > 1. Dropping (or detaching) and adding (or attaching) a partition can never > cause the rule to be violated. > > 2. We can easily build a tuple-routing data structure based on the largest > modulus. > > For example: If the user has > partition 1 with (modulus 2, remainder 1), > partition 2 with (modulus 4, remainder 2), > partition 3 with (modulus 8, remainder 0) and > partition 4 with (modulus 8, remainder 4), > > then we can build the following tuple routing array in the relcache: > > == lookup table for hashvalue % 8 == > 0 => p3 > 1 => p1 > 2 => p2 > 3 => p1 > 4 => p4 > 5 => p1 > 6 => p2 > 7 => p1 > > 3. It's also quite easy to test with a proposed new partition overlaps with > any > existing partition. Just build the mapping array and see if you ever end up > trying to assign a partition to a slot that's already been assigned to some > other partition. > > We can still work on the proposed syntax - and I am open for suggestions. > One > more thought is to use FOR VALUES HAVING like: > CREATE TABLE foo1 PARTITION OF foo FOR VALUES HAVING (modulus 2, remainder > 1); > > But still more thoughts/inputs welcome here. > > Attached patch implements former syntax, here is quick demonstration: > > 1.CREATE : > create table foo (a integer, b text) partition by hash (a); > create table foo1 partition of foo with (modulus 2, remainder 1); > create table foo2 partition of foo with (modulus 4, remainder 2); > create table foo3 partition of foo with (modulus 8
Re: [HACKERS] [POC] hash partitioning
Hi Aleksander ,
Thank you for reviewing the patch.
On Wed, 1 Mar 2017 17:08:49 +0300
Aleksander Alekseev <a.aleks...@postgrespro.ru> wrote:
> Hi, Yugo.
>
> Today I've had an opportunity to take a closer look on this patch. Here are
> a few things that bother me.
>
> 1a) There are missing commends here:
>
> ```
> --- a/src/include/catalog/pg_partitioned_table.h
> +++ b/src/include/catalog/pg_partitioned_table.h
> @@ -33,6 +33,9 @@ CATALOG(pg_partitioned_table,3350) BKI_WITHOUT_OIDS
> charpartstrat; /* partitioning strategy */
> int16 partnatts; /* number of partition key columns */
>
> + int16 partnparts;
> + Oid parthashfunc;
> +
> ```
>
> 1b) ... and here:
>
> ```
> --- a/src/include/nodes/parsenodes.h
> +++ b/src/include/nodes/parsenodes.h
> @@ -730,11 +730,14 @@ typedef struct PartitionSpec
> NodeTag type;
> char *strategy; /* partitioning strategy ('list' or 'range')
> */
> List *partParams; /* List of PartitionElems */
> + int partnparts;
> + List *hashfunc;
> int location; /* token location, or -1 if unknown */
> } PartitionSpec;
> ```
ok, I'll add comments for these members;
>
> 2) I believe new empty lines in patches are generally not welcomed by
> community:
>
> ```
> @@ -49,6 +52,8 @@ CATALOG(pg_partitioned_table,3350) BKI_WITHOUT_OIDS
> pg_node_tree partexprs; /* list of expressions in the partition key;
> * one item for each zero entry in
> partattrs[] */
> #endif
> +
> +
> } FormData_pg_partitioned_table;
> ```
I'll remove it from the patch.
>
> 3) One test fails on my laptop (Arch Linux, x64) [1]:
>
> ```
> ***
> *** 344,350
> CREATE TABLE partitioned (
> a int
> ) PARTITION BY HASH (a);
> ! ERROR: unrecognized partitioning strategy "hash"
> -- specified column must be present in the table
> CREATE TABLE partitioned (
> a int
> --- 344,350
> CREATE TABLE partitioned (
> a int
> ) PARTITION BY HASH (a);
> ! ERROR: number of partitions must be specified for hash partition
> -- specified column must be present in the table
> CREATE TABLE partitioned (
> a int
> ```
These are expected behaviors in the current patch. However, there
are some discussions on the specification about CREATE TABLE, so
it may be changed.
>
> Exact script I'm using for building and testing PostgreSQL could be
> found here [2].
>
> 4) As I already mentioned - missing documentation.
I think writing the documentation should be waited fo the specification
getting a consensus.
>
> In general patch looks quite good to me. I personally believe it has all
> the changes to be accepted in current commitfest. Naturally if community
> will come to a consensus regarding keywords, whether all partitions
> should be created automatically, etc :)
>
> [1] http://afiskon.ru/s/dd/20cbe21934_regression.diffs.txt
> [2] http://afiskon.ru/s/76/a4fb71739c_full-build.sh.txt
>
> On Wed, Mar 01, 2017 at 06:10:10PM +0900, Yugo Nagata wrote:
> > Hi Aleksander,
> >
> > On Tue, 28 Feb 2017 18:05:36 +0300
> > Aleksander Alekseev <a.aleks...@postgrespro.ru> wrote:
> >
> > > Hi, Yugo.
> > >
> > > Looks like a great feature! I'm going to take a closer look on your code
> > > and write a feedback shortly. For now I can only tell that you forgot
> > > to include some documentation in the patch.
> >
> > Thank you for looking into it. I'm forward to your feedback.
> > This is a proof of concept patch and additional documentation
> > is not included. I'll add this after reaching a consensus
> > on the specification of the feature.
> >
> > >
> > > I've added a corresponding entry to current commitfest [1]. Hope you
> > > don't mind. If it's not too much trouble could you please register on a
> > > commitfest site and add yourself to this entry as an author? I'm pretty
> > > sure someone is using this information for writing release notes or
> > > something like this.
> >
> > Thank you for registering it to the commitfest. I have added me as an
> > auther.
> >
> > >
> > > [1] https://commitfest.postgresql.org/13/1059/
> > >
> > > On Tue, Feb 28, 2017 at 11:33:13PM +0900, Yugo Nagata wrote:
> > > > Hi all,
> > > >
> > > > Now we have a declarative partitioning, but hash partitioning is not
> > > > implemented yet. Attached is a POC patch to add the hash p
Re: [HACKERS] [POC] hash partitioning
On Wed, 1 Mar 2017 10:52:58 +0530 amul sul <sula...@gmail.com> wrote: > On Tue, Feb 28, 2017 at 8:03 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > Hi all, > > > > Now we have a declarative partitioning, but hash partitioning is not > > implemented yet. Attached is a POC patch to add the hash partitioning > > feature. I know we will need more discussions about the syntax and other > > specifications before going ahead the project, but I think this runnable > > code might help to discuss what and how we implement this. > > > > Great. Thanks. > > > * Description > > > > In this patch, the hash partitioning implementation is basically based > > on the list partitioning mechanism. However, partition bounds cannot be > > specified explicitly, but this is used internally as hash partition > > index, which is calculated when a partition is created or attached. > > > > The tentative syntax to create a partitioned table is as bellow; > > > > CREATE TABLE h (i int) PARTITION BY HASH(i) PARTITIONS 3 USING hashint4; > > > > The number of partitions is specified by PARTITIONS, which is currently > > constant and cannot be changed, but I think this is needed to be changed > in > > some manner. A hash function is specified by USING. Maybe, specifying hash > > function may be ommitted, and in this case, a default hash function > > corresponding to key type will be used. > > > > A partition table can be create as bellow; > > > > CREATE TABLE h1 PARTITION OF h; > > CREATE TABLE h2 PARTITION OF h; > > CREATE TABLE h3 PARTITION OF h; > > > > FOR VALUES clause cannot be used, and the partition bound is > > calclulated automatically as partition index of single integer value. > > > > When trying create partitions more than the number specified > > by PARTITIONS, it gets an error. > > > > postgres=# create table h4 partition of h; > > ERROR: cannot create hash partition more than 3 for h > > > > An inserted record is stored in a partition whose index equals > > abs(hashfunc(key)) % . In the above > > example, this is abs(hashint4(i))%3. > > > > postgres=# insert into h (select generate_series(0,20)); > > INSERT 0 21 > > > > postgres=# select *,tableoid::regclass from h; > > i | tableoid > > +-- > > 0 | h1 > > 1 | h1 > > 2 | h1 > > 4 | h1 > > 8 | h1 > > 10 | h1 > > 11 | h1 > > 14 | h1 > > 15 | h1 > > 17 | h1 > > 20 | h1 > > 5 | h2 > > 12 | h2 > > 13 | h2 > > 16 | h2 > > 19 | h2 > > 3 | h3 > > 6 | h3 > > 7 | h3 > > 9 | h3 > > 18 | h3 > > (21 rows) > > > > * Todo / discussions > > > > In this patch, we cannot change the number of partitions specified > > by PARTITIONS. I we can change this, the partitioning rule > > ( = abs(hashfunc(key)) % ) > > is also changed and then we need reallocatiing records between > > partitions. > > > > In this patch, user can specify a hash function USING. However, > > we migth need default hash functions which are useful and > > proper for hash partitioning. > > > IMHO, we should try to keep create partition syntax simple and aligned > with other partition strategy. For e.g: > CREATE TABLE h (i int) PARTITION BY HASH(i); > > I Agree that it is unavoidable partitions number in modulo hashing, > but we can do in other hashing technique. Have you had thought about > Linear hashing[1] or Consistent hashing[2]? This will allow us to > add/drop > partition with minimal row moment. Thank you for your information of hash technique. I'll see them and try to allowing the number of partitions to be changed. Thanks, Yugo Nagata > > +1 for the default hash function corresponding to partitioning key type. > > Regards, > Amul > > > [1] https://en.wikipedia.org/wiki/Linear_hashing > [2] https://en.wikipedia.org/wiki/Consistent_hashing -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
On Wed, 1 Mar 2017 10:30:09 +0530 Rushabh Lathia <rushabh.lat...@gmail.com> wrote: > On Tue, Feb 28, 2017 at 8:03 PM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > > Hi all, > > > > Now we have a declarative partitioning, but hash partitioning is not > > implemented yet. Attached is a POC patch to add the hash partitioning > > feature. I know we will need more discussions about the syntax and other > > specifications before going ahead the project, but I think this runnable > > code might help to discuss what and how we implement this. > > > > * Description > > > > In this patch, the hash partitioning implementation is basically based > > on the list partitioning mechanism. However, partition bounds cannot be > > specified explicitly, but this is used internally as hash partition > > index, which is calculated when a partition is created or attached. > > > > The tentative syntax to create a partitioned table is as bellow; > > > > CREATE TABLE h (i int) PARTITION BY HASH(i) PARTITIONS 3 USING hashint4; > > > > The number of partitions is specified by PARTITIONS, which is currently > > constant and cannot be changed, but I think this is needed to be changed in > > some manner. A hash function is specified by USING. Maybe, specifying hash > > function may be ommitted, and in this case, a default hash function > > corresponding to key type will be used. > > > > A partition table can be create as bellow; > > > > CREATE TABLE h1 PARTITION OF h; > > CREATE TABLE h2 PARTITION OF h; > > CREATE TABLE h3 PARTITION OF h; > > > > FOR VALUES clause cannot be used, and the partition bound is > > calclulated automatically as partition index of single integer value. > > > > When trying create partitions more than the number specified > > by PARTITIONS, it gets an error. > > > > postgres=# create table h4 partition of h; > > ERROR: cannot create hash partition more than 3 for h > > > > An inserted record is stored in a partition whose index equals > > abs(hashfunc(key)) % . In the above > > example, this is abs(hashint4(i))%3. > > > > postgres=# insert into h (select generate_series(0,20)); > > INSERT 0 21 > > > > postgres=# select *,tableoid::regclass from h; > > i | tableoid > > +-- > > 0 | h1 > > 1 | h1 > > 2 | h1 > > 4 | h1 > > 8 | h1 > > 10 | h1 > > 11 | h1 > > 14 | h1 > > 15 | h1 > > 17 | h1 > > 20 | h1 > > 5 | h2 > > 12 | h2 > > 13 | h2 > > 16 | h2 > > 19 | h2 > > 3 | h3 > > 6 | h3 > > 7 | h3 > > 9 | h3 > > 18 | h3 > > (21 rows) > > > > > This is good, I will have closer look into the patch, but here are > few quick comments. Thanks. I'm looking forward to your comments. > > - CREATE HASH partition syntax adds two new keywords and ideally > we should try to avoid adding additional keywords. Also I can see that > HASH keyword been added, but I don't see any use of newly added > keyword in gram.y. Yes, you are right. HASH keyword is not necessary. I'll remove it from the patch. > > - Also I didn't like the idea of fixing number of partitions during the > CREATE > TABLE syntax. Thats something that needs to be able to changes. I agree. The number specified by PARTIONS should be the *initial* number of partitions and this should be abelt to be changed. I'm investigating the way. > > > > > * Todo / discussions > > > > In this patch, we cannot change the number of partitions specified > > by PARTITIONS. I we can change this, the partitioning rule > > ( = abs(hashfunc(key)) % ) > > is also changed and then we need reallocatiing records between > > partitions. > > > > In this patch, user can specify a hash function USING. However, > > we migth need default hash functions which are useful and > > proper for hash partitioning. > > > > +1 > > - With fixing default hash function and not specifying number of partitions > during CREATE TABLE - don't need two new additional columns into > pg_partitioned_table catalog. I think the option to specify a hash function is needed because user may want to use a user-defined hash function for some reasons, for example, when a user-defined type is used as a partition key. > > > > Currently, even when we issue SELECT query with a condition, > > postgres looks into all partitions regardless of each partition's > > constraint, because this is complicated such like "abs(hashint4(i))%3 = 0". > > &
Re: [HACKERS] [POC] hash partitioning
Hi Ammit, On Wed, 1 Mar 2017 11:14:15 +0900 Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > Nagata-san, > > On 2017/02/28 23:33, Yugo Nagata wrote: > > Hi all, > > > > Now we have a declarative partitioning, but hash partitioning is not > > implemented yet. Attached is a POC patch to add the hash partitioning > > feature. I know we will need more discussions about the syntax and other > > specifications before going ahead the project, but I think this runnable > > code might help to discuss what and how we implement this. > > Great! Thank you! > > > * Description > > > > In this patch, the hash partitioning implementation is basically based > > on the list partitioning mechanism. However, partition bounds cannot be > > specified explicitly, but this is used internally as hash partition > > index, which is calculated when a partition is created or attached. > > > > The tentative syntax to create a partitioned table is as bellow; > > > > CREATE TABLE h (i int) PARTITION BY HASH(i) PARTITIONS 3 USING hashint4; > > > > The number of partitions is specified by PARTITIONS, which is currently > > constant and cannot be changed, but I think this is needed to be changed in > > some manner. A hash function is specified by USING. Maybe, specifying hash > > function may be ommitted, and in this case, a default hash function > > corresponding to key type will be used. > > > > A partition table can be create as bellow; > > > > CREATE TABLE h1 PARTITION OF h; > > CREATE TABLE h2 PARTITION OF h; > > CREATE TABLE h3 PARTITION OF h; > > > > FOR VALUES clause cannot be used, and the partition bound is > > calclulated automatically as partition index of single integer value. > > > > When trying create partitions more than the number specified > > by PARTITIONS, it gets an error. > > > > postgres=# create table h4 partition of h; > > ERROR: cannot create hash partition more than 3 for h > > Instead of having to create each partition individually, wouldn't it be > better if the following command > > CREATE TABLE h (i int) PARTITION BY HASH (i) PARTITIONS 3; > > created the partitions *automatically*? > > It makes sense to provide a way to create individual list and range > partitions separately, because users can specify custom bounds for each. > We don't need that for hash partitions, so why make users run separate > commands (without the FOR VALUES clause) anyway? We may perhaps need to > offer a way to optionally specify a user-defined name for each partition > in the same command, along with tablespace, storage options, etc. By > default, the names would be generated internally and the user can ALTER > individual partitions after the fact to specify tablespace, etc. I though that creating each partition individually is needed because some user will want to specify a tablespce to each partition. However, as you say, that isn't need for many cases because use can move a partition to other tablespaces afterward by ALTER. Thanks, Yugo Nagata > > Thanks, > Amit > > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [POC] hash partitioning
Hi Aleksander, On Tue, 28 Feb 2017 18:05:36 +0300 Aleksander Alekseev <a.aleks...@postgrespro.ru> wrote: > Hi, Yugo. > > Looks like a great feature! I'm going to take a closer look on your code > and write a feedback shortly. For now I can only tell that you forgot > to include some documentation in the patch. Thank you for looking into it. I'm forward to your feedback. This is a proof of concept patch and additional documentation is not included. I'll add this after reaching a consensus on the specification of the feature. > > I've added a corresponding entry to current commitfest [1]. Hope you > don't mind. If it's not too much trouble could you please register on a > commitfest site and add yourself to this entry as an author? I'm pretty > sure someone is using this information for writing release notes or > something like this. Thank you for registering it to the commitfest. I have added me as an auther. > > [1] https://commitfest.postgresql.org/13/1059/ > > On Tue, Feb 28, 2017 at 11:33:13PM +0900, Yugo Nagata wrote: > > Hi all, > > > > Now we have a declarative partitioning, but hash partitioning is not > > implemented yet. Attached is a POC patch to add the hash partitioning > > feature. I know we will need more discussions about the syntax and other > > specifications before going ahead the project, but I think this runnable > > code might help to discuss what and how we implement this. > > > > * Description > > > > In this patch, the hash partitioning implementation is basically based > > on the list partitioning mechanism. However, partition bounds cannot be > > specified explicitly, but this is used internally as hash partition > > index, which is calculated when a partition is created or attached. > > > > The tentative syntax to create a partitioned table is as bellow; > > > > CREATE TABLE h (i int) PARTITION BY HASH(i) PARTITIONS 3 USING hashint4; > > > > The number of partitions is specified by PARTITIONS, which is currently > > constant and cannot be changed, but I think this is needed to be changed in > > some manner. A hash function is specified by USING. Maybe, specifying hash > > function may be ommitted, and in this case, a default hash function > > corresponding to key type will be used. > > > > A partition table can be create as bellow; > > > > CREATE TABLE h1 PARTITION OF h; > > CREATE TABLE h2 PARTITION OF h; > > CREATE TABLE h3 PARTITION OF h; > > > > FOR VALUES clause cannot be used, and the partition bound is > > calclulated automatically as partition index of single integer value. > > > > When trying create partitions more than the number specified > > by PARTITIONS, it gets an error. > > > > postgres=# create table h4 partition of h; > > ERROR: cannot create hash partition more than 3 for h > > > > An inserted record is stored in a partition whose index equals > > abs(hashfunc(key)) % . In the above > > example, this is abs(hashint4(i))%3. > > > > postgres=# insert into h (select generate_series(0,20)); > > INSERT 0 21 > > > > postgres=# select *,tableoid::regclass from h; > > i | tableoid > > +-- > > 0 | h1 > > 1 | h1 > > 2 | h1 > > 4 | h1 > > 8 | h1 > > 10 | h1 > > 11 | h1 > > 14 | h1 > > 15 | h1 > > 17 | h1 > > 20 | h1 > > 5 | h2 > > 12 | h2 > > 13 | h2 > > 16 | h2 > > 19 | h2 > > 3 | h3 > > 6 | h3 > > 7 | h3 > > 9 | h3 > > 18 | h3 > > (21 rows) > > > > * Todo / discussions > > > > In this patch, we cannot change the number of partitions specified > > by PARTITIONS. I we can change this, the partitioning rule > > ( = abs(hashfunc(key)) % ) > > is also changed and then we need reallocatiing records between > > partitions. > > > > In this patch, user can specify a hash function USING. However, > > we migth need default hash functions which are useful and > > proper for hash partitioning. > > > > Currently, even when we issue SELECT query with a condition, > > postgres looks into all partitions regardless of each partition's > > constraint, because this is complicated such like "abs(hashint4(i))%3 = 0". > > > > postgres=# explain select * from h where i = 10; > > QUERY PLAN > > -- > > Append (cost=0.00..125.62 rows=40 width=4) > >-> Seq Scan on h (cost=0.00..0.00 ro
[HACKERS] [POC] hash partitioning
Hi all,
Now we have a declarative partitioning, but hash partitioning is not
implemented yet. Attached is a POC patch to add the hash partitioning
feature. I know we will need more discussions about the syntax and other
specifications before going ahead the project, but I think this runnable
code might help to discuss what and how we implement this.
* Description
In this patch, the hash partitioning implementation is basically based
on the list partitioning mechanism. However, partition bounds cannot be
specified explicitly, but this is used internally as hash partition
index, which is calculated when a partition is created or attached.
The tentative syntax to create a partitioned table is as bellow;
CREATE TABLE h (i int) PARTITION BY HASH(i) PARTITIONS 3 USING hashint4;
The number of partitions is specified by PARTITIONS, which is currently
constant and cannot be changed, but I think this is needed to be changed in
some manner. A hash function is specified by USING. Maybe, specifying hash
function may be ommitted, and in this case, a default hash function
corresponding to key type will be used.
A partition table can be create as bellow;
CREATE TABLE h1 PARTITION OF h;
CREATE TABLE h2 PARTITION OF h;
CREATE TABLE h3 PARTITION OF h;
FOR VALUES clause cannot be used, and the partition bound is
calclulated automatically as partition index of single integer value.
When trying create partitions more than the number specified
by PARTITIONS, it gets an error.
postgres=# create table h4 partition of h;
ERROR: cannot create hash partition more than 3 for h
An inserted record is stored in a partition whose index equals
abs(hashfunc(key)) % . In the above
example, this is abs(hashint4(i))%3.
postgres=# insert into h (select generate_series(0,20));
INSERT 0 21
postgres=# select *,tableoid::regclass from h;
i | tableoid
+--
0 | h1
1 | h1
2 | h1
4 | h1
8 | h1
10 | h1
11 | h1
14 | h1
15 | h1
17 | h1
20 | h1
5 | h2
12 | h2
13 | h2
16 | h2
19 | h2
3 | h3
6 | h3
7 | h3
9 | h3
18 | h3
(21 rows)
* Todo / discussions
In this patch, we cannot change the number of partitions specified
by PARTITIONS. I we can change this, the partitioning rule
( = abs(hashfunc(key)) % )
is also changed and then we need reallocatiing records between
partitions.
In this patch, user can specify a hash function USING. However,
we migth need default hash functions which are useful and
proper for hash partitioning.
Currently, even when we issue SELECT query with a condition,
postgres looks into all partitions regardless of each partition's
constraint, because this is complicated such like "abs(hashint4(i))%3 = 0".
postgres=# explain select * from h where i = 10;
QUERY PLAN
--
Append (cost=0.00..125.62 rows=40 width=4)
-> Seq Scan on h (cost=0.00..0.00 rows=1 width=4)
Filter: (i = 10)
-> Seq Scan on h1 (cost=0.00..41.88 rows=13 width=4)
Filter: (i = 10)
-> Seq Scan on h2 (cost=0.00..41.88 rows=13 width=4)
Filter: (i = 10)
-> Seq Scan on h3 (cost=0.00..41.88 rows=13 width=4)
Filter: (i = 10)
(9 rows)
However, if we modify a condition into a same expression
as the partitions constraint, postgres can exclude unrelated
table from search targets. So, we might avoid the problem
by converting the qual properly before calling predicate_refuted_by().
postgres=# explain select * from h where abs(hashint4(i))%3 =
abs(hashint4(10))%3;
QUERY PLAN
--
Append (cost=0.00..61.00 rows=14 width=4)
-> Seq Scan on h (cost=0.00..0.00 rows=1 width=4)
Filter: ((abs(hashint4(i)) % 3) = 2)
-> Seq Scan on h3 (cost=0.00..61.00 rows=13 width=4)
Filter: ((abs(hashint4(i)) % 3) = 2)
(5 rows)
Best regards,
Yugo Nagata
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 41c0056..3820920 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3074,7 +3074,7 @@ StorePartitionKey(Relation rel,
AttrNumber *partattrs,
List *partexprs,
Oid *partopclass,
- Oid *partcollation)
+ Oid *partcollation, int16 partnparts, Oid hashfunc)
{
int i;
int2vector *partattrs_vec;
@@ -3121,6 +3121,8 @@ StorePartitionKey(Relation rel,
values[Anum_pg_partitioned_table_partrelid - 1] = ObjectIdGetDatum(RelationGetRelid(rel));
values[Anum_pg_partitioned_table_partstrat - 1] = CharGetDatum(strategy);
values[Anum_pg_partitioned_table_partnatts - 1] = Int16GetDatum(partnatts);
+ values[Anum_pg_partitioned_table_partnparts - 1] = Int16GetDatum(partnparts);
+ values[Anum_pg_partitioned_table_parthashfunc - 1] = ObjectIdGetDatum(hashfunc);
values[Anum_pg_partitioned_table_partattrs -
Re: [HACKERS] Declarative partitioning - another take
Hi Amit, On Thu, 23 Feb 2017 16:29:32 +0900 Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > Thanks for taking care of that. > > + * PartitionRoot relation descriptor for parent > relation > > Maybe: relation descriptor for root parent relation This seems better. Patch is updated. Thanks, -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 6332ea0..af3367a 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -326,6 +326,7 @@ typedef struct JunkFilter * onConflictSetWhere list of ON CONFLICT DO UPDATE exprs (qual) * PartitionCheck partition check expression * PartitionCheckExpr partition check expression state + * PartitionRoot relation descriptor for root parent relation * */ typedef struct ResultRelInfo -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Declarative partitioning - another take
Hi, I found that a comment for PartitionRoot in ResultRelInfo is missing. Although this is trivial, since all other members have comments, I think it is needed. Attached is the patch to fix it. Regards, Yugo Nagata On Tue, 27 Dec 2016 17:59:05 +0900 Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > On 2016/12/26 19:46, Amit Langote wrote: > > (Perhaps, the following should be its own new thread) > > > > I noticed that ExecProcessReturning() doesn't work properly after tuple > > routing (example shows how returning tableoid currently fails but I > > mention some other issues below): > > > > create table p (a int, b int) partition by range (a); > > create table p1 partition of p for values from (1) to (10); > > insert into p values (1) returning tableoid::regclass, *; > > tableoid | a | b > > --+---+--- > > -| 1 | > > (1 row) > > > > INSERT 0 1 > > > > I tried to fix that in 0007 to get: > > > > insert into p values (1) returning tableoid::regclass, *; > > tableoid | a | b > > --+---+--- > > p| 1 | > > (1 row) > > > > INSERT 0 1 > > > > But I think it *may* be wrong to return the root table OID for tuples > > inserted into leaf partitions, because with select we get partition OIDs: > > > > select tableoid::regclass, * from p; > > tableoid | a | b > > --+---+--- > > p1 | 1 | > > (1 row) > > > > If so, that means we should build the projection info (corresponding to > > the returning list) for each target partition somehow. ISTM, that's going > > to have to be done within the planner by appropriate inheritance > > translation of the original returning targetlist. > > Turns out getting the 2nd result may not require planner tweaks after all. > Unless I'm missing something, translation of varattnos of the RETURNING > target list can be done as late as ExecInitModifyTable() for the insert > case, unlike update/delete (which do require planner's attention). > > I updated the patch 0007 to implement the same, including the test. While > doing that, I realized map_partition_varattnos introduced in 0003 is > rather restrictive in its applicability, because it assumes varno = 1 for > the expressions it accepts as input for the mapping. Mapping returning > (target) list required modifying map_partition_varattnos to accept > target_varno as additional argument. That way, we can map arbitrary > expressions from the parent attributes numbers to partition attribute > numbers for expressions not limited to partition constraints. > > Patches 0001 to 0006 unchanged. > > Thanks, > Amit -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 6332ea0..845bdf2 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -326,6 +326,7 @@ typedef struct JunkFilter * onConflictSetWhere list of ON CONFLICT DO UPDATE exprs (qual) * PartitionCheck partition check expression * PartitionCheckExpr partition check expression state + * PartitionRoot relation descriptor for parent relation * */ typedef struct ResultRelInfo -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Documentation improvements for partitioning
Hi,
There is a very small typo that a comma is missing.
Attached is the patch to fix it.
On Wed, 15 Feb 2017 07:57:53 -0500
Robert Haas <robertmh...@gmail.com> wrote:
> On Wed, Feb 15, 2017 at 4:26 AM, Ashutosh Bapat
> <ashutosh.ba...@enterprisedb.com> wrote:
> > Noticed some typos in the documentation. Here's patch to correct
> > those. Sorry, if it has been already taken care of.
>
> Thanks. That is indeed nonstandard capitalization. Committed.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
--
Yugo Nagata <nag...@sraoss.co.jp>
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 5779eac..ef0f7cf 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2899,7 +2899,7 @@ VALUES ('Albany', NULL, NULL, 'NY');
- Since primary keys are not supported on partitioned tables
+ Since primary keys are not supported on partitioned tables,
foreign keys referencing partitioned tables are not supported, nor
are foreign key references from a partitioned table to some other table.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backport of pg_statistics typos fix
On Wed, 8 Feb 2017 14:54:17 -0500 Robert Haas <robertmh...@gmail.com> wrote: > On Tue, Feb 7, 2017 at 4:22 AM, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > I found typos "pg_statistics" in REL9_6_STABLE, but that has been > > fixed in the master branch. > > > > Fix typo: pg_statistics -> pg_statistic > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=5a366b4ff4ceceb9793fcc13c3f097ee0d32c56d;hp=f7c62462402972b13d10e43f104ca0c0fecb6d08 > > > > I think it would be better to backport this to other branches. > > We usually leave such decisions to the discretion of the committer, > because back-porting such changes takes time and sometimes it just > isn't that important. Nobody's likely to be confused by a few > instances of writing pg_statistics rather than pg_statistic. > Personally, I favor not back-porting such things in most cases, > because I think patches that get back-ported should be strictly > limited to bug fixes, and typos in code comments aren't bug fixes. > But not everyone has the same opinion on this. What's your reason for > wanting it back-ported? I agree typos in code comments aren't bug fixes and not need to get back-ported. However, there are typos also in the document. The scalarltsel function retrieves the histogram for unique1 from - pg_statistics. For manual queries it is more + pg_statistic. For manual queries it is more convenient to look in the simpler pg_stats view: I think this might be a document bug, but if nobody cares of it, I also don't mind. Thanks, > > BTW, looking at that commit, this change looks to have adjusted this > from being wrong to still being wrong: > > -Allow pg_statistics to be reset by calling > pg_stat_reset() (Christopher) > +Allow pg_statistic to be reset by calling > pg_stat_reset() (Christopher) > > It's true that pg_stat_reset() doesn't reset the nonexistent > pg_statistics table. But it doesn't reset pg_statistic either. IIUC, > it resets the data gathered by the statistics collector, which is > something else altogether. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Postgres_fdw behaves oddly
27:14.332 JST [3075] DETAIL: Key (val)=(3) already exists. > 2017-02-03 15:27:14.332 JST [3075] CONTEXT: Remote SQL command: COMMIT > TRANSACTION > 2017-02-03 15:27:14.332 JST [3075] STATEMENT: COMMIT; > 2017-02-03 15:27:14.332 JST [3084] WARNING: there is no transaction in > progress > WARNING: there is no transaction in progress > ERROR: duplicate key value violates unique constraint "lt_val_key" > DETAIL: Key (val)=(3) already exists. > CONTEXT: Remote SQL command: COMMIT TRANSACTION > postgres=# SELECT * FROM lt; > val > - > 1 > 3 > 4 > (3 rows) > *Test 4:** > **===* > In a transaction insert two rows one each to the two foreign tables. > Both the rows violates the constraint. So at the time of COMMIT it > returns error. I think this is also expected behavior. > > postgres=# BEGIN; > BEGIN > postgres=# INSERT INTO ft1_lt VALUES (1); -- Violates constraint > INSERT 0 1 > postgres=# INSERT INTO ft2_lt VALUES (3); -- Violates constraint > INSERT 0 1 > postgres=# COMMIT; > 2017-02-03 15:29:18.857 JST [3081] ERROR: duplicate key value violates > unique constraint "lt_val_key" > 2017-02-03 15:29:18.857 JST [3081] DETAIL: Key (val)=(1) already exists. > 2017-02-03 15:29:18.857 JST [3081] STATEMENT: COMMIT TRANSACTION > 2017-02-03 15:29:18.858 JST [3075] ERROR: duplicate key value violates > unique constraint "lt_val_key" > 2017-02-03 15:29:18.858 JST [3075] DETAIL: Key (val)=(1) already exists. > 2017-02-03 15:29:18.858 JST [3075] CONTEXT: Remote SQL command: COMMIT > TRANSACTION > 2017-02-03 15:29:18.858 JST [3075] STATEMENT: COMMIT; > 2017-02-03 15:29:18.858 JST [3081] WARNING: there is no transaction in > progress > WARNING: there is no transaction in progress > ERROR: duplicate key value violates unique constraint "lt_val_key" > DETAIL: Key (val)=(1) already exists. > CONTEXT: Remote SQL command: COMMIT TRANSACTION > postgres=# > postgres=# SELECT * FROM lt; > val > - > 1 > 3 > 4 > (3 rows) > *Test 5:** > **===* > In a transaction insert two rows one each to the two foreign tables. > Both the rows violates the constraint. Here error is expected at COMMIT > time but transaction does not give any error and it takes lock waiting > for a transaction to finish. > postgres=# BEGIN; > BEGIN > postgres=# INSERT INTO ft1_lt VALUES *(3)*; -- Violates constraint > INSERT 0 1 > postgres=# INSERT INTO ft2_lt VALUES *(3)*; -- Violates constraint > INSERT 0 1 > postgres=# COMMIT; > . > . > . > > postgres=# select datid,datname,pid,wait_event_type,wait_event,query > from pg_stat_activity; > -[ RECORD 1 > ]---+- > datid | 13123 > datname | postgres > pid | 3654 > wait_event_type | *Lock* > wait_event | *transactionid* > query | COMMIT TRANSACTION > > Note: Test 4 and Test 5 are same but in Test 5 both the foreign servers > trying to insert the same data. > > Is this a expected behavior of postgres_fdw? > > Regards, > Vinayak Pokale > > NTT Open Source Software Center > -- Yugo Nagata <nag...@sraoss.co.jp> test=# select * from pg_stat_activity ; -[ RECORD 1 ]+- datid| 16384 datname | test pid | 16626 usesysid | 10 usename | yugo-n application_name | psql client_addr | client_hostname | client_port | -1 backend_start| 2017-02-07 20:38:28.629562+09 xact_start | 2017-02-07 20:41:21.978381+09 query_start | 2017-02-07 20:41:29.118791+09 state_change | 2017-02-07 20:41:29.118796+09 wait_event_type | wait_event | state| active backend_xid | backend_xmin | query| end; -[ RECORD 2 ]+- datid| 16384 datname | test pid | 16796 usesysid | 10 usename | yugo-n application_name | postgres_fdw client_addr | client_hostname | client_port | -1 backend_start| 2017-02-07 20:38:49.664541+09 xact_start | 2017-02-07 20:41:25.25321+09 query_start | 2017-02-07 20:41:25.254316+09 state_change | 2017-02-07 20:41:25.254369+09 wait_event_type | wait_event | state| idle in transaction backend_xid | 9088 backend_xmin | 9088 query| DEALLOCATE pgsql_fdw_prep_4 -[ RECORD 3 ]+- datid| 16384 datname | test pid | 16840 usesysid | 10 usename | yugo-n application_name | postgres_fdw client_addr | client_hostname | client_port | -1
[HACKERS] Backport of pg_statistics typos fix
Hi, I found typos "pg_statistics" in REL9_6_STABLE, but that has been fixed in the master branch. Fix typo: pg_statistics -> pg_statistic https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=5a366b4ff4ceceb9793fcc13c3f097ee0d32c56d;hp=f7c62462402972b13d10e43f104ca0c0fecb6d08 I think it would be better to backport this to other branches. Regards, -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix a comment in feelist.c
Hi, I've found a mistake in a comment of StrategyNotifyBgWriter in freelist.c. bgwriterLatch was replaced by bgwprocno in the following commit, but this is remained in the comment. commit d72731a70450b5e7084991b9caa15cb58a2820df Author: Andres Freund <and...@anarazel.de> Date: Thu Dec 25 18:24:20 2014 +0100 Lockless StrategyGetBuffer clock sweep hot path. Attached a patch. -- Yugo Nagata <nag...@sraoss.co.jp> diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index b68ab20..5d0a636 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -406,8 +406,8 @@ StrategySyncStart(uint32 *complete_passes, uint32 *num_buf_alloc) /* * StrategyNotifyBgWriter -- set or clear allocation notification latch * - * If bgwriterLatch isn't NULL, the next invocation of StrategyGetBuffer will - * set that latch. Pass NULL to clear the pending notification before it + * If bgwprocno isn't -1, the next invocation of StrategyGetBuffer will + * set that latch. Pass -1 to clear the pending notification before it * happens. This feature is used by the bgwriter process to wake itself up * from hibernation, and is not meant for anybody else to use. */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] per-statement-level INSTEAD OF triggers
Hi hackers, I'm asking out of curiosity, do anyone know why we don't have per-statement-level INSTEAD OF triggers? I looked into the standard SQL (20xx draft), but I can't find the restriction such that INSTEAD OF triggers must be row-level. Is there any technical difficulties, or other reasons for the current implementation? Best regards, -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] T_PrivGrantee is left in NodeTag
Hi, I found that codes about T_PrivGrantee was removed by the following commit; http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=31eae6028eca4365e7165f5f33fee1ed0486aee0 but T_PrivGrantee is left in NodeTag in src/include/nodes/nodes.h. Is it intended? -- Yugo Nagata <nag...@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Fwd: [HACKERS] Proposal: variant of regclass
On Mon, 7 Apr 2014 12:00:49 -0400 Robert Haas robertmh...@gmail.com wrote: In other words, let's revert the whole refactoring of this file to create reg*_guts functions, and instead just copy the relevant logic for the name lookups into the new functions. For to_regproc(), for example, it would look like this (untested): names = stringToQualifiedNameList(pro_name_or_oid); clist = FuncnameGetCandidates(names, -1, NIL, false, false, false); if (clist == NULL || clist-next != NULL) result = InvalidOid; else result = clist-oid; With that change, this patch will actually get a whole lot smaller, change less already-existing code, and deliver cleaner behavior. Here is an updated patch. I rewrote regproc.c not to use _guts functions, and fixed to_reg* not accept a numeric OID. I also updated the documents and some comments. Is this better than the previous one? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Yugo Nagata nag...@sraoss.co.jp to_regclass.patch.v7 Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Fwd: [HACKERS] Proposal: variant of regclass
On Thu, 23 Jan 2014 13:19:37 +0200
Marti Raudsepp ma...@juffo.org wrote:
Resending to Tatsuo Ishii and Yugo Nagata, your email server was
having problems yesterday:
Thanks for resending!
This is the mail system at host sraigw2.sra.co.jp.
yug...@sranhm.sra.co.jp: mail for srasce.sra.co.jp loops back to myself
t-is...@sra.co.jp: mail for srasce.sra.co.jp loops back to myself
-- Forwarded message --
From: Marti Raudsepp ma...@juffo.org
Date: Thu, Jan 23, 2014 at 3:39 AM
Subject: Re: [HACKERS] Proposal: variant of regclass
To: Yugo Nagata nag...@sraoss.co.jp
Cc: Tatsuo Ishii is...@postgresql.org, pgsql-hackers
pgsql-hackers@postgresql.org, Vik Fearing vik.fear...@dalibo.com,
Robert Haas robertmh...@gmail.com, Tom Lane t...@sss.pgh.pa.us,
Pavel Golub pa...@gf.microolap.com, Pavel Golub
pa...@microolap.com, Andres Freund and...@2ndquadrant.com, Pavel
Stěhule pavel.steh...@gmail.com
On Wed, Jan 22, 2014 at 1:44 PM, Yugo Nagata nag...@sraoss.co.jp wrote:
On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
Tatsuo Ishii is...@postgresql.org wrote:
parseTypeString() is called by some other functions and I avoided
influences of modifying the definition on them, since this should
raise errors in most cases. This is same reason for other *MissingOk
functions in parse_type.c.
Is it better to write definitions of these function and all there callers?
Yes, for parseTypeString certainly. There have been many refactorings
like that in the past and all of them use this pattern.
Ok. I'll rewrite the definition and there callers.
typenameTypeIdAndMod is less clear since the code paths differ so
much, maybe keep 2 versions (merging back to 1 function is OK too, but
in any case you don't need 3).
I'll also fix this in either way to not use typenameTypeIdAndMod_guts.
typenameTypeIdAndModMissingOk(...)
{
Type tup = LookupTypeName(pstate, typeName, typmod_p);
if (tup == NULL || !((Form_pg_type) GETSTRUCT(tup))-typisdefined)
*typeid_p = InvalidOid;
else
*typeid_p = HeapTupleGetOid(tup);
if (tup)
ReleaseSysCache(tup);
}
typenameTypeIdAndMod(...)
{
Type tup = typenameType(pstate, typeName, typmod_p);
*typeid_p = HeapTupleGetOid(tup);
ReleaseSysCache(tup);
}
Also, there's no need for else here:
if (raiseError)
ereport(ERROR, ...);
else
return InvalidOid;
Regards,
Marti
--
Yugo Nagata nag...@sraoss.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: variant of regclass
On Wed, 22 Jan 2014 20:04:12 +0900 (JST)
Tatsuo Ishii is...@postgresql.org wrote:
On Tue, Jan 14, 2014 at 9:28 AM, Yugo Nagata nag...@sraoss.co.jp wrote:
Here is the patch to implement to_regclass, to_regproc, to_regoper,
and to_regtype.
+ static Datum regclass_guts(char *class_name_or_oid, bool raiseError);
Minor bikeshedding, a lot of code currently uses an argument named
missing_ok for this purpose (with inverse meaning of course). Any
reasons why you chose raiseError instead?
Originally the proposal checks errors like syntactical one in addition
to missing objects. So I think raiseError was more appropriate at
that time. Now they only check missing objects. So renaming to
missing_ok could be more appropriate.
I only had a brief look at the patch, so maybe I'm missing something.
But I don't think you should create 3 variants of these functions:
* parseTypeString calls parseTypeString_guts with false
* parseTypeStringMissingOk calls parseTypeString_guts with true
* parseTypeString_guts
And this is just silly:
if (raiseError)
parseTypeString(typ_name_or_oid, result, typmod);
else
parseTypeStringMissingOk(typ_name_or_oid, result, typmod);
Just add an argument to parseTypeString and patch all the callers.
Leave the disccusion to Yugo..
parseTypeString() is called by some other functions and I avoided
influences of modifying the definition on them, since this should
raise errors in most cases. This is same reason for other *MissingOk
functions in parse_type.c.
Is it better to write definitions of these function and all there callers?
if requested object is not found,
returns InvalidOid, rather than raises an error.
I thought the consensus was that returning NULL is better than
InvalidOid? From an earlier message:
On Thu, Dec 5, 2013 at 5:25 PM, Robert Haas robertmh...@gmail.com wrote:
Another advantage of this approach is that, IIUC, type input functions
can't return a NULL value. So 'pg_klass'::regclass could return 0,
but not NULL. On the other hand, toregclass('pg_klass') *could*
return NULL, which seems conceptually cleaner.
Not sure. There's already at least one counter example:
pg_my_temp_schema() oid OID of session's temporary schema, or 0 if none
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp
--
Yugo Nagata nag...@sraoss.co.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 64-bit API for large object
Currently lo_initialize() throws an error if one of oids are not available. I doubt we do the same way for 64-bit functions since this will make 9.3 libpq unable to access large objects stored in pre-9.2 PostgreSQL servers. It seems to me the situation to split the case of pre-9.2 and post-9.3 using a condition of conn-sversion = 90300. Agreed. I'll fix it like that. 4) src/test/examples/testlo64.c added for 64-bit API example(Yugo Nagata) Comments and suggestions are welcome. miscellaneous comments are below. Regression test is helpful. Even though no need to try to create 4TB large object, it is helpful to write some chunks around the design boundary. Could you add some test cases that writes some chunks around 4TB offset. Agreed. I'll do that. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Yugo Nagata nag...@sraoss.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
