public inbox for [email protected]
help / color / mirror / Atom feedSIGDANGER and oomd
10+ messages / 5 participants
[nested] [flat]
* SIGDANGER and oomd
@ 2018-09-19 23:51 Thomas Munro <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Munro @ 2018-09-19 23:51 UTC (permalink / raw)
To: pgsql-hackers
Hello hackers,
On AIX, which I think might have been a pioneer in overcommitting
memory (?), they have the wonderfully named signal SIGDANGER. It's
delivered to every process on the system as a final courtesy some time
before the OOM killer starts delivering SIGKILL. The theory is that
you might be able to give back memory to avoid the carnage.
Most (all?) popular operating systems overcommit memory, and from time
to time someone proposes that they should have SIGDANGER too. I've
often wondered if it makes sense or not as a concept: although you
might manage to avoid getting killed for a while, is your access
pattern sustainable? On the other hand, maybe if you're less afraid
of the OOM you could use resources better. I'm not sure, but perhaps
it's just too blunt an instrument. Certainly it would be possible for
PostgreSQL to respond to SIGDANGER by dropping caches and perhaps even
old idle backends via a CHECK_FOR_INTERRUPTS() handler, and combined
with log monitoring so that you know you have a serious problem and
need to consider making changes so it doesn't happen again, I think
over all it's probably a good thing to support, because your system
might actually survive the storm. This question was somewhat
hypothetical, because Linux doesn't have it, and I don't expect we
have too many AIX users. Neither does FreeBSD, though I've been
tempted to propose it there.
Recently Facebook published oomd[1][2] for Linux. I haven't tried it
(for one thing you currently need a patched kernel which is a barrier
to casual investigation) but basically it's a better OOM mousetrap
that runs in userspace. It could be interesting for PostgreSQL
installations, because you can configure it to be smarter about what
to kill, and you can teach it to give you feedback and warnings about
pressure. It may be that no modification to PostgreSQL is required at
all to make good use of this, or it may be that you'd want a new
signalling mechanism that we'd have to provide, amounting to a
home-made equivalent of SIGDANGER (or something richer).
I don't have a specific proposal, but thought this was newsworthy and
might give someone some ideas.
[1] https://github.com/facebookincubator/oomd
[2] https://code.fb.com/production-engineering/open-sourcing-oomd-a-new-approach-to-handling-ooms/
--
Thomas Munro
http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH 01/10] Define macros to make XLogReadRecord a state machine
@ 2019-04-18 01:22 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2019-04-18 01:22 UTC (permalink / raw)
To minimize apparent imapct on code, use some macros as syntax sugar. This is a similar stuff with ExecInterpExpr but a bit different. The most significant difference is that this stuff allows some functions are leaved midst of their work then continue. Roughly speaking this is used as the follows.
enum retval
some_func()
{
static .. internal_variables;
XLR_SWITCH();
...
XLR_LEAVE(STATUS1, RETVAL_CONTINUE);
...
XLR_LEAVE(STATUS2, RETVAL_CONTINUE2);
...
XLR_SWITCH_END();
XLR_RETURN(RETVAL_FINISH);
}
The caller uses the function as follows:
while (some_func() != RETVAL_FINISH)
{
<do some work>;
}
---
src/backend/access/transam/xlogreader.c | 53 +++++++++++++++++++++++++++++++++
src/include/access/xlogreader.h | 3 ++
2 files changed, 56 insertions(+)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 9196aa3aae..0e49ea6ab7 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -29,6 +29,59 @@
#include "utils/memutils.h"
#endif
+/*
+ * Use computed-goto-based state dispatch when computed gotos are available.
+ * But use a separate symbol so that it's easy to adjust locally in this file
+ * for development and testing.
+ */
+#ifdef HAVE_COMPUTED_GOTO
+#define XLR_USE_COMPUTED_GOTO
+#endif /* HAVE_COMPUTED_GOTO */
+
+/*
+ * Macros for state dispatch.
+ *
+ * XLR_SWITCH - just hides the switch if not in use.
+ * XLR_CASE - labels the implementation of named state.
+ * XLR_LEAVE - leave the function and return here at the next call.
+ * XLR_RETURN - return from the function and set state to initial state.
+ * XLR_END - just hides the closing brace if not in use.
+ */
+#if defined(XLR_USE_COMPUTED_GOTO)
+#define XLR_SWITCH() \
+ do { \
+ if ((XLR_STATE).j) \
+ goto *((void *) (XLR_STATE).j); \
+ XLR_CASE(XLR_INIT_STATE); \
+ } while (0)
+#define XLR_CASE(name) name:
+#define XLR_LEAVE(name, code) do { \
+ (XLR_STATE).j = (&&name); return (code); \
+ XLR_CASE(name); \
+ } while (0)
+#define XLR_RETURN(code) \
+ do { \
+ (XLR_STATE).j = (&&XLR_INIT_STATE); return (code); \
+ } while (0)
+#define XLR_SWITCH_END()
+#else /* !XLR_USE_COMPUTED_GOTO */
+#define XLR_SWITCH() \
+ /* Don't call duplicatedly */ \
+ switch ((XLR_STATE).c) { \
+ XLR_CASE(XLR_INIT_STATE); \
+#define XLR_CASE(name) case name:
+#define XLR_LEAVE(name, code) \
+ do { \
+ (XLR_STATE).c = (name); return (code); \
+ XLR_CASE(name); \
+ } while (0)
+#define XLR_RETURN(code) \
+ do { \
+ (XLR_STATE).c = (XLR_INIT_STATE); return (code); \
+ } while (0)
+#define XLR_SWITCH_END() }
+#endif /* XLR_USE_COMPUTED_GOTO */
+
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index f3bae0bf49..30500c35c7 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -240,6 +240,9 @@ extern bool DecodeXLogRecord(XLogReaderState *state, XLogRecord *record,
#define XLogRecBlockImageApply(decoder, block_id) \
((decoder)->blocks[block_id].apply_image)
+/* Reset the reader state */
+#define XLREAD_RESET(state) ((state)->xlnd_state.j = (state)->xlread_state.j = 0)
+
extern bool RestoreBlockImage(XLogReaderState *recoder, uint8 block_id, char *dst);
extern char *XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len);
extern bool XLogRecGetBlockTag(XLogReaderState *record, uint8 block_id,
--
2.16.3
----Next_Part(Fri_Apr_26_17_40_34_2019_888)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v2-0002-Make-ReadPageInternal-a-state-machine.patch"
^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH 01/10] Define macros to make XLogReadRecord a state machine
@ 2019-04-18 01:22 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2019-04-18 01:22 UTC (permalink / raw)
To minimize apparent imapct on code, use some macros as syntax sugar. This is a similar stuff with ExecInterpExpr but a bit different. The most significant difference is that this stuff allows some functions are leaved midst of their work then continue. Roughly speaking this is used as the follows.
enum retval
some_func()
{
static .. internal_variables;
XLR_SWITCH();
...
XLR_LEAVE(STATUS1, RETVAL_CONTINUE);
...
XLR_LEAVE(STATUS2, RETVAL_CONTINUE2);
...
XLR_SWITCH_END();
XLR_RETURN(RETVAL_FINISH);
}
The caller uses the function as follows:
while (some_func() != RETVAL_FINISH)
{
<do some work>;
}
---
src/backend/access/transam/xlogreader.c | 63 +++++++++++++++++++++++++++++++++
src/include/access/xlogreader.h | 3 ++
2 files changed, 66 insertions(+)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 9196aa3aae..5299765040 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -29,6 +29,69 @@
#include "utils/memutils.h"
#endif
+/*
+ * Use computed-goto-based opcode dispatch when computed gotos are available.
+ * But use a separate symbol so that it's easy to adjust locally in this file
+ * for development and testing.
+ */
+#ifdef HAVE_COMPUTED_GOTO
+#define XLR_USE_COMPUTED_GOTO
+#endif /* HAVE_COMPUTED_GOTO */
+
+/*
+ * Macros for opcode dispatch.
+ *
+ * XLR_SWITCH - just hides the switch if not in use.
+ * XLR_CASE - labels the implementation of named expression step type.
+ * XLR_DISPATCH - jump to the implementation of the step type for 'op'.
+ * XLR_LEAVE - leave the function and return here at the next call.
+ * XLR_RETURN - return from the function and set state to initial state.
+ * XLR_END - just hides the closing brace if not in use.
+ */
+#if defined(XLR_USE_COMPUTED_GOTO)
+#define XLR_SWITCH() \
+ /* Don't call duplicatedly */ \
+ static int callcnt = 0 PG_USED_FOR_ASSERTS_ONLY; \
+ do { \
+ if ((XLR_STATE).j) \
+ goto *((void *) (XLR_STATE).j); \
+ XLR_CASE(XLR_INIT_STATE); \
+ Assert(++callcnt == 1); \
+ } while (0)
+#define XLR_CASE(name) name:
+#define XLR_DISPATCH() goto *((void *) (XLR_STATE).j)
+#define XLR_LEAVE(name, code) do { \
+ (XLR_STATE).j = (&&name); return (code); \
+ XLR_CASE(name); \
+ } while (0)
+#define XLR_RETURN(code) \
+ do { \
+ Assert(--callcnt == 0); \
+ (XLR_STATE).j = (&&XLR_INIT_STATE); return (code); \
+ } while (0)
+#define XLR_SWITCH_END()
+#else /* !XLR_USE_COMPUTED_GOTO */
+#define XLR_SWITCH() \
+ /* Don't call duplicatedly */ \
+ static int callcnt = 0 PG_USED_FOR_ASSERTS_ONLY; \
+ switch ((XLR_STATE).c) { \
+ XLR_CASE(XLR_INIT_STATE); \
+ Assert(++callcnt == 1); \
+#define XLR_CASE(name) case name:
+#define XLR_DISPATCH() goto starteval
+#define XLR_LEAVE(name, code) \
+ do { \
+ (XLR_STATE).c = (name); return (code); \
+ XLR_CASE(name); \
+ } while (0)
+#define XLR_RETURN(code) \
+ do { \
+ Assert(--callcnt == 0); \
+ (XLR_STATE).c = (XLR_INIT_STATE); return (code); \
+ } while (0)
+#define XLR_SWITCH_END() }
+#endif /* XLR_USE_COMPUTED_GOTO */
+
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index f3bae0bf49..30500c35c7 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -240,6 +240,9 @@ extern bool DecodeXLogRecord(XLogReaderState *state, XLogRecord *record,
#define XLogRecBlockImageApply(decoder, block_id) \
((decoder)->blocks[block_id].apply_image)
+/* Reset the reader state */
+#define XLREAD_RESET(state) ((state)->xlnd_state.j = (state)->xlread_state.j = 0)
+
extern bool RestoreBlockImage(XLogReaderState *recoder, uint8 block_id, char *dst);
extern char *XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len);
extern bool XLogRecGetBlockTag(XLogReaderState *record, uint8 block_id,
--
2.16.3
----Next_Part(Thu_Apr_18_21_02_57_2019_406)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="0002-Make-ReadPageInternal-a-state-machine.patch"
^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH 01/10] Define macros to make XLogReadRecord a state machine
@ 2019-04-18 01:22 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2019-04-18 01:22 UTC (permalink / raw)
To minimize apparent impact on code, use some macros as syntax
sugar. This is a similar stuff with ExecInterpExpr but a bit
different. The most significant difference is that this stuff allows
some functions are leaved midst of their work then continue. Roughly
speaking this is used as the follows.
enum retval
some_func()
{
static .. internal_variables;
XLR_SWITCH(INITIAL_STATUS);
...
XLR_LEAVE(STATUS1, RETVAL_CONTINUE);
...
XLR_LEAVE(STATUS2, RETVAL_CONTINUE2);
...
XLR_SWITCH_END();
XLR_RETURN(RETVAL_FINISH);
}
The caller uses the function as follows:
while (some_func() != RETVAL_FINISH)
{
<do some work>;
}
---
src/backend/access/transam/xlogreader.c | 83 +++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 41dae916b4..69d20e1f2f 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -29,6 +29,89 @@
#include "utils/memutils.h"
#endif
+/*
+ * Use computed-goto-based state dispatch when computed gotos are available.
+ * But use a separate symbol so that it's easy to adjust locally in this file
+ * for development and testing.
+ */
+#ifdef HAVE_COMPUTED_GOTO
+#define XLR_USE_COMPUTED_GOTO
+#endif /* HAVE_COMPUTED_GOTO */
+
+/*
+ * The state machine functions relies on static local variables. They cannot
+ * be reentered after non-local exit using ereport/elog for consistency. The
+ * assertion macros protect the functions from reenter after non-local exit.
+ */
+#ifdef USE_ASSERT_CHECKING
+#define XLR_REENT_PROTECT_ENTER() \
+ do { Assert(!__xlr_running); __xlr_running = true; } while (0)
+#define XLR_REENT_PROTECT_LEAVE()\
+ do { __xlr_running = false; } while (0)
+#else
+#define XLR_REENT_PROTECT_ENTER()
+#define XLR_REENT_PROTECT_LEAVE()
+#endif
+
+/*
+ * Macros for state dispatch.
+ *
+ * XLR_SWITCH - prologue code for state machine including switch itself.
+ * XLR_CASE - labels the implementation of named state.
+ * XLR_LEAVE - leave the function and return here at the next call.
+ * XLR_RETURN - return from the function and set state to initial state.
+ * XLR_END - just hides the closing brace if not in use.
+ */
+#if defined(XLR_USE_COMPUTED_GOTO)
+#define XLR_SWITCH(name) \
+ static bool __xlr_running PG_USED_FOR_ASSERTS_ONLY = false; \
+ static void *__xlr_init_state = &&name; \
+ static void *__xlr_state = &&name; \
+ do { \
+ XLR_REENT_PROTECT_ENTER(); \
+ goto *__xlr_state; \
+ XLR_CASE(name); \
+ } while (0)
+#define XLR_CASE(name) name:
+#define XLR_LEAVE(name, code) \
+ do { \
+ __xlr_state = (&&name); \
+ XLR_REENT_PROTECT_LEAVE(); \
+ return (code); \
+ XLR_CASE(name); \
+ } while (0)
+#define XLR_RETURN(code) \
+ do { \
+ __xlr_state = __xlr_init_state; \
+ XLR_REENT_PROTECT_LEAVE(); \
+ return (code); \
+ } while (0)
+#define XLR_SWITCH_END()
+#else /* !XLR_USE_COMPUTED_GOTO */
+#define XLR_SWITCH(name) \
+ static bool __xlr_running = false PG_USED_FOR_ASSERTS_ONLY; \
+ static int __xlr_init_state = name; \
+ static int __xlr_state = name; \
+ XLR_REENT_PROTECT_ENTER(); \
+ switch (__xlr_state) { \
+ XLR_CASE(name)
+#define XLR_CASE(name) case name:
+#define XLR_LEAVE(name, code) \
+ do { \
+ __xlr_state = (name); \
+ XLR_REENT_PROTECT_LEAVE(); \
+ return (code); \
+ XLR_CASE(name); \
+ } while (0)
+#define XLR_RETURN(code) \
+ do { \
+ __xlr_state = __xlr_init_state; \
+ XLR_REENT_PROTECT_LEAVE(); \
+ return (code); \
+ } while (0)
+#define XLR_SWITCH_END() }
+#endif /* XLR_USE_COMPUTED_GOTO */
+
static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr,
--
2.16.3
----Next_Part(Wed_Jul_10_13_18_10_2019_842)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v4-0002-Make-ReadPageInternal-a-state-machine.patch"
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: role self-revocation
@ 2022-03-11 16:51 Robert Haas <[email protected]>
2022-03-24 16:46 ` Re: role self-revocation Robert Haas <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Robert Haas @ 2022-03-11 16:51 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Peter Eisentraut <[email protected]>; David G. Johnston <[email protected]>; Joshua Brindle <[email protected]>; Mark Dilger <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Fri, Mar 11, 2022 at 11:34 AM Tom Lane <[email protected]> wrote:
> Note that either case would also involve making entries in pg_shdepend;
> although for the case of roles owned by/granted to the bootstrap
> superuser, we could omit those on the usual grounds that we don't need
> to record dependencies on pinned objects.
That makes sense to me, but it still doesn't solve the problem of
agreeing on role ownership vs. WITH ADMIN OPTION vs. something else.
I find it ironic (and frustrating) that Mark implemented what I think
is basically what you're arguing for, it got stuck because Stephen
didn't like it, we then said OK so let's try to find out what Stephen
would like, only to have you show up and say that it's right the way
he already had it. I'm not saying that you're wrong, or for that
matter that he's wrong. I'm just saying that if both of you are
absolutely bent on having it the way you want it, either one of you is
going to be sad, or we're not going to make any progress.
Never mind the fact that neither of you seem interested in even giving
a hearing to my preferred way of doing it. :-(
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: role self-revocation
2022-03-11 16:51 Re: role self-revocation Robert Haas <[email protected]>
@ 2022-03-24 16:46 ` Robert Haas <[email protected]>
2022-03-24 17:10 ` Re: role self-revocation Tom Lane <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Robert Haas @ 2022-03-24 16:46 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Peter Eisentraut <[email protected]>; David G. Johnston <[email protected]>; Joshua Brindle <[email protected]>; Mark Dilger <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Fri, Mar 11, 2022 at 11:51 AM Robert Haas <[email protected]> wrote:
> On Fri, Mar 11, 2022 at 11:34 AM Tom Lane <[email protected]> wrote:
> > Note that either case would also involve making entries in pg_shdepend;
> > although for the case of roles owned by/granted to the bootstrap
> > superuser, we could omit those on the usual grounds that we don't need
> > to record dependencies on pinned objects.
>
> That makes sense to me, but it still doesn't solve the problem of
> agreeing on role ownership vs. WITH ADMIN OPTION vs. something else.
Notwithstanding the lack of agreement on that point, I believe that
what we should do for v15 is remove the session user
self-administration exception. We have pretty much established that it
was originally introduced in error. It later was found to be a
security vulnerability, and that resulted in the exception being
narrowed without removing it altogether. While there are differences
of opinion on what the larger plan here ought to be, nobody's proposal
plan involves retaining that exception. Neither has anyone offered a
plausible use case for the current behavior, so there's no reason to
think that removing it would break anything.
However, it might. And if it does, I think it would be best if
removing that exception were the *only* change in this area made by
that release. If for v16 or v17 or v23 we implement Plan Tom or Plan
Stephen or Plan Robert or something else, and along the way we remove
that self-administration exception, we're going to have a real fire
drill if it turns out that the self-administration exception was
important for some reason we're not seeing right now. If, on the other
hand, we remove that exception in v15, then if anything breaks, it'll
be a lot easier to deal with. Worst case scenario we just revert the
removal of that exception, which will be a very localized change if
nothing else has been done that depends heavily on its having been
removed.
So I propose to commit something like what I posted here:
http://postgr.es/m/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: role self-revocation
2022-03-11 16:51 Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 16:46 ` Re: role self-revocation Robert Haas <[email protected]>
@ 2022-03-24 17:10 ` Tom Lane <[email protected]>
2022-03-24 17:26 ` Re: role self-revocation Robert Haas <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2022-03-24 17:10 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Stephen Frost <[email protected]>; Peter Eisentraut <[email protected]>; David G. Johnston <[email protected]>; Joshua Brindle <[email protected]>; Mark Dilger <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Robert Haas <[email protected]> writes:
> Notwithstanding the lack of agreement on that point, I believe that
> what we should do for v15 is remove the session user
> self-administration exception. We have pretty much established that it
> was originally introduced in error.
Agreed.
> However, it might. And if it does, I think it would be best if
> removing that exception were the *only* change in this area made by
> that release.
Good idea, especially since it's getting to be too late to consider
anything more invasive anyway.
> So I propose to commit something like what I posted here:
> http://postgr.es/m/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com
+1, although the comments might need some more work. In particular,
I'm not sure that this bit is well stated:
+ * A role cannot have WITH ADMIN OPTION on itself, because that would
+ * imply a membership loop.
We already do consider a role to be a member of itself:
regression=# create role r;
CREATE ROLE
regression=# grant r to r;
ERROR: role "r" is a member of role "r"
regression=# grant r to r with admin option;
ERROR: role "r" is a member of role "r"
It might be better to just say "By policy, a role cannot have WITH ADMIN
OPTION on itself". But if you want to write a defense of that policy,
this isn't a very good one.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: role self-revocation
2022-03-11 16:51 Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 16:46 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 17:10 ` Re: role self-revocation Tom Lane <[email protected]>
@ 2022-03-24 17:26 ` Robert Haas <[email protected]>
2022-03-28 14:51 ` Re: role self-revocation Stephen Frost <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Robert Haas @ 2022-03-24 17:26 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Stephen Frost <[email protected]>; Peter Eisentraut <[email protected]>; David G. Johnston <[email protected]>; Joshua Brindle <[email protected]>; Mark Dilger <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Thu, Mar 24, 2022 at 1:10 PM Tom Lane <[email protected]> wrote:
> > However, it might. And if it does, I think it would be best if
> > removing that exception were the *only* change in this area made by
> > that release.
>
> Good idea, especially since it's getting to be too late to consider
> anything more invasive anyway.
I'd say it's definitely too late at this point.
> > So I propose to commit something like what I posted here:
> > http://postgr.es/m/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com
>
> +1, although the comments might need some more work. In particular,
> I'm not sure that this bit is well stated:
>
> + * A role cannot have WITH ADMIN OPTION on itself, because that would
> + * imply a membership loop.
>
> We already do consider a role to be a member of itself:
>
> regression=# create role r;
> CREATE ROLE
> regression=# grant r to r;
> ERROR: role "r" is a member of role "r"
> regression=# grant r to r with admin option;
> ERROR: role "r" is a member of role "r"
>
> It might be better to just say "By policy, a role cannot have WITH ADMIN
> OPTION on itself". But if you want to write a defense of that policy,
> this isn't a very good one.
That sentence is present in the current code, along with a bunch of
other sentences, which the patch renders irrelevant. So I just deleted
all of the other stuff and kept the sentence that is still relevant to
the revised code. I think your proposed replacement is an improvement,
but let's be careful not to get sucked into too much of a wordsmithing
exercise in a patch that's here to make a functional change.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: role self-revocation
2022-03-11 16:51 Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 16:46 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 17:10 ` Re: role self-revocation Tom Lane <[email protected]>
2022-03-24 17:26 ` Re: role self-revocation Robert Haas <[email protected]>
@ 2022-03-28 14:51 ` Stephen Frost <[email protected]>
2022-03-28 18:31 ` Re: role self-revocation Robert Haas <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Frost @ 2022-03-28 14:51 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; David G. Johnston <[email protected]>; Joshua Brindle <[email protected]>; Mark Dilger <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
Greetings,
* Robert Haas ([email protected]) wrote:
> On Thu, Mar 24, 2022 at 1:10 PM Tom Lane <[email protected]> wrote:
> > > However, it might. And if it does, I think it would be best if
> > > removing that exception were the *only* change in this area made by
> > > that release.
> >
> > Good idea, especially since it's getting to be too late to consider
> > anything more invasive anyway.
>
> I'd say it's definitely too late at this point.
Agreed.
> > > So I propose to commit something like what I posted here:
> > > http://postgr.es/m/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com
> >
> > +1, although the comments might need some more work. In particular,
> > I'm not sure that this bit is well stated:
Also +1 on this.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: role self-revocation
2022-03-11 16:51 Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 16:46 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 17:10 ` Re: role self-revocation Tom Lane <[email protected]>
2022-03-24 17:26 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-28 14:51 ` Re: role self-revocation Stephen Frost <[email protected]>
@ 2022-03-28 18:31 ` Robert Haas <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Robert Haas @ 2022-03-28 18:31 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; David G. Johnston <[email protected]>; Joshua Brindle <[email protected]>; Mark Dilger <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
On Mon, Mar 28, 2022 at 10:51 AM Stephen Frost <[email protected]> wrote:
> > > > So I propose to commit something like what I posted here:
> > > > http://postgr.es/m/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com
> > >
> > > +1, although the comments might need some more work. In particular,
> > > I'm not sure that this bit is well stated:
>
> Also +1 on this.
OK, done using Tom's proposed wording.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2022-03-28 18:31 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-09-19 23:51 SIGDANGER and oomd Thomas Munro <[email protected]>
2019-04-18 01:22 [PATCH 01/10] Define macros to make XLogReadRecord a state machine Kyotaro Horiguchi <[email protected]>
2019-04-18 01:22 [PATCH 01/10] Define macros to make XLogReadRecord a state machine Kyotaro Horiguchi <[email protected]>
2019-04-18 01:22 [PATCH 01/10] Define macros to make XLogReadRecord a state machine Kyotaro Horiguchi <[email protected]>
2022-03-11 16:51 Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 16:46 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-24 17:10 ` Re: role self-revocation Tom Lane <[email protected]>
2022-03-24 17:26 ` Re: role self-revocation Robert Haas <[email protected]>
2022-03-28 14:51 ` Re: role self-revocation Stephen Frost <[email protected]>
2022-03-28 18:31 ` Re: role self-revocation Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox