Date: Thu, 11 Apr 2024 09:38:07 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
	"Leung, Anthony" <antholeu@amazon.com>,
	"Andrey M. Borodin" <x4mmm@yandex-team.ru>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Allow non-superuser to cancel superuser tasks.
Message-ID: <20240411143807.GC1882158@nathanxps13>
References: <ZhCPlBVRWnv4j-u4@paquier.xyz>
 <20240406010751.GA167028@nathanxps13>
 <ZhNv2yixxyfxD7Iy@paquier.xyz>
 <E35F20FF-4E2A-440C-8097-79B5748AD2DF@amazon.com>
 <ZhTXvFKMkdWDXqG0@paquier.xyz>
 <CALdSSPjVmZMcFm14dzzAGenssJAmV400wBYA=UNWMi9aJeXJMw@mail.gmail.com>
 <ZhXIH3JuKX34r905@paquier.xyz>
 <20240410150034.GA1673069@nathanxps13>
 <ZhcQ8Ki3Ck6VlYqu@paquier.xyz>
 <CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6=3TBKABu9C3_97YGOoMg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6=3TBKABu9C3_97YGOoMg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20240411143807.GC1882158%40nathanxps13>
Precedence: bulk

On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote:
>> It's feeling more natural here to check that we have a superuser-owned
>> backend first, and then do a lookup of the process type.
> 
>> I figured since there's no reason to rely on that behavior, we might as
>> well do a bit of future-proofing in case autovacuum workers are ever not
>> run as InvalidOid.
> 
> I have combined them into this:
> 
> if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId))
> && pgstat_get_backend_type(GetNumberFromPGProc(proc)) == B_AUTOVAC_WORKER)
> 
> This is both future-proofing and natural, I suppose. Downside of this
> is double checking condition (!OidIsValid(proc->roleId) ||
> superuser_arg(proc->roleId)), but i think that is ok for the sake of
> simplicity.

If we want to retain the check, IMO we might as well combine the first two
blocks like Anthony proposed:

	if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId))
	{
		ProcNumber procNumber = GetNumberFromPGProc(proc);
		PGBackendStatus procStatus = pgstat_get_beentry_by_proc_number(procNumber);

		if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER &&
			!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVAC_WORKER))
			return SIGNAL_BACKEND_NOAUTOVAC;
		else if (!superuser())
			return SIGNAL_BACKEND_NOSUPERUSER;
	}

+      <row>
+       <entry>pg_signal_autovacuum_worker</entry>
+       <entry>Allow signaling autovacuum worker backend to cancel or terminate</entry>
+      </row>

I think we need to be more specific about what pg_cancel_backend() and
pg_terminate_backend() do for autovacuum workers.  The code offers some
clues:

	/*
	 * SIGINT is used to signal canceling the current table's vacuum; SIGTERM
	 * means abort and exit cleanly, and SIGQUIT means abandon ship.
	 */
	pqsignal(SIGINT, StatementCancelHandler);
	pqsignal(SIGTERM, die);

+/* ----------
+ * pgstat_get_backend_type() -
+ *
+ * Return the backend type of the backend for the given proc number.
+ * ----------
+ */
+BackendType
+pgstat_get_backend_type(ProcNumber procNumber)
+{
+	PgBackendStatus *ret;
+
+	ret = pgstat_get_beentry_by_proc_number(procNumber);
+
+	if (!ret)
+		return B_INVALID;
+
+	return ret->st_backendType;
+}

I'm not sure we really need to introduce a new function for this.  I
avoided using it in my example snippet above.  But, maybe it'll come in
handy down the road...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com