Formally deprecate journal events?

[woodruffw]

Just wanted to record this proposal somewhere: the "Project journal" view has had a "deprecation coming soon" warning for a few years now:

Users are told to visit the security history view instead, which is both generally more detailed (per event) and has more event types. That view has existed since 2019 (#6339).

Given that the replacement has been around for a couple of years and that we've been warning about deprecation for as long, does it make sense to formally deprecate journal events and set a timeline for their removal?

[divbzero]

Deprecating journal entries seems to make sense to me. The deprecation warning and new events framework has been in place for about 3 years ago (from your PR #6339) and we also have generic events to add events to any model we would want in the future (from @di’s PR #8324).

What action would be taken to “formally” deprecating the project journal? Would it be announcing a specific date or version when the project journal view would be removed?

To help inform this discussion, do we have insight into how much the project journal page (/manage/project/{project.name}/journal/) is used?

[woodruffw]

What action would be taken to “formally” deprecating the project journal? Would it be announcing a specific date or version when the project journal view would be removed?

One of the PyPI maintainers/admins can correct me, but I think it would involve at minimum:

• Changing the current view's language from "will be deprecated" to "is deprecated", with a specific removal date.
• Making an announcement on discuss.python.org's packaging forum describing the deprecation

And then, in terms of the removal itself, it should mostly just be a matter of removing all uses of JournalEvent, deleting the routes and views, and maybe redirecting the current "project journal" route to the "security history" view instead.

[dstufft]

Is JournalEvent different than JournalEntry?

[dstufft]

Ok I see the context now, JournalEntry is what you're actually talking about.

The big thing that relies on the journal is the serial number is actually just the journal entry's ID number, so we would need to add something new to give the behavior that Bandersnatch et al rely on.

[woodruffw]

Is JournalEvent different than JournalEntry?

Huh, my response via email never appeared here...

Yeah, this was just me mixing up words 😅 -- JournalEntry is what I meant.

[woodruffw]

The big thing that relies on the journal is the serial number is actually just the journal entry's ID number, so we would need to add something new to give the behavior that Bandersnatch et al rely on.

I might be missing it, but where does the project's serial number (I'm assuming that's last_serial) depend on the journal entry's ID? I see it as a column on Project:

    last_serial = Column(Integer, nullable=False, server_default=sql.text("0"))

...and that we render it include it in the datamodel used to render the simple index:

def _simple_index(request, serial):
    # Fetch the name and normalized name for all of our projects
    projects = (
        request.db.query(Project.name, Project.normalized_name)
        .order_by(Project.normalized_name)
        .all()
    )

    return {
        "meta": {"api-version": API_VERSION, "_last-serial": serial},
        "projects": [{"name": p.name} for p in projects],
    }

[dstufft]

Most of our denormalization in Warehouse is managed by database level triggers, this one was added here.

CREATE OR REPLACE FUNCTION maintain_project_last_serial()
RETURNS TRIGGER AS $$
DECLARE
    targeted_name text;
BEGIN
    IF TG_OP = 'INSERT' THEN
        targeted_name := NEW.name;
    ELSEIF TG_OP = 'UPDATE' THEN
        targeted_name := NEW.name;
    ELSIF TG_OP = 'DELETE' THEN
        targeted_name := OLD.name;
    END IF;
    UPDATE packages
    SET last_serial = j.last_serial
    FROM (
        SELECT max(id) as last_serial
        FROM journals
        WHERE journals.name = targeted_name
    ) as j
    WHERE packages.name = targeted_name;
    RETURN NULL;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER update_project_last_serial
AFTER INSERT OR UPDATE OR DELETE ON journals
FOR EACH ROW EXECUTE PROCEDURE maintain_project_last_serial();

[woodruffw]

Makes sense -- if I'm understanding the semantics correctly, this only needs to be a monotonically increasing integer, right? In other words, we could theoretically set the last_serial to the current Unix timestamp (64 bits) whenever the relevant replacement security event is triggered?

Edit: Or maybe not on security events, since those aren't formally tied to projects and have high volume, but there's probably something we can trigger on.

[dstufft]

So funny enough, the original mirroring protocol literally used timestamps, as in "give me changes since X".

The serial method was added because there were changes being missed because if you had multiple things being uploaded in the same second as when a mirror says "give me all the changes since X" such that it got some of them but not all of them, the later ones would be skipped.

There's also clock skew problems with different uploaders maybe drifting from each other ¹.

We could work around it by munging things so that when a mirror says, "give me all the changes since X", we just subtract some number from that to give a bit of a window for variations like that. That would probably work? We'd be giving up some level of "we know the mirror will get exactly every change" for "we're pretty sure that unless something goes really weird, they'd get every change".

It looks like Bandersnatch records it's high-water mark for what serial it last saw by just doing max([serials_I_saw_in_this_recent_run]), so other than clock drift causing serials to go backwards, I think that would work fine.

Footnotes

1. Clocks can drift backwards too, but we could fix that by using the monotonic clock APIs. ↩

[brainwane]

Given that the replacement has been around for a couple of years and that we've been warning about deprecation for as long, does it make sense to formally deprecate journal events and set a timeline for their removal?

Yes. It's been long enough.

[woodruffw]

Okay, just to get the ball rolling: any objection to me tweaking the view's current language to emphasize that journal events are deprecated, but without a removal timeline yet?

Another thought I just had: a minimally disruptive change would be to remove the view itself and most of the content from JournalEvents, but the table (and its IDs) around for the time being. That way we could do the deprecation from a browser-facing side without breaking bandersnatch or any other mirroring software.

[dstufft]

Yea we can remove journals from the UI completely, that's not a problem

[woodruffw]

Cool. This isn't scoped in any of my current work, but I'm happy to spend some of my own time doing that this weekend/sometime in the evening this coming week 🙂

[dstufft]

It just occurred to me that we don't need to use something like a timestamp or anything, we can just keep using the database for this.

We get a monotonically increasing, globally unique and ordered serial number because every change inserts a record to the journals table and we use journals.id, but what makes journals.id suitable for this is that it's using the postgres SERIAL type which implements a counter that can only increase, and which you can get the next value from, which happens automatically when that id column is left blank.

But there is no requirement that SERIAL has to be used for primary keys or that it can only be incremented by omitting a value and letting the DB fill in the default value, we could do it by turning Project.last_serial into a database SERIAL column, and whenever something happens that triggers a mirror to need to resync, then we just need to run a query like:

UPDATE projects
SET last_serial = nextval(pg_get_serial_sequence("projects", "last_serial"))
WHERE id = %s

When writing that, it occurs to me that even the SERIAL solution can have the problem of missed updates, because while the sequence guarantees uniqueness and that the serial will be generated in order, it doesn't guarantee that they will be committed in order, so you can still get into situations where two transactions happen A and B which get assigned serial numbers 1 and 2. Then B commits with serial 2, a mirror queries and gets the latest serial is 2, then A commits with serial 1, but gets skipped because 1 < 2.

We could introduce the same kind of window as talked about in the previous comment, go back N changes, but that's kind of hard because we don't actually know how many of those changes were created concurrently, whereas using a timestamp means that we can infer meaning from the serial, and make smart choices about a look back window.

We can even work around the clock skew between multiple nodes problem by letting the database assign our timestamp serial, since we only have 1 master database and we're unlikely to ever have more than that ¹, and just use something like:

UPDATE porjects
SET last_serial = extract(epoch from now())
WHERE id = %s

So we'd only have to worry about the PG server drifting backwards, which we're on RDS and I assume they're running ntp to keep that from happening, so that might be the best option, and then just introduce some 30s or 60s look back window.

Footnotes

1. Multi writer PostgreSQL deployments are hard and the write traffic to PyPI is relatively tiny. We're currently not even a multiple server setup for reads, which is far far more likely to happen. ↩