Re: Parallel worker error

On Mon, Sep 4, 2017 at 5:46 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> It seems like the consensus is to move forward with this approach. I
> have written a patch implementing the above idea. Note, that to use
> SetCurrentRoleId, we need the value of guc "is_superuser" for the
> current user and we don't pass this value to parallel workers as this
> is PGC_INTERNAL guc variable. So, I have passed this value via
> FixedParallelState.

If I apply this patch and run 'make check', the tests all pass. If I
then revert all the changes to parallel.c and keep only the changes to
guc.c, the tests still pass. IOW, the part of this patch that makes
the tests pass is the changes to guc.c, that just skip saving and
restoring the role altogether.

It looks to me like we need to get all of the following variables from
miscinit.c set to the correct values:

static Oid AuthenticatedUserId = InvalidOid;
static Oid SessionUserId = InvalidOid;
static Oid OuterUserId = InvalidOid;
static Oid CurrentUserId = InvalidOid;
static bool AuthenticatedUserIsSuperuser = false;
static bool SessionUserIsSuperuser = false;
static int SecurityRestrictionContext = 0;
static bool SetRoleIsActive = false;

To do that, I think we first need to call InitializeSessionUserId(),
which will set AuthenticatedUserId and AuthenticatedUserIsSuperuser.
Then, we need to call SetSessionUserId(), which will set SessionUserId
and SessionUserIsSuperuser. Then, we need to call SetCurrentRoleId(),
which will set SetRoleIsActive and OuterUserId. Then finally we need
to call SetUserIdAndSecContext, which sets CurrentUserId and
SecurityRestrictionContext. The order matters, because the earlier
functions in this series overwrite the values set by later ones as a
side-effect. Furthermore, it matters in each case that the values
passed to those functions are taken from the corresponding variables
in the leader.

In the unpatched code, looking at ParallelWorkerMain(), we call
BackgroundWorkerInitializeConnectionByOid() first; that calls
InitializeSessionUserId(). Next we call RestoreGUCState(), which
because of the restore-the-role-last hack calls SetSessionUserId (when
session_authorization is restored) followed by SetCurrentRoleId()
(when role is restored). Finally it calls SetUserIdAndSecContext().

With your patch, the order is wrong. SetCurrentRoleId() is called
AFTER SetUserIdAndSecContext(). Furthermore, the role ID passed to
SetCurrentRoleId() is always the same one passed to
SetUserIdAndSecContext(). But those roles might be different.
fps->current_user_id is set by a call to GetUserIdAndSecContext(),
which reads CurrentUserId, not OuterUserId. I think you need to pass
the value returned by GetCurrentRoleId() as an additional element of
FixedParallelState.

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