Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @knz)

-- commits line 4 at r2:
is it really?

-- commits line 19 at r2:
is it really?

-- commits line 42 at r6:
"cluster setting"

-- commits line 49 at r6:
do we really need this cluster setting? I'd vote against it.

pkg/sql/event_log.go line 128 at r2 (raw file):

	// For DDL statements, this is typically the ID of
	// the affected descriptor.
	targetID int32

please put a comment somewhere around the schema definition for system.eventlog saying that the respective column is not populated any more (or is populated with 0)

pkg/sql/event_log.go line 286 at r2 (raw file):

		prevDescID := m.DescriptorID
		*m = commonSQLEventDetails
		if m.DescriptorID == 0 {

I don't really understand what's going on here. Perhaps put a method comment explaining what should and should not be populated in commonSQLEventDetails.

pkg/sql/event_log.go line 534 at r2 (raw file):

	reportingID := ex.s.cfg.NodeInfo.NodeID.SQLInstanceID()
	const colsPerEvent = 5
	// Note: we insert the alue zero as targetID because sadly this

alue

pkg/sql/event_log.go line 534 at r2 (raw file):

	reportingID := ex.s.cfg.NodeInfo.NodeID.SQLInstanceID()
	const colsPerEvent = 5
	// Note: we insert the alue zero as targetID because sadly this

let's add a migration removing that constraint?

pkg/sql/event_log.go line 453 at r4 (raw file):

func insertEventRecords(
	ctx context.Context,
	execCfg *ExecutorConfig,

I'm not sure changing this param is an improvement. The main thing that's needed out of the ExecutorConfig is an InternalExecutor; for the rest of the needed things, the ExecutorConfig is not a very appropriate struct to pass. Between the two, I prefer it how it was.

pkg/sql/event_log.go line 406 at r6 (raw file):

)

var eventLogAsyncWritesMaxAttempts = settings.RegisterIntSetting(

I'd vote we hardcode a number instead of this setting

pkg/sql/event_log.go line 414 at r6 (raw file):

)

var eventLogAsyncWritesAttemptTimeout = settings.RegisterDurationSetting(

I'd vote we hardcode a timeout instead of this setting

pkg/sql/event_log.go line 461 at r6 (raw file):

	// filtering. Easiest is to pretend the event is sourced here.
	//
	// NB: insertEventRecords never returns an error when provided a nil txn.

instead of this NB, assert that the error is nil and put the explanation in the panic

pkg/sql/event_log.go line 551 at r6 (raw file):

	}
	// No: do them async.
	txn.AddCommitTrigger(func(ctx context.Context) {

I'm confused; doesn't this crash if txn is nil?

pkg/sql/event_log.go line 564 at r6 (raw file):

	stopper := execCfg.RPCContext.Stopper
	return stopper.RunAsyncTask(
		context.Background(), "record-events", func(bgCtx context.Context) {

why not use ctx?

pkg/sql/event_log.go line 598 at r6 (raw file):

}

func synchronousWriteToSystemEventsTable(

nit: remove synchronous from the name

This is ready for another look.

NB: the very last commit is currently expected to fail CI due to #86132. I'll rebase when the fix in #86153 merges.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)

-- commits line 4 at r2:

I've fixed the release justifications throughout.

-- commits line 19 at r2:

Done.

-- commits line 42 at r6:

Done.

-- commits line 49 at r6:

It's not public. We need it for certain logic tests (not all) which is why a testing knob won't do.

pkg/sql/event_log.go line 128 at r2 (raw file):

Done.

pkg/sql/event_log.go line 286 at r2 (raw file):

Added comments and extended the commit message to explain what's going on.

pkg/sql/event_log.go line 534 at r2 (raw file):

Done.

pkg/sql/event_log.go line 534 at r2 (raw file):

Added a TODO.

pkg/sql/event_log.go line 453 at r4 (raw file):

It's mostly for the benefit of the next commit. I've explained in the commit message.

pkg/sql/event_log.go line 406 at r6 (raw file):

OK, Done.

pkg/sql/event_log.go line 414 at r6 (raw file):

Done.

pkg/sql/event_log.go line 461 at r6 (raw file):

Done.

pkg/sql/event_log.go line 551 at r6 (raw file):

It did 😅
Ive fixed it since.

pkg/sql/event_log.go line 564 at r6 (raw file):

Because ctx is linked to the surrounding query's execution, and gets canceled when the execution stops. The async task writing the event to the log can run after the surrounding SQL statement has completed execution.

pkg/sql/event_log.go line 598 at r6 (raw file):

Done.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @knz)

-- commits line 49 at r6:

you can use a cluster-opt: ... line in logic tests to set testing knobs (and add support to logic.go for a new knob). If the tests can get away with setting the knob for the whole duration of the test (and I think here they do). I fell like a knob here would be preferable (even to a non-public setting), but up to you.

-- commits line 69 at r13:
I don't think this should be a separate commit.

pkg/sql/event_log.go line 260 at r9 (raw file):

// have access to a (*planner) and the current statement metadata.
//
// It does so by synthetizing the (shared) CommonSQLEventDetails,

Now what's going on is more clear because of the code comment, but I don't understand what "synthesize" means here. This method takes in commonSQLEventDetails, it doesn't seem to synthesize anything. I'd remove this paragraph.

pkg/sql/event_log.go line 486 at r12 (raw file):

// the run-time type of the event payload.
//
// If the txn field is non-nil and the cluster setting

This comment is hard to parse, in my opinion. If server.eventlog.sync_writes.enabled didn't exist, there'd be no need for it :P

pkg/sql/event_log.go line 490 at r12 (raw file):

// on the given txn object. Otherwise, an asynchronous task is spawned
// to do the write.
// If txn is nil, the event is written asynchronously.

even if txn is not nil, the write is still async (as a commit trigger), right? I think commit triggers should be mentioned in this comment.

pkg/sql/event_log.go line 585 at r12 (raw file):

	stopper := execCfg.RPCContext.Stopper
	if err := stopper.RunAsyncTask(
		context.Background(), "record-events", func(bgCtx context.Context) {

If origCtx stays named origCtx, then you can rename bgCtx to ctx, which would be better in my opinion.
As a nit, I would also rename origCtx to ctx as the parameter name (from the perspective of the function's caller, origCtx doesn't make sense), and I'd do a origCtx := ctx before the RunAsyncTask() call.

pkg/sql/event_log.go line 585 at r12 (raw file):

	stopper := execCfg.RPCContext.Stopper
	if err := stopper.RunAsyncTask(
		context.Background(), "record-events", func(bgCtx context.Context) {

please put a comment around here saying that we don't want to inherit the caller's cancelation

pkg/sql/event_log.go line 620 at r12 (raw file):

			}
		}); err != nil {
		log.Ops.Warningf(origCtx, "failed to start task to log %d events: %v", len(entries), err)

I don't think using the Ops channel here (as opposed to simply log.Infof makes sense). In fact, I wouldn't log this error at all, and definitely not at the warning level. If the server is shutting down and we can't write to the system table any more, I think it's just bad luck; I don't think anyone will benefit from these log message.
Same for the Warning above too.

I've merged the last two commits as recommended.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei)

-- commits line 49 at r6:

Done.

-- commits line 69 at r13:

Done.

pkg/sql/event_log.go line 260 at r9 (raw file):

Done.

pkg/sql/event_log.go line 486 at r12 (raw file):

Done.

pkg/sql/event_log.go line 490 at r12 (raw file):

Done.

pkg/sql/event_log.go line 585 at r12 (raw file):

Done.

pkg/sql/event_log.go line 585 at r12 (raw file):

Done.

pkg/sql/event_log.go line 620 at r12 (raw file):

The one above is definitely useful because perhaps the eventlog table is unavailable, and we want to see symptoms of that.

For the one here, rephrased as an assertion failure. This ensures we get a sentry report for it if it's ever hit.