Re: Added schema level support for publication.
On Fri, Oct 22, 2021 at 8:56 PM vignesh C wrote: > I am getting a compilation error in the latest patch on HEAD. I think was relying on some variable removed by a recent commit 92316a4582a5714d4e494aaf90360860e7fec37a. While looking at that compilation error, I observed that we don't need the second and third parameters in pg_dump.c/getPublicationNamespaces() as those are not getting used. -- With Regards, Amit Kapila.
Re: prevent immature WAL streaming
On Mon, Oct 25, 2021 at 7:02 AM Kyotaro Horiguchi wrote: > > At Fri, 22 Oct 2021 18:43:52 +0530, Amul Sul wrote in > > Any thoughts about the patch posted previously? > > Honestly, xlogreader looks fine with the current shape. The reason is > that it seems cleaner as an interface boundary since the caller of > xlogreader doesn't need to know about the details of xlogreader. The > current code nicely hides the end+1 confusion. > > Even if we want to get rid of global variables in xlog.c, I don't > understand why we remove only abortedRecPtr. That change makes things > more complex as a whole by letting xlog.c be more conscious of > xlogreader's internals. I'm not sure I like that aspect of the patch. > Because we have other ways to get abortedRecPtr without having a global variable, but we don't have such a way for missingContrecPtr, AFAICU. I agree using global variables makes things a bit easier, but those are inefficient when you want to share those with other processes -- that would add extra burden to shared memory. Regards, Amul
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
On Mon, Oct 25, 2021 at 9:43 AM Kyotaro Horiguchi wrote: > > Do you think pg_signal_backend is the wrong group to allow usage of > > pg_log_backend_memory_contexts()? Alternatively, it could simply not > > Yes. I think it would be danger that who is allowed to dump memory > context into log files by granting pg_signal_backend also can > terminate other backends. > > > pg_log_backend_memory_contexts()? Alternatively, it could simply not > > GRANT anything, and leave that up to the administrator to choose who > > can use it. > > *I* prefer that. I'm not sure I'm the only one to think so, though.. How about we have a separate predefined role for the functions that deal with server logs? I'm not sure if Mark Dilger's patch on new predefined roles has one, if not, how about something like pg_write_server_log/pg_manage_server_log/some other name? If not with a new predefined role, how about expanding the scope of existing pg_write_server_files role? Regards, Bharath Rupireddy.
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
At Sun, 24 Oct 2021 20:31:37 -0700, Jeff Davis wrote in > On Mon, 2021-10-25 at 11:53 +0900, Kyotaro Horiguchi wrote: > > In other words, I don't think pg_signal_backends is not meant to > > control "log something on another session" or "rotate log file". > > It's > > danger that if we allow somewone to rotate log files, that means to > > allow same user to terminate another session. > > The current patch doesn't allow members of pg_signal_backend to rotate > the log file. Ah, sorry, I might have confused with some other discussion. > Do you think pg_signal_backend is the wrong group to allow usage of > pg_log_backend_memory_contexts()? Alternatively, it could simply not Yes. I think it would be danger that who is allowed to dump memory context into log files by granting pg_signal_backend also can terminate other backends. > pg_log_backend_memory_contexts()? Alternatively, it could simply not > GRANT anything, and leave that up to the administrator to choose who > can use it. *I* prefer that. I'm not sure I'm the only one to think so, though.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
On Mon, 2021-10-25 at 11:53 +0900, Kyotaro Horiguchi wrote: > In other words, I don't think pg_signal_backends is not meant to > control "log something on another session" or "rotate log file". > It's > danger that if we allow somewone to rotate log files, that means to > allow same user to terminate another session. The current patch doesn't allow members of pg_signal_backend to rotate the log file. Do you think pg_signal_backend is the wrong group to allow usage of pg_log_backend_memory_contexts()? Alternatively, it could simply not GRANT anything, and leave that up to the administrator to choose who can use it. Regards, Jeff Davis
Re: Added schema level support for publication.
On Fri, Oct 22, 2021 at 11:59 AM Greg Nancarrow wrote: > > On Fri, Oct 22, 2021 at 12:41 PM Greg Nancarrow wrote: > > > > I was also previously concerned about what the behavior should be when > > only including just the partitions of a partitioned table in a > > publication using ALL TABLES IN SCHEMA and having > > publish_via_partition_root=true. It seems to implicitly include the > > partitioned table in the publication. But I did some testing and found > > that this is the current behavior when only the partitions are > > individually included in a publication using TABLE, so it seems to be > > OK. > > > > Thinking some more about this, it still may still be confusing to the > user if not explicitly stated in the ALL TABLES IN SCHEMA case. > How about adding some additional explanation to the end of the > following paragraph: > > + > + When a partitioned table is published via schema level publication, all > + of its existing and future partitions irrespective of it being from the > + publication schema or not are implicitly considered to be part of the > + publication. So, even operations that are performed directly on a > + partition are also published via publications that its ancestors are > + part of. > + > > Something like: > > Similarly, if a partition is published via schema level publication > and publish_via_partition_root=true, the parent partitioned table is > implicitly considered to be part of the publication, irrespective of > it being from the publication schema or not. > I don't think we need to explicitly mention this in docs as this is quite similar to what is happening for the "For Table" case as well and it seems to be clarified by "publish_via_partition_root" definition in Create Publication docs [1]. [1] - https://www.postgresql.org/docs/devel/sql-createpublication.html -- With Regards, Amit Kapila.
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
At Sun, 24 Oct 2021 09:50:58 -0700, Jeff Davis wrote in > On Sat, 2021-10-23 at 20:42 +, Bossart, Nathan wrote: > > The predefined roles documentation notes > > that members of pg_signal_backend cannot signal superuser-owned > > backends, but AFAICT pg_log_backend_memory_contexts() has no such > > restriction at the moment. Should we add this? > > Added, good catch. > > > This is unrelated to this patch, but should we also consider opening > > up pg_reload_conf() and pg_rotate_logfile() to members of > > pg_signal_backend? Those are the other "server signaling functions" > > I > > see in the docs. > > Those are actually signalling the postmaster, not an ordinary backend. > Also, those functions are already GRANTable, so I think we should leave > them as-is. I'm afraid that it might be wrong that all backend-signalling features are allowed by that priviledge. pg_signal_backends is described in the doc as: https://www.postgresql.org/docs/devel/predefined-roles.html > Signal another backend to cancel a query or terminate its session. Here, the term "signal" there seems to mean interrupting something on that session or the session itself. Addition to that I don't think "terminate a session or the query on a session" and "log something on another session" and "rotate log file" don't fall into the same category in a severity view. In other words, I don't think pg_signal_backends is not meant to control "log something on another session" or "rotate log file". It's danger that if we allow somewone to rotate log files, that means to allow same user to terminate another session. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Skipping logical replication transactions on subscriber side
On Wed, Oct 6, 2021 at 11:18 AM Greg Nancarrow wrote: > > On Mon, Oct 4, 2021 at 4:31 PM Amit Kapila wrote: > > > > I think here the main point is that does this addresses Peter's > > concern for this Patch to use a separate syntax? Peter E., can you > > please confirm? Do let us know if you have something else going in > > your mind? > > > > Peter's concern seemed to be that the use of a subscription option, > though convenient, isn't an intuitive natural fit for providing this > feature (i.e. ability to skip a transaction by xid). I tend to have > that feeling about using a subscription option for this feature. I'm > not sure what possible alternative syntax he had in mind and currently > can't really think of a good one myself that fits the purpose. > > I think that the 1st and 2nd patch are useful in their own right, but > couldn't this feature (i.e. the 3rd patch) be provided instead as an > additional Replication Management function (see 9.27.6)? > e.g. pg_replication_skip_xid > After some thoughts on the syntax, it's somewhat natural to me if we support the skip transaction feature with another syntax like (I prefer the former): ALTER SUBSCRIPTION ... [SET|RESET] SKIP TRANSACTION xxx; or ALTER SUBSCRIPTION ... SKIP TRANSACTION xxx; (setting NONE as XID to reset the XID to skip) The primary reason to have another syntax is that ability to skip a transaction seems not to be other subscription parameters such as slot_name, binary, streaming that are dumped by pg_dump. FWIW IMO the ability to disable the subscription on an error would be a subscription parameter. The user is likely to want to specify this option also at CREATE SUBSCRIPTION and wants it to be dumped by pg_dump. So I think we can think of the skip xid option separately from this parameter. Also, I think we can think of the syntax for this ability (skipping a transaction) separately from the syntax of the general conflict resolution feature. I guess that we might rather need a whole new syntax for conflict resolution. In addition, the user will want to dump the definitions of confliction resolution by pg_dump in common cases, unlike the skip XID. As Amit pointed out, we might want to allow users to skip changes based on something other than XID but the candidates seem only a few to me (LSN, time, and something else?). If these are only a few, probably we don’t need to worry about syntax bloat. Regarding an additional replication management function proposed by Greg, it seems a bit unnatural to me; the subscription is created and altered by DDL but why is only skipping the transaction option specified by an SQL function? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: prevent immature WAL streaming
At Fri, 22 Oct 2021 18:43:52 +0530, Amul Sul wrote in > Any thoughts about the patch posted previously? Honestly, xlogreader looks fine with the current shape. The reason is that it seems cleaner as an interface boundary since the caller of xlogreader doesn't need to know about the details of xlogreader. The current code nicely hides the end+1 confusion. Even if we want to get rid of global variables in xlog.c, I don't understand why we remove only abortedRecPtr. That change makes things more complex as a whole by letting xlog.c be more conscious of xlogreader's internals. I'm not sure I like that aspect of the patch. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: add retry mechanism for achieving recovery target before emitting FATA error "recovery ended before configured recovery target was reached"
At Wed, 20 Oct 2021 21:35:44 +0530, Bharath Rupireddy wrote in > Hi, > > The FATAL error "recovery ended before configured recovery target was > reached" introduced by commit at [1] in PG 14 is causing the standby > to go down after having spent a good amount of time in recovery. There > can be cases where the arrival of required WAL (for reaching recovery > target) from the archive location to the standby may take time and > meanwhile the standby failing with the FATAL error isn't good. > Instead, how about we make the standby wait for a certain amount of > time (with a GUC) so that it can keep looking for the required WAL. If > it gets the required WAL during the wait time, then it succeeds in > reaching the recovery target (no FATAL error of course). If it > doesn't, the timeout occurs and the standby fails with the FATAL > error. The value of the new GUC can probably be set to the average > time it takes for the WAL to reach archive location from the primary + > from archive location to the standby, default 0 i.e. disabled. > > I'm attaching a WIP patch. I've tested it on my dev system and the > recovery regression tests are passing with it. I will provide a better > version later, probably with a test case. > > Thoughts? It looks like starting a server in non-hot standby mode only fetching from archive. The only difference is it doesn't have timeout. Doesn't that cofiguration meet your requirements? Or, if timeout matters, I agree with Jeff. Retrying in restore_command looks fine. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Assorted improvements in pg_dump
Justin Pryzby writes:
> On Sun, Oct 24, 2021 at 05:10:55PM -0400, Tom Lane wrote:
>> + if (tbloids->len > 1)
> I think this should say
> + if (tbloids->len > 0)
No, >1 is the correct test, because it's checking the string length
and we started by stuffing a '{' into the string. Maybe needs a
comment.
> BTW, the ACL patch makes the overhead 6x lower (6.9sec vs 1.2sec) for pg_dump
> -t
> of a single, small table. Thanks for that.
Yeah --- I haven't done any formal measurements of the case where you're
selecting a small number of tables, but I did note that it decreased a
good deal compared to HEAD.
regards, tom lane
Re: pg_receivewal starting position
On Sun, Oct 24, 2021 at 07:20:57PM +0530, Bharath Rupireddy wrote: > Thanks. I've no further comments on the v10 patch. Okay, thanks. I have applied this part, then. -- Michael signature.asc Description: PGP signature
Re: Assorted improvements in pg_dump
On Sun, Oct 24, 2021 at 05:10:55PM -0400, Tom Lane wrote: > 0003 is the same except I added a missing free(). > > 0004 is a new patch based on an idea from Andres Freund [1]: > in the functions that repetitively issue the same query against > different tables, issue just one query and use a WHERE clause > to restrict the output to the tables we care about. I was > skeptical about this to start with, but it turns out to be > quite a spectacular win. On my machine, the time to pg_dump > the regression database (with "-s") drops from 0.91 seconds > to 0.39 seconds. For a database with 1 toy tables, the > time drops from 18.1 seconds to 2.3 seconds. + if (tbloids->len > 1) + appendPQExpBufferChar(tbloids, ','); + appendPQExpBuffer(tbloids, "%u", tbinfo->dobj.catId.oid); I think this should say + if (tbloids->len > 0) That doesn't matter much since catalogs aren't dumped as such, and we tend to count in base 10 and not base 1. BTW, the ACL patch makes the overhead 6x lower (6.9sec vs 1.2sec) for pg_dump -t of a single, small table. Thanks for that. -- Justin
Re: pg_dump versus ancient server versions
On 2021-Oct-24, Robert Haas wrote: > You know, one thing we could think about doing is patching some of the > older branches to make them compile on modern machines. That would not > only be potentially useful for people who are upgrading from ancient > versions, but also for hackers trying to do research on the origin of > bugs or performance problems, and also for people who are trying to > maintain some kind of backward compatibility or other and want to test > against old versions. I think it is worth *some* effort, at least as far back as we want to claim that we maintain pg_dump and/or psql compatibility, assuming it is not too onerous. For instance, I wouldn't want to clutter buildfarm or CI dashboards with testing these branches, unless it is well isolated from regular ones; we shouldn't commit anything that's too invasive; and we shouldn't make any claims about supportability of these abandoned branches. As an example, I did backpatch one such fix to 8.3 (just over a year) and 8.2 (four years) after they had closed -- see d13f41d21538 and 105f3ef492ab. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"
Re: pg_dump versus ancient server versions
Robert Haas writes: > You know, one thing we could think about doing is patching some of the > older branches to make them compile on modern machines. That would not > only be potentially useful for people who are upgrading from ancient > versions, but also for hackers trying to do research on the origin of > bugs or performance problems, and also for people who are trying to > maintain some kind of backward compatibility or other and want to test > against old versions. Yeah. We have done that in the past; I thought more than once, but right now the only case I can find is d13f41d21/105f3ef49. There are some other post-EOL commits in git, but I think the others were mistakes from over-enthusiastic back-patching, while that one was definitely an intentional portability fix for EOL'd versions. > I don't know whether that's really worth the effort and I expect Tom > will say that it's not. If he does say ,that, he may be right. Hmm ... I guess the question is how much work we feel like putting into that, and how we'd track whether old branches still work, and on what platforms. It could easily turn into a time sink that's not justified by the value. I do see your point that there's some value in it; I'm just not sure about the cost/benefit ratio. One thing we could do that would help circumscribe the costs is to say "we are not going to consider issues involving new compiler warnings or bugs caused by more-aggressive optimization". We could mechanize that pretty effectively by changing configure shortly after a branch's EOL to select -O0 and no extra warning flags, so that anyone building from branch tip would get those switch choices. (I have no idea what this might look like on the Windows side, but I'm concerned by the fact that we seem to need fixes every time a new Visual Studio major version comes out.) regards, tom lane
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On 10/24/21, 10:20 AM, "Jeff Davis" wrote: > On Sun, 2021-10-24 at 20:19 +0530, Bharath Rupireddy wrote: >> Are there any other database activities that fall under the >> "maintenance" category? How about CLUSTER, REINDEX? I didn't check >> the >> code for their permissions. > > I looked around and didn't see much else to fit into this category. > CLUSTER and REINDEX are a little too specific for a generic maintenance > operation -- it's unlikely that you'd want to perform those expensive > operations just to tidy up. But if you think something else should fit, > let me know. My initial reaction was that members of pg_maintenance should be able to do all of these things (VACUUM, ANALYZE, CLUSTER, REINDEX, and CHECKPOINT). It's true that some of these are more expensive or disruptive than others, but how else could we divvy it up? Maybe one option is to have two separate roles, one for commands that require lower lock levels (i.e., ANALYZE and VACUUM without TRUNCATE and FULL), and another for all of the maintenance commands. Nathan
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
On 10/24/21, 9:51 AM, "Jeff Davis" wrote: > On Sat, 2021-10-23 at 20:42 +, Bossart, Nathan wrote: >> The predefined roles documentation notes >> that members of pg_signal_backend cannot signal superuser-owned >> backends, but AFAICT pg_log_backend_memory_contexts() has no such >> restriction at the moment. Should we add this? > > Added, good catch. The new patch looks good to me. Nathan
Re: pg_dump versus ancient server versions
On Fri, Oct 22, 2021 at 7:51 PM Alvaro Herrera wrote: > I just tried to build 9.1. My config line there doesn't have ssl, but I > do get this in the compile stage: Hmm. You know, one thing we could think about doing is patching some of the older branches to make them compile on modern machines. That would not only be potentially useful for people who are upgrading from ancient versions, but also for hackers trying to do research on the origin of bugs or performance problems, and also for people who are trying to maintain some kind of backward compatibility or other and want to test against old versions. I don't know whether that's really worth the effort and I expect Tom will say that it's not. If he does say that, he may be right. But I think if I were trying to extract my data from an old 7.4 database, I think I'd find it a lot more useful if I could make 9.0 or 9.2 or something compile and talk to it than if I had to use v15 and hope that held together somehow. It doesn't really make sense to try to keep compatibility of any sort with versions we can no longer test against. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Sun, 2021-10-24 at 20:19 +0530, Bharath Rupireddy wrote: > At this point, the idea of having a new role for maintenance work > looks good. With this patch and Mark Dilger's patch introducing a > bunch of new predefined roles, one concern is that we might reach to > a > state where we will have patches being proposed for new predefined > roles for every database activity and the superuser eventually will > have nothing to do in the database, it just becomes dummy? The idea is that, in different environments, the notion of an "administrator" should have different capabilities and different risks. By making the privileges more fine-grained, we enable those different use cases. I don't see it as necessarily a problem if superuser doesn't have much left to do. > I'm not sure if Mark Dilger's patch on new predefined roles has a > suitable/same role that we can use here. I didn't see one. I think one of the most common reasons to do manual checkpoints and vacuums is for performance testing, so another potential name might be "pg_performance". But "pg_maintenance" seemed a slightly better fit. > Are there any other database activities that fall under the > "maintenance" category? How about CLUSTER, REINDEX? I didn't check > the > code for their permissions. I looked around and didn't see much else to fit into this category. CLUSTER and REINDEX are a little too specific for a generic maintenance operation -- it's unlikely that you'd want to perform those expensive operations just to tidy up. But if you think something else should fit, let me know. Thank you, Jeff Davis
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
On Sun, 2021-10-24 at 19:58 +0530, Bharath Rupireddy wrote: > It looks like we are better off with removing explicit superuser() > checks from the functions and using normal GRANT based system, see > others agreeing on this at [1]. As we have lots of functions that are > doing explicit superuser() checks, I'm sure someday they all will > have > to be moved to GRANT system. Note that some functions have additional checks that can't be expressed with GRANT -- see pg_cancel_backend(), for example. But I agree in general that GRANT is the way to go most of the time. > The current code is a mix - some > functions do explicit checks (I've seen many of them with the comment > at [2]) and others do it via GRANT system. I'm not saying that we > should be dealing with those here in this thread, all I'm looking for > is that we have a note of it in the postgres todo list in the wiki so > that someone interested can pick that work up. Thoughts? It seems like there's agreement on the direction, but I don't know that there's a good place to write it down. Probably better to just fix as many of the functions as we can, and then when people add new ones, they'll copy the GRANT pattern rather than the explicit superuser check. > Comments on the patch: > 1) testrole1 and testrole2 look generic, how about Michael had a similar comment. Renamed, thank you. > 2) It seems like the privileges.sql is the right place to place the > test cases, but I'm fine with keeping all the test cases of the > function together. If we add all the function privilege checks there, I think it will overwhelm the other interesting tests happening in that file. > 3) It might be enough to do has_function_privilege, just a thought - > isn't it better if we execute the function with the test roles set > in. > This will help us catch the permission denied error message in the > test output files. Missed this comment. I'll tweak this before commit. > 4) Isn't the +#define CATALOG_VERSION_NO 202110230 going to be set to > the date on which the patch gets committed? I just put it in there so that I wouldn't forget, but I'll update it at commit time. > 5) The following change is being handled in the patch at [3], I know > it is appropriate to have it in this patch, but please mention it in > the commit message on why we do this change. I will remove this > change > from my patch at [3]. > -SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid()); > +SELECT pg_log_backend_memory_contexts(pg_backend_pid()); What would you like me to mention? Regards, Jeff Davis
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
On Sat, 2021-10-23 at 20:42 +, Bossart, Nathan wrote:
> The predefined roles documentation notes
> that members of pg_signal_backend cannot signal superuser-owned
> backends, but AFAICT pg_log_backend_memory_contexts() has no such
> restriction at the moment. Should we add this?
Added, good catch.
> This is unrelated to this patch, but should we also consider opening
> up pg_reload_conf() and pg_rotate_logfile() to members of
> pg_signal_backend? Those are the other "server signaling functions"
> I
> see in the docs.
Those are actually signalling the postmaster, not an ordinary backend.
Also, those functions are already GRANTable, so I think we should leave
them as-is.
Regards,
Jeff Davis
From 3be296819d6195cbafce72ab085c8430f1e8a502 Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Fri, 22 Oct 2021 13:40:35 -0700
Subject: [PATCH] Allow pg_signal_backend members to call
pg_log_backend_memory_contexts().
Discussion: https://postgr.es/m/e5cf6684d17c8d1ef4904ae248605ccd6da03e72.ca...@j-davis.com
---
doc/src/sgml/func.sgml | 5 +++--
src/backend/catalog/system_functions.sql | 5 +
src/backend/utils/adt/mcxtfuncs.c| 20 -
src/include/catalog/catversion.h | 2 +-
src/test/regress/expected/misc_functions.out | 23 ++--
src/test/regress/sql/misc_functions.sql | 15 +++--
6 files changed, 53 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5677032cb28..ee8903fe484 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25332,8 +25332,9 @@ SELECT collation for ('foo' COLLATE "de_DE");
(See for more information),
but will not be sent to the client regardless of
.
-Only superusers can request to log the memory contexts.
-
+This is also allowed if the calling role has been granted
+pg_signal_backend, however only superusers can
+cancel superuser backends.
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index a416e94d371..2cd5e3fe17b 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -699,6 +699,8 @@ REVOKE EXECUTE ON FUNCTION pg_ls_dir(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_ls_dir(text,boolean,boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer) FROM PUBLIC;
+
--
-- We also set up some things as accessible to standard roles.
--
@@ -713,6 +715,9 @@ GRANT EXECUTE ON FUNCTION pg_ls_tmpdir() TO pg_monitor;
GRANT EXECUTE ON FUNCTION pg_ls_tmpdir(oid) TO pg_monitor;
+GRANT EXECUTE ON FUNCTION pg_log_backend_memory_contexts(integer)
+ TO pg_signal_backend;
+
GRANT pg_read_all_settings TO pg_monitor;
GRANT pg_read_all_stats TO pg_monitor;
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 0d52613bc32..a2f45fb8578 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -162,10 +162,10 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
* pg_log_backend_memory_contexts
* Signal a backend process to log its memory contexts.
*
- * Only superusers are allowed to signal to log the memory contexts
- * because allowing any users to issue this request at an unbounded
- * rate would cause lots of log messages and which can lead to
- * denial of service.
+ * Only members of pg_signal_backend are allowed to signal to log the memory
+ * contexts because allowing any users to issue this request at an unbounded
+ * rate would cause lots of log messages and which can lead to denial of
+ * service.
*
* On receipt of this signal, a backend sets the flag in the signal
* handler, which causes the next CHECK_FOR_INTERRUPTS() to log the
@@ -177,12 +177,6 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
int pid = PG_GETARG_INT32(0);
PGPROC *proc;
- /* Only allow superusers to log memory contexts. */
- if (!superuser())
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be a superuser to log memory contexts")));
-
proc = BackendPidGetProc(pid);
/*
@@ -205,6 +199,12 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(false);
}
+ /* Only allow superusers to signal superuser-owned backends. */
+ if (superuser_arg(proc->roleId) && !superuser())
+ ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be a superuser to log memory contexts of superuser backend")));
+
if (SendProcSignal(pid, PROCSIG_LOG_MEMORY_CONTEXT, proc->backendId) < 0)
{
/* Again, just a warning to allow loops */
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3253b8751b1..039f6338604 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* mmddN */
-#define
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Sun, Oct 24, 2021 at 7:49 AM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > On Sun, Oct 24, 2021 at 3:15 AM Jeff Davis wrote: > > > > Add new predefined role pg_maintenance, which can issue VACUUM, > > ANALYZE, CHECKPOINT. > > > Are there any other database activities that fall under the > "maintenance" category? How about CLUSTER, REINDEX? I didn't check the > code for their permissions. > > I would not lump the I/O intensive cluster and reindexing commands, and vacuum full, into the same permission bucket as vacuum and analyze. Checkpoint fits in the middle of that continuum. However, given that both vacuum and analyze are run to ensure good planner statistics during normal usage of the database, while the others, including checkpoint, either are non-normal usage or don't influence the planner, I would shift checkpoint to the same permission that covers cluster and reindex. David J.
Re: Predefined role pg_maintenance for VACUUM, ANALYZE, CHECKPOINT.
On Sun, Oct 24, 2021 at 3:15 AM Jeff Davis wrote: > > Add new predefined role pg_maintenance, which can issue VACUUM, > ANALYZE, CHECKPOINT. > > Patch attached. At this point, the idea of having a new role for maintenance work looks good. With this patch and Mark Dilger's patch introducing a bunch of new predefined roles, one concern is that we might reach to a state where we will have patches being proposed for new predefined roles for every database activity and the superuser eventually will have nothing to do in the database, it just becomes dummy? I'm not sure if Mark Dilger's patch on new predefined roles has a suitable/same role that we can use here. Are there any other database activities that fall under the "maintenance" category? How about CLUSTER, REINDEX? I didn't check the code for their permissions. Regards, Bharath Rupireddy.
Re: Postgres perl module namespace
On 10/20/21 08:40, Andrew Dunstan wrote:
> On 10/19/21 11:22 PM, Michael Paquier wrote:
>> On Tue, Oct 19, 2021 at 10:16:06PM +0200, Erik Rijkers wrote:
[0001-move-perl-test-modules-to-PostgreSQL-Test-namespace.patch ]
[0002-move-PostgreSQL-Test-PostgresVersion-up-in-the-names.patch]
>> It seems to me that the hardest part is sorted out with the naming and
>> pathing of the modules, so better to apply them sooner than later.
>
> Yeah, my plan is to apply it today or tomorrow
>
>
>>> Those patches gave some complains about PostgreSQL/Test/PostgresVersion.pm
>>> being absent so I added this deletion. I'm not sure that's correct but it
>>> enabled the build and check-world ran without errors.
>> Your change is incorrect, as we want to install PostgresVersion.pm.
>> What's needed here is the following:
>> {PostgresVersion.pm => PostgreSQL/Version.pm}
>>
>> And so the patch needs to be changed like that:
>> - $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/PostgresVersion.pm
>> '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm'
>> + $(INSTALL_DATA) $(srcdir)/PostgreSQL/Version.pm
>> '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
>> [...]
>> - rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm'
>> + rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
> right. There are one or two other cosmetic changes too.
>
>
... and pushed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Allow pg_signal_backend members to use pg_log_backend_memory_stats().
On Sun, Oct 24, 2021 at 1:27 AM Jeff Davis wrote: > > > Simple patch to implement $SUBJECT attached. > > pg_signal_backend seems like the appropriate predefined role, because > pg_log_backend_memory_contexts() is implemented by a sending signal. +1. It looks like we are better off with removing explicit superuser() checks from the functions and using normal GRANT based system, see others agreeing on this at [1]. As we have lots of functions that are doing explicit superuser() checks, I'm sure someday they all will have to be moved to GRANT system. The current code is a mix - some functions do explicit checks (I've seen many of them with the comment at [2]) and others do it via GRANT system. I'm not saying that we should be dealing with those here in this thread, all I'm looking for is that we have a note of it in the postgres todo list in the wiki so that someone interested can pick that work up. Thoughts? Comments on the patch: 1) testrole1 and testrole2 look generic, how about regress_mcxt_role1/2? There's no problem as they are misc_functions.sql local, but still role names can be more readable. +CREATE ROLE testrole1 IN ROLE pg_signal_backend; +CREATE ROLE testrole2; 2) It seems like the privileges.sql is the right place to place the test cases, but I'm fine with keeping all the test cases of the function together. 3) It might be enough to do has_function_privilege, just a thought - isn't it better if we execute the function with the test roles set in. This will help us catch the permission denied error message in the test output files. 4) Isn't the +#define CATALOG_VERSION_NO 202110230 going to be set to the date on which the patch gets committed? 5) The following change is being handled in the patch at [3], I know it is appropriate to have it in this patch, but please mention it in the commit message on why we do this change. I will remove this change from my patch at [3]. -SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid()); +SELECT pg_log_backend_memory_contexts(pg_backend_pid()); [1] - https://www.postgresql.org/message-id/CAOuzzgpp0dmOFjWC4JDvk57ZQGm8umCrFdR1at4b80xuF0XChw%40mail.gmail.com [2] - * Permission checking for this function is managed through the normal * GRANT system. */ [3] - https://www.postgresql.org/message-id/CALj2ACVXk1roswqFpiCOMHrsB%2BxxW7HG536krGAzF%3DmWXh3eWQ%40mail.gmail.com Regards, Bharath Rupireddy.
Re: pg_receivewal starting position
On Sun, Oct 24, 2021 at 12:21 PM Michael Paquier wrote: > > On Sun, Oct 24, 2021 at 09:08:01AM +0530, Bharath Rupireddy wrote: > > pg_get_replication_slots holds the ReplicationSlotControlLock until > > the end of the function so it can be assured that *slot contents will > > not change. In ReadReplicationSlot, the ReplicationSlotControlLock is > > released immediately after taking *slot pointer into slot_contents. > > Isn't it better if we hold the lock until the end of the function so > > that we can avoid the slot contents becoming stale problems? > > The reason is different in the case of pg_get_replication_slots(). We > have to hold ReplicationSlotControlLock for the whole duration of the > shared memory scan to return back to the user a consistent set of > information to the user, for all the slots. Thanks. I've no further comments on the v10 patch. Regards, Bharath Rupireddy.
Re: [PATCH] Make ENOSPC not fatal in semaphore creation
On Mon, Oct 18, 2021 at 10:07 AM Thomas Munro wrote:
> Note: The best kind would be *unnamed* POSIX semas, where we get to
> control their placement in existing memory; that's what we do on Linux
> and FreeBSD. They weren't supported on OpenBSD last time we checked:
> it rejects requests for shared ones. I wonder if someone could
> implement them with just a few lines of user space code, using atomic
> counters and futex() for waiting.
I meant that it'd be cool if OpenBSD implemented shared memory unnamed
semas that way (as other OSes do), but just for fun I tried
implementing that in PostgreSQL. I already had a patch to provide a
wrapper API for futexes on a bunch of OSes including OpenBSD (because
I've been looking into ways to rewrite lwlock.c to use futexes
directly and skip all the per-backend semaphore stuff). That made it
easy to write a quick-and-dirty clone of sem_{init,wait,post}() using
atomics and futexes.
Sadly, although the attached proof-of-concept patch allows a
PREFERRED_SEMAPHORES=FUTEX build to pass tests on macOS (which also
lacks native unnamed semas), FreeBSD and Linux (which don't need this
but are interesting to test), and it also works on OpenBSD with
shared_memory_type=sysv, it doesn't work on OpenBSD with
shared_memory_type=mmap (the default). I suspect OpenBSD's futex(2)
has a bug: inherited anonymous shared mmap memory seems to confuse it
so that wakeups are lost. Arrrgh!
From 38bb61aeeefc24cdc772474b0fb4f1802d3b7577 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Sun, 24 Oct 2021 21:48:26 +1300
Subject: [PATCH 1/2] A basic API for futexes.
A thin wrapper for basic 32 bit futex wait and wake. Currently, it maps
to native support on Linux, FreeBSD, OpenBSD and macOS, with detection
via configure. More operating systems and more operations are possible.
---
configure | 4 +-
configure.ac| 4 ++
src/include/pg_config.h.in | 12
src/include/port/pg_futex.h | 139
4 files changed, 157 insertions(+), 2 deletions(-)
create mode 100644 src/include/port/pg_futex.h
diff --git a/configure b/configure
index 4ffefe4655..23f1cbe9d0 100755
--- a/configure
+++ b/configure
@@ -13381,7 +13381,7 @@ $as_echo "#define HAVE_STDBOOL_H 1" >>confdefs.h
fi
-for ac_header in atomic.h copyfile.h execinfo.h getopt.h ifaddrs.h langinfo.h mbarrier.h poll.h sys/epoll.h sys/event.h sys/ipc.h sys/prctl.h sys/procctl.h sys/pstat.h sys/resource.h sys/select.h sys/sem.h sys/shm.h sys/sockio.h sys/tas.h sys/uio.h sys/un.h termios.h ucred.h wctype.h
+for ac_header in atomic.h copyfile.h execinfo.h getopt.h ifaddrs.h langinfo.h linux/futex.h mbarrier.h poll.h sys/epoll.h sys/event.h sys/futex.h sys/ipc.h sys/prctl.h sys/procctl.h sys/pstat.h sys/resource.h sys/select.h sys/sem.h sys/shm.h sys/sockio.h sys/tas.h sys/uio.h sys/umtx.h sys/un.h termios.h ucred.h wctype.h
do :
as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
ac_fn_c_check_header_mongrel "$LINENO" "$ac_header" "$as_ac_Header" "$ac_includes_default"
@@ -15492,7 +15492,7 @@ fi
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
-for ac_func in backtrace_symbols clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit kqueue mbstowcs_l memset_s poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink readv setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink syncfs sync_file_range uselocale wcstombs_l writev
+for ac_func in __ulock_wait backtrace_symbols clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit kqueue mbstowcs_l memset_s poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink readv setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink syncfs sync_file_range uselocale wcstombs_l writev
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.ac b/configure.ac
index 44ee3ebe2f..542b46437e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1363,10 +1363,12 @@ AC_CHECK_HEADERS(m4_normalize([
getopt.h
ifaddrs.h
langinfo.h
+ linux/futex.h
mbarrier.h
poll.h
sys/epoll.h
sys/event.h
+ sys/futex.h
sys/ipc.h
sys/prctl.h
sys/procctl.h
@@ -1378,6 +1380,7 @@ AC_CHECK_HEADERS(m4_normalize([
sys/sockio.h
sys/tas.h
sys/uio.h
+ sys/umtx.h
sys/un.h
termios.h
ucred.h
@@ -1698,6 +1701,7 @@ LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
AC_CHECK_FUNCS(m4_normalize([
+ __ulock_wait
backtrace_symbols
clock_gettime
copyfile
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 15ffdd895a..6bd2f7b5d8 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -367,6 +367,9 @@
/* Define to 1 if you have the `link' function. */
#undef HAVE_LINK
+/* Define to 1 if you have the header file. */
+#undef
Re: Proposal: allow database-specific role memberships
Hi all, Thank you for the feedback so far! Attached is a completed implementation (including tests and documentation). Based on the feedback I have received so far, I will be submitting this implementation to the commitfest. Thanks again, Kenaniah On Mon, Oct 11, 2021 at 9:05 AM Stephen Frost wrote: > Greetings, > > * David G. Johnston (david.g.johns...@gmail.com) wrote: > > On Monday, October 11, 2021, Stephen Frost wrote: > > > I don't think "just don't grant access to those other databases" > > > is actually a proper answer- there is certainly a use-case for "I want > > > user X to have read access to all tables in *this* database, and also > > > allow them to connect to some other database but not have that same > > > level of access there." > > > > Sure, that has a benefit. But creating a second user for the other > > database and putting the onus on the user to use the correct credentials > > when logging into a particular database is a valid option - it is in > fact > > the status quo. Due to the complexity of adding a whole new grant > > dimension to the system the status quo is an appealing option. Annoyance > > factor aside it technically solves the per-database permissions problem > put > > forth. > > I disagree entirely that forcing users to have multiple accounts and to > deal with "using the correct one" is at all reasonable. That's an utter > hack that results in a given user having multiple different accounts- > something that gets really ugly to deal with in enterprise deployments > which use any kind of centralized authentication system. > > No, that's not a solution. Perhaps there's another way to implement > this capability that is simpler than what's proposed here, but saying > "just give each user two accounts" isn't a solution. Sure, it'll work > for existing released versions of PG, just like there's a lot of things > that people can do to hack around our deficiencies, but that doesn't > change that these are areas which we are lacking and where we should be > trying to provide a proper solution. > > Thanks, > > Stephen > database-role-memberships-v2.patch Description: Binary data
Re: pg_receivewal starting position
On Sun, Oct 24, 2021 at 09:08:01AM +0530, Bharath Rupireddy wrote: > pg_get_replication_slots holds the ReplicationSlotControlLock until > the end of the function so it can be assured that *slot contents will > not change. In ReadReplicationSlot, the ReplicationSlotControlLock is > released immediately after taking *slot pointer into slot_contents. > Isn't it better if we hold the lock until the end of the function so > that we can avoid the slot contents becoming stale problems? The reason is different in the case of pg_get_replication_slots(). We have to hold ReplicationSlotControlLock for the whole duration of the shared memory scan to return back to the user a consistent set of information to the user, for all the slots. -- Michael signature.asc Description: PGP signature
