Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack

Hello.

At Fri, 27 Jul 2018 11:31:23 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180727023123(dot)GE1754(at)paquier(dot)xyz>
> On Thu, Jul 26, 2018 at 11:14:31PM +0000, Bossart, Nathan wrote:
> > On 7/26/18, 3:42 PM, "Michael Paquier" <michael(at)paquier(dot)xyz> wrote:
> > Minus a possible documentation update, 0003 seems almost ready, too.
>
> The docs mentioned that shared catalogs are processed, so I did not
> bother, but visibly your comment is that we could be more precise about
> the ownership in this case? An attempt is attached.
>
> > For 0002, would adding vacuum_skip_rel() before and after
> > try_relation_open() in vacuum_rel() be enough? That way, we could
> > avoid emitting an ERROR for only the VACUUM FULL case (and skip it
> > with a WARNING like everything else).
>
> Er, we need a lock on it while looking at its data in vacuum_rel(), no?
> So the call to vacuum_skip_rel() needs to happen after
> try_relation_open(), once we are sure that the relation is opened.
> Having two set of checks is actually better as the operation can involve
> multiple operations.

I intended the same as Bossart said. It is not reasonable that
VACUUM and VACUUM FULL behaves differently on rejection for the
same reason. The objective of the first (or early) check is
rejecting (non-superuser's) unprivileged VACUUM FULL. Any
possible modifications on a syscache tuple before taking a lock
doesn't seem to harm in the point of view. Anyway the lock
acquired in expand_vacuum_rel is not held until vacuum_rel.

> The error messages generated by vacuum_skip_rel are not especially smart
> when elevel >= ERROR. As we need a proper errcode for that case as well
> just using a separate error message is less confusing. I have

Apart from the discussion above, it is uneasy for me that the
messages seem heavily affected by the callers context.

> implemented my idea in the updated set attached. Another issue I have
> found is that when doing for example a system-wide analyze, we would
> finish with spurious warnings, as toast relations need to be discarded
> from the first set of relations built.

It seems reasonable.

> Anyway, I have done more work on the patches, mainly I have fixed the
> calls to RangeVarGetRelidExtended using booleans. I have added

I think that it is useful to let callback reutrn bool value that
indicates whether stop or go. This allows WARNING in the
callback.

> isolation tests for cases which are cheap, aka those not involving a
> system-wide operation. Running those tests on HEAD, it is easy to see
> that TRUNCATE or VACUUM complete after a session doing a catalog lookup
> commits its transaction. VACUUM skips a relation and VACUUM FULL issues
> an error.
>
> Regarding those patches, I am pretty happy how things turn out for
> TRUNCATE and REINDEX, way less for VACUUM, so getting 0001 and 0003
> committed first makes the most sense to me as their logic is rather
> straight-forward (well way of speaking ;p ).

About 0001, I'm not sure what the "session-level" means but it
looks fine and works as expected. 0003 looks fine overall. I
think I heard that gender-free wording is appreciated but it
might be only about documentation.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center